Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VPN-5783: Change IPInfoPanel's a11y navigation order so that content is after the close button #8596

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

vinocher
Copy link
Contributor

@vinocher vinocher commented Nov 20, 2023

Description

The IPInfoPanel was before its close button in the QML, and focus was on the close button when the panel was opened. This meant that the user needed to navigate backwards from the Close button to read the panel text using a screen reader, which is not intuitive. Fixed by moving IPInfoPanel after its Close button. This allows the user to read the panel's text by navigating forward in a screen reader.

Also fixed the following:
Previously the IPInfoPanel had its z-order set to the same z-order as the buttons, which hid some visibility bugs. Specifically, the visible properties of connectionInfoToggleButton and ipInfoToggleButton were not correct. connectionInfoToggleButton should be visible only if the IPInfoPanel is not opened.

Reference

VPN-5783, GitHub 8423

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

Copy link
Contributor

@MattLichtenstein MattLichtenstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else looks good.

Do you think it would make sense to move the VPNToggle after the IpInfo button while we're fixing navigation order here. Seems weird to me that we go from Connection info button > VPN toggle > IpInfo button when the VPN is on

src/ui/screens/home/controller/ControllerView.qml Outdated Show resolved Hide resolved
@vinocher
Copy link
Contributor Author

Do you think it would make sense to move the VPNToggle after the IpInfo button while we're fixing navigation order here. Seems weird to me that we go from Connection info button > VPN toggle > IpInfo button when the VPN is on

Yes, that seems like the correct order. However, that will mean that a keyboard user will need to do an extra tab to reach the Toggle button, the most important part of the UI. Also, not sure if we have any tests that depend on that order. Perhaps this should be done in another change.

@MattLichtenstein
Copy link
Contributor

Yes, that seems like the correct order. However, that will mean that a keyboard user will need to do an extra tab to reach the Toggle button, the most important part of the UI.

I'd think it makes more sense to have to have keyboard navigation flow in a natural and expected order than to save the user one extra button press.

Also, not sure if we have any tests that depend on that order. Perhaps this should be done in another change.

Don't believe we have any tests that verify tab order, but I am totally fine with coming back to this in the future

@vinocher
Copy link
Contributor Author

I'd think it makes more sense to have to have keyboard navigation flow in a natural and expected order than to save the user one extra button press.
Don't believe we have any tests that verify tab order, but I am totally fine with coming back to this in the future

Yes, it makes sense to do this now. Fixed.

Copy link
Contributor

@MattLichtenstein MattLichtenstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@vinocher vinocher merged commit 3c9de54 into main Nov 21, 2023
114 of 118 checks passed
@vinocher vinocher deleted the vinocher/connectionInfoNavOrder branch November 21, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants