Closed
Bug 977267
Opened 10 years ago
Closed 10 years ago
[a11y] autocomplete suggestions not visible in screen reader
Categories
(DevTools :: Console, defect, P2)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: rcampbell, Assigned: Jamie, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: access, Whiteboard: [lang=js])
Attachments
(1 file, 3 obsolete files)
10.02 KB,
patch
|
Jamie
:
review+
|
Details | Diff | Splinter Review |
console's input line's autocomplete popup does not display results in screen reader. autocomplete list items should have aria-live-region attribute set. q.v., Firebug.
Updated•10 years ago
|
Priority: -- → P2
Whiteboard: [mentor=msucan][lang=js]
Updated•10 years ago
|
Mentor: mihai.sucan
Whiteboard: [mentor=msucan][lang=js] → [lang=js]
Updated•10 years ago
|
Assignee: nobody → schalk.neethling.bugs
Updated•10 years ago
|
Assignee: schalk.neethling.bugs → nobody
Comment 1•10 years ago
|
||
Jamie, I believe the patch you attached to bug 305206 is better suited for this bug. That bug is about the browser console (ctrl+shift+j), but you are working on the web console (ctrl+shift+k), which is what *this* bug is about.
Comment 2•10 years ago
|
||
Transferring the discussion over to this bug, since it is more appropriate. Below feedback is based on attachment 8474478 [details] [diff] [review]: (In reply to James Teh [:Jamie] from bug 305206 comment #46) > This patch: > * makes the console output a live region (except for input from the user); > * stops the autocomplete from being treated as an alert for accessibility; > and > * focuses the selected autocomplete item for accessibility. This works really awesome! I tried the patch in a local build, and the web console behaves so much nicer with both speech and braille. > 1. What are your thoughts on the output area? I guess it'd be super nice if > the items were focusable list items, but I'm not sure it's strictly > necessary, as you can get at these easily with, say, NVDA object navigation. > (I'm partially tempted to give it role="document" so you can just read it > with, say, NVDA browse mode. This seems a bit extreme, though.) I think either would be good. If browse mode, we should make sure that items that can be activated are activatable. I kind of like the idea of putting this in a virtual document. You can submit two alternate patches if you like and we can play with either a bit to see if it works. A list of items where navigation would work with the arrow keys would, however, be more appropriate for keyboard users. e. g. one tab stop on this list, or F6/Shift+F6, and then select the items with the arrow keys and enter to do something with it. > 2. Should the links in the output area be tabbable? This doesn't make sense > to me if the items themselves aren't, but the question is whether this will > break keyboard users who aren't also screen reader users. I doubt it, since > right now, they can't interact with the items in any other way except for > the links. See above. If we go with the route of a virtual documents, the links should definitely be tabable. > 3. Even aside from (1) and (2), I think the live region and autocomplete > stuff is a huge improvement already. Should we split this into multiple bugs? Only if you find that the stuff from user input is going to be bigger. Small bugs have the advantage that their changes are easy to track and one improvement can land if the other needs more work.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #2) > > 1. What are your thoughts on the output area? I guess it'd be super nice if > > the items were focusable list items... > > (I'm partially tempted to give it role="document" so you can just read it > > with, say, NVDA browse mode. ... > I think either would be good. If browse mode, we should make sure that items > that can be activated are activatable. Note that they can be activated without being tabbable; see below. > A list of items where > navigation would work with the arrow keys would, however, be more > appropriate for keyboard users. That would definitely have UI/UX implications, as these aren't currently focusable. If we were going to do that, I'd prefer to do this separately. > e. g. one tab stop on this list, or > F6/Shift+F6, and then select the items with the arrow keys and enter to do > something with it. We'd probably need a way to focus any links separately from the list items themselves. > > 2. Should the links in the output area be tabbable? > See above. If we go with the route of a virtual documents, the links should > definitely be tabable. If we set tabindex="-1", they can be activated without being tabbable. The problem with them being tabbable is that it clutters the tab order, which makes it difficult to get to the toolbar.
Comment 4•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #3) > (In reply to Marco Zehe (:MarcoZ) from comment #2) > Note that they can be activated without being tabbable; see below. Sounds good! > We'd probably need a way to focus any links separately from the list items > themselves. Well, as long as when one presses Enter on a list item while it has focus, and it activates the link, that might work, too, don't you think? > If we set tabindex="-1", they can be activated without being tabbable. The > problem with them being tabbable is that it clutters the tab order, which > makes it difficult to get to the toolbar. Sounds good!
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #4) > > We'd probably need a way to focus any links separately from the list items > > themselves. > Well, as long as when one presses Enter on a list item while it has focus, > and it activates the link, that might work, too, don't you think? That assumes there's nothing special about clicking on or activating the context menu for the item itself. I'm not sure at this stage.
Comment 6•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #5) > (In reply to Marco Zehe (:MarcoZ) from comment #4) > > > We'd probably need a way to focus any links separately from the list items > > > themselves. > > Well, as long as when one presses Enter on a list item while it has focus, > > and it activates the link, that might work, too, don't you think? > That assumes there's nothing special about clicking on or activating the > context menu for the item itself. I'm not sure at this stage. I think we can deal with this separately. I think the patch on bug 305206 is actually already improving things by a huge magnitude. Since the code is shared, I'll f+ it there if you like.
Assignee | ||
Comment 7•10 years ago
|
||
Bug 588010 suggests that everything that can be activated is now tabbable and can be activated via the keyboard. Removing these from the tab order without an alternative would therefore be a regression.
Assignee | ||
Comment 8•10 years ago
|
||
Bug 305206 isn't the appropriate place for this after all. I've split the autocomplete stuff out of that patch and will file another bug for the output area. Note that comment 0 mentions aria-live, but this isn't the ideal way to handle accessibility for autocompletes, hence aria-activedescendant approach used in this patch.
Comment 9•10 years ago
|
||
Comment on attachment 8477051 [details] [diff] [review] Patch Redirecting review request as per IRC. :)
Attachment #8477051 -
Flags: review?(rcampbell) → review?(bgrinstead)
Comment 10•10 years ago
|
||
Comment on attachment 8477051 [details] [diff] [review] Patch Review of attachment 8477051 [details] [diff] [review]: ----------------------------------------------------------------- 1) We should add a test case for this, basically making sure that the aria-activedescendant gets updated as the selectedIndex changes (and removed as it is cleared / destroyed). I think the file browser_webconsole_bug_585991_autocomplete_popup.js would be a good place to put it, we should be able to slide a few assertions in the various cases here. 2) We should make sure that clearItems() is called on destroy() inside of the autocomplete popup to ensure that the activedescendant is cleared in that case. ::: browser/devtools/shared/autocomplete-popup.js @@ +344,5 @@ > this._list.selectedIndex = aIndex; > if (this.isOpen && this._list.ensureIndexIsVisible) { > this._list.ensureIndexIsVisible(this._list.selectedIndex); > } > + if (aIndex == -1) { You could replace this condition with (!this._list.selectedItem), which would also cover an invalid index being passed in when it was already null. Basically, it would bail out in the second statement in this case instead of continuing on and throwing an error: this.selectedIndex = -1; this.selectedIndex = 100000; @@ +349,5 @@ > + // Return accessibility focus to the input. > + this._document.activeElement.removeAttribute("aria-activedescendant"); > + return; > + } > + let item = this._list.selectedItem; I'd prefer to see the logic for generating the IDs inside of appendItem, set on the listItem variable. Then here we can just assume that this._list.selectedItem.id is available. So all of this could be narrowed down to a single line: this._document.activeElement.setAttribute("aria-activedescendant", this._list.selectedItem.id);
Attachment #8477051 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 11•10 years ago
|
||
Thanks for the review! (In reply to Brian Grinstead [:bgrins] from comment #10) > 2) We should make sure that clearItems() is called on destroy() inside of > the autocomplete popup to ensure that the activedescendant is cleared in > that case. destroy() calls hidePopup(), which clears aria-activedescendant. There shouldn't be a case where the popup is hidden and aria-activedescendant is set. Or am I missing something? > ... So all of this could be narrowed > down to a single line: > this._document.activeElement.setAttribute("aria-activedescendant", > this._list.selectedItem.id); That'd mean we're fetching this._list.selectedItem twice (the first time for the bail out condition above). I wasn't sure whether calling this._list.selectedItem might be expensive, hence the variable. Is it definitely cheap enough to do this?
Assignee | ||
Comment 12•10 years ago
|
||
When I run this from the root of my checkout on Windows: ./mach mochitest-browser browser/devtools/webconsole/ it reports that it's running 0 tests (and obviously, it does just that). How do I run these tests?
Comment 13•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #12) > When I run this from the root of my checkout on Windows: > ./mach mochitest-browser browser/devtools/webconsole/ > it reports that it's running 0 tests (and obviously, it does just that). How > do I run these tests? ./mach mochitest-devtools browser/devtools/webconsole/test
Assignee | ||
Comment 14•10 years ago
|
||
This patch: 1. Adds tests. - I can't actually check that aria-activedescendant is *correct*, only that it changes when appropriate. Checking it properly would require access to the internal richlistitems, which seems inappropriate. - I do get the document using popup._document, which is private. This isn't ideal, but I don't know of a better way to do this which isn't insanely convoluted. 2. Applies feedback, except clearing aria-activedescendant in destroy(), which I don't think is necessary as per comment 11. 3. Updates accessibility focus appropriately when the selectedItem property is set. Previously, it was only updated for selectedIndex.
Attachment #8477051 -
Attachment is obsolete: true
Attachment #8479848 -
Flags: review?(bgrinstead)
Comment 15•10 years ago
|
||
Comment on attachment 8479848 [details] [diff] [review] Patch v2 Review of attachment 8479848 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/autocomplete-popup.js @@ +306,5 @@ > + * @private > + */ > + _updateAccessibilityForSelected: function AP__updateAccessibilityForSelected() > + { > + if (!this._list.selectedItem) { This and the whole block should be indented by two spaces. The end of the method looks correct, but these lines at the top are missing an indentation.
Comment 16•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #11) > Thanks for the review! > > (In reply to Brian Grinstead [:bgrins] from comment #10) > > 2) We should make sure that clearItems() is called on destroy() inside of > > the autocomplete popup to ensure that the activedescendant is cleared in > > that case. > destroy() calls hidePopup(), which clears aria-activedescendant. There > shouldn't be a case where the popup is hidden and aria-activedescendant is > set. Or am I missing something? You are right, it doesn't need to change
Comment 17•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #11) > > ... So all of this could be narrowed > > down to a single line: > > this._document.activeElement.setAttribute("aria-activedescendant", > > this._list.selectedItem.id); > That'd mean we're fetching this._list.selectedItem twice (the first time for > the bail out condition above). I wasn't sure whether calling > this._list.selectedItem might be expensive, hence the variable. Is it > definitely cheap enough to do this? Yes, that won't be a problem.
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #15) > This and the whole block should be indented by two spaces. Arrrg! Sorry. I got a bit overzealous when I was trimming indentation. :) > The end of the > method looks correct Actually, the last line of the method needs to be indented more than usual because it's wrapping from the previous line. What does Mozilla do here? I see Mozilla code sometimes indents to the bracket of the function call, but that would result in a massive indent here. I went for a normal logical level indent.
Comment 19•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #18) > > The end of the > > method looks correct > Actually, the last line of the method needs to be indented more than usual > because it's wrapping from the previous line. What does Mozilla do here? I > see Mozilla code sometimes indents to the bracket of the function call, but > that would result in a massive indent here. I went for a normal logical > level indent. But that's exactly what we usually do. For the sighties who like the stuff lined up nicely. Same goes for if statements that need to wrap. They end on the && or whatever one needs as an operator, and the next statement starts at the same level as the first character following the opening paren of the if statement, so all the conditions line up neatly below one another. Not exactly comfortable for a braille display, I know, but that's how it's done.
Comment 20•10 years ago
|
||
Comment on attachment 8479848 [details] [diff] [review] Patch v2 Review of attachment 8479848 [details] [diff] [review]: ----------------------------------------------------------------- Please also add a commit message to the patch ::: browser/devtools/shared/autocomplete-popup.js @@ +304,5 @@ > + * Update accessibility appropriately when the selected item is changed. > + * > + * @private > + */ > + _updateAccessibilityForSelected: function AP__updateAccessibilityForSelected() May as well name this something like _updateAriaActiveDescendant since that is all the function does. We could always rename it if it ends up doing more accessibility stuff ::: browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_popup.js @@ +46,5 @@ > > is(popup.selectedIndex, 2, > "Index of the first item from bottom is selected."); > is(popup.selectedItem, items[2], "First item from bottom is selected"); > + let oldAriaActiveDesc = input.getAttribute("aria-activedescendant"); Instead of keeping track of oldActiveDesc/newActiveDesc, I would suggest you have a function declared up by the input declaration like this: function getActiveDescendant() { return input.ownerDocument.getElementById( input.getAttribute("aria-activedescendant")); } Then any conditions for selectedItem in the test can immediately be followed by a check to the active descendant, like so: is(popup.selectedItem, items[1], "item1 is selected"); ok(getActiveDescendant().selected, "Aria active descendant is correct"); Since we just confirmed that the correct element is selected in the previous assertion, checking to make sure that the richlistitem is selected should be a sufficient check. You could keep the checks that are making sure there is no active descendant as-is, though: ok(!input.hasAttribute("aria-activedescendant"), "no aria-activedescendant");
Attachment #8479848 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Marco Zehe (:MarcoZ) from comment #19) > > I > > see Mozilla code sometimes indents to the bracket of the function call, but > > that would result in a massive indent here. > But that's exactly what we usually do. For the sighties who like the stuff > lined up nicely. Fair enough. What if the called function (including indentation and path) was, say, 60 characters long, but its arguments were more than 20 characters? That's entirely possible.
Comment 22•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #21) > Fair enough. What if the called function (including indentation and path) > was, say, 60 characters long, but its arguments were more than 20 > characters? That's entirely possible. I honestly don't know.
Assignee | ||
Comment 23•10 years ago
|
||
Applying feedback. Thanks again for review.
Attachment #8479848 -
Attachment is obsolete: true
Attachment #8480296 -
Flags: review?(bgrinstead)
Comment 24•10 years ago
|
||
Comment on attachment 8480296 [details] [diff] [review] Patch v3 Review of attachment 8480296 [details] [diff] [review]: ----------------------------------------------------------------- Please add r=bgrins to commit message. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3558090c1466
Attachment #8480296 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #24) > Please add r=bgrins to commit message. Shall do. I assume it's correct to only do this in a subsequent patch once I get r+, though? > Pushed to try: > https://tbpl.mozilla.org/?tree=Try&rev=3558090c1466 That report shows test failures for Linux, Windows 7 and Windows 8 (but not other platforms) in chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_markupview_events-overflow.js. However, i don't see how that could be caused by this patch. How do we proceed from here?
Comment 26•10 years ago
|
||
(In reply to James Teh [:Jamie] from comment #25) > (In reply to Brian Grinstead [:bgrins] from comment #24) > > Please add r=bgrins to commit message. > Shall do. I assume it's correct to only do this in a subsequent patch once I > get r+, though? > > > Pushed to try: > > https://tbpl.mozilla.org/?tree=Try&rev=3558090c1466 > That report shows test failures for Linux, Windows 7 and Windows 8 (but not > other platforms) in > chrome://mochitests/content/browser/browser/devtools/markupview/test/ > browser_markupview_events-overflow.js. However, i don't see how that could > be caused by this patch. How do we proceed from here? Yeah that was unrelated. Here is a new push: https://tbpl.mozilla.org/?tree=Try&rev=7d4fe2f6fa05. I've r+ed your patch, so feel free to reupload with the updated commit message with an r+
Assignee | ||
Comment 27•10 years ago
|
||
Only change is the commit message. Carrying r+ forward from previous patch. Thanks for all of your help.
Attachment #8480296 -
Attachment is obsolete: true
Attachment #8481001 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 28•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/812a0ba8e122
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/812a0ba8e122
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Flags: qe-verify-
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•