Bookmarks toolbar buttons exposed to accessibility even when hidden (due to CSS visibility: visible)
Categories
(Firefox :: Toolbars and Customization, defect, P3)
Tracking
()
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
- Enable VoiceOver
- Focus the web area with the VO cursor
- 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.
Comment 2•3 years ago
|
||
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.
Reporter | ||
Comment 3•3 years ago
|
||
(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?
Comment 4•3 years ago
|
||
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.
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
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.
Assignee | ||
Comment 6•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 7•3 years ago
|
||
Morgan, can you test out the attached patch? I didn't test it with accessibility tools like VoiceOver.
Reporter | ||
Comment 8•3 years ago
|
||
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!
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
Comment 10•3 years ago
|
||
Backed out for causing failures in browser_toolbar_overflow.js.
Backout link: https://hg.mozilla.org/integration/autoland/rev/d388917afca4c1035b38707bf994c38171e0e8be
Failure log: https://treeherder.mozilla.org/logviewer?job_id=352818277&repo=autoland&lineNumber=11954
Comment 11•3 years ago
|
||
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.
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Comment 12•2 years ago
|
||
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?
Assignee | ||
Updated•2 years ago
|
Comment 13•2 years ago
|
||
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.
Comment 14•2 years ago
|
||
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. :-)
Assignee | ||
Comment 15•2 years ago
|
||
(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 becausetestEle.style.visibility
will always beundefined
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?
Comment 16•2 years ago
|
||
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
Assignee | ||
Comment 17•2 years ago
|
||
I just transferred ownership of the patch to you :)
Updated•2 years ago
|
Comment 18•2 years ago
|
||
Sweet! I've updated the patch with the changes, so it should be good to go :)
Comment 19•2 years ago
|
||
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
Comment 20•2 years ago
|
||
bugherder |
Updated•10 months ago
|
Description
•