Closed Bug 1731889 Opened 3 years ago Closed 2 years ago

Bookmarks toolbar buttons exposed to accessibility even when hidden (due to CSS visibility: visible)

Categories

(Firefox :: Toolbars and Customization, defect, P3)

defect

Tracking

()

RESOLVED FIXED
101 Branch
Accessibility Severity s3
Tracking Status
firefox101 --- fixed

People

(Reporter: morgan, Assigned: jaws)

Details

(Keywords: access)

Attachments

(1 file)

STR:
0. Add bookmarks to the bookmarks bar, ensure toolbar is hidden

  1. Enable VoiceOver
  2. Focus the web area with the VO cursor
  3. Attempt to reach the navigation toolbar with VO+previous

Expected:
VO moves focus from the web area to the navigation toolbar

Actual:
VO focuses an invisible node in the upper left corner of the screen, between the chrome and the web area, and reads your bookmark toolbar bookmarks in order. After navigating through them, VO focuses the navigation toolbar.

Triaging

Severity: -- → S3
Priority: -- → P3

Ah, yes. I came across this when I was implementing toolbar keyboard navigation. It happens because even though the toolbar gets visibility: collapse, the buttons inside it have visibility: visible (computed style). I don't know why or how they are hidden visually in that state.

I'd say this should be moved into the toolbar component and addressed there.

(In reply to James Teh [:Jamie] from comment #2)

Ah, yes. I came across this when I was implementing toolbar keyboard navigation. It happens because even though the toolbar gets visibility: collapse, the buttons inside it have visibility: visible (computed style). I don't know why or how they are hidden visually in that state.

I'd say this should be moved into the toolbar component and addressed there.

Oh interesting -- so this isn't a VO specific bug?

No; those buttons are incorrectly exposed to a11y on Windows when invisible too. However, Windows users don't tend to use screen reader review commands as much in an app like Firefox, so it's not really "noticeable" for most users on Windows.

Severity: S3 → --
Component: Disability Access APIs → Toolbars and Customization
Keywords: access
Product: Core → Firefox
Whiteboard: [access-s3]
OS: macOS → All
Hardware: Desktop → All
Summary: Bookmarks toolbar remains navigable with VO when hidden → Bookmarks toolbar buttons exposed to accessibility even when hidden (due to CSS visibility: visible)
Priority: P3 → --

https://searchfox.org/mozilla-central/rev/f62d42b1d98e67dc3da05d586f71103df02b8c4a/browser/components/places/content/browserPlacesViews.js#1350 explicity sets visibility to "visible". I wonder if we can instead call element.style.removeProperty().

Setting severity to S4 based on comment 4.

Severity: -- → S4
Priority: -- → P3
Assignee: nobody → jaws
Status: NEW → ASSIGNED

Morgan, can you test out the attached patch? I didn't test it with accessibility tools like VoiceOver.

Flags: needinfo?(mreschenberg)

Tested with VO:
If I hide the bookmarks bar, VO doesn't navigate it -- it jumps straight from the nav toolbar to the web area.
If I show the bookmarks bar, VO navs it like normal :)

Looks good to me!

Flags: needinfo?(mreschenberg)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0d24b8d7d30
Remove the 'visibility' property instead of setting it to 'visible' so a11y tools don't read these when their parent is hidden. r=morgan

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jaws, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(mreschenberg)
Flags: needinfo?(jaws)
Flags: needinfo?(mreschenberg)

I've looked through the failing test and I really don't understand why the node not having the visiblity style causes the children collection to have a different size.

Marco, do you see what I could be missing here?

Flags: needinfo?(jaws)
Flags: needinfo?(mak)

The severity field for this bug is set to S4. However, the accessibility severity is higher, [access-s3].
:jaws, could you consider increasing the severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jaws)

Hey it seems like it's just that the tests need to be updated to use getComputedStyle(testEle).visibility. The reason is because testEle.style.visibility will always be undefined for visible elements since we are removing the visibility property instead of setting it. This breaks test_overflow on line 48 https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_toolbar_overflow.js#48 as the "visibleNodes" wouldn't actually find the visible nodes.

I tested the changes locally and applying these changes seems to result in everything working as expected.

I've made a comment on phabricator with the diff. :-)

(In reply to gliu20 from comment #14)

Hey it seems like it's just that the tests need to be updated to use getComputedStyle(testEle).visibility. The reason is because testEle.style.visibility will always be undefined for visible elements since we are removing the visibility property instead of setting it. This breaks test_overflow on line 48 https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_toolbar_overflow.js#48 as the "visibleNodes" wouldn't actually find the visible nodes.

I tested the changes locally and applying these changes seems to result in everything working as expected.

I've made a comment on phabricator with the diff. :-)

Thanks! I haven't had the time to come back to this. Would you like to take over the patch there?

Flags: needinfo?(mak)
Flags: needinfo?(jaws)
Flags: needinfo?(gliu10000)

Sure! Should I be submitting a new patch? I'm an outside contributor so I'm not sure I have permissions to commandeer revision--the only options I have are accept revision and request changes? https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#other-revision-actions

Flags: needinfo?(gliu10000) → needinfo?(jaws)

I just transferred ownership of the patch to you :)

Flags: needinfo?(jaws)
Attachment #9242715 - Attachment description: Bug 1731889 - Remove the 'visibility' property instead of setting it to 'visible' so a11y tools don't read these when their parent is hidden. → Bug 1731889 - Remove the 'visibility' property instead of setting it to 'visible' so a11y tools don't read these when their parent is hidden. r?morgan,jaws

Sweet! I've updated the patch with the changes, so it should be good to go :)

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25fa7e029b5b
Remove the 'visibility' property instead of setting it to 'visible' so a11y tools don't read these when their parent is hidden. r=morgan,jaws
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch
Accessibility Severity: --- → s3
Whiteboard: [access-s3]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: