Closed Bug 1285591 Opened 4 years ago Closed 4 years ago

[a11y] Fix accessibility in devtools autocompletes

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file)

Bug 1266456 migrated devtools autocompletes to use HTML. The list of suggestions now live in a different document than the autocomplete input. 

The relationship between the input and the selected item used to be set via an aria-activedescendant attribute set on the input, pointing to the id of the selected item.

However this is not working when the input and the selected elements live in different frames/documents. The goal of this bug is to restore accessibility features to the devtools autocompletes. 

(for instance this could imply cloning the suggestiosn in the same document as the input)
Depends on: 1266456
Whiteboard: [devtools-html] [triage]
Hi Julian, feel free to mark Marco and I for an a11y review for this bug. Thanks!
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Hi Yura,

I started implementing the solution described in comment#0. I think I managed to restore a similar experience. Could you give it a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f662f7fdc7c ?

The impacted autocomplete fields are:
- inspector HTML search
- inspector rule view autocompletes (when editing/adding a property name/value)
- inspector markup view autocompletes (when editing a style attribute for instance)
- console
- style editor

Thanks!
Flags: needinfo?(yzenevich)
(In reply to Julian Descottes [:jdescottes] from comment #2)
> Hi Yura,
> 
> I started implementing the solution described in comment#0. I think I
> managed to restore a similar experience. Could you give it a try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f662f7fdc7c ?
> 
> The impacted autocomplete fields are:
> - inspector HTML search
> - inspector rule view autocompletes (when editing/adding a property
> name/value)
> - inspector markup view autocompletes (when editing a style attribute for
> instance)
> - console
> - style editor
> 
> Thanks!

Couple of questions, I'm happy to try again once they are resolved:
* aria-activedecendant doesn't seem to be removed when move from a state where I have something entered in the searchbox (inspector search in this case) and the pop-up open to where I delete the value of the search and the pop-up is closed
* not sure if aria-label is really needed (likely not) because list item's name should be calculated from it's sub-tree. Unless there's something weird being announced from an inner scroll bar.
*
> +    } else if (this.anchor) {
> +      this._anchor.removeAttribute("aria-activedescendant");
>      }
Do you mean else if (this._anchor)  here ?
Flags: needinfo?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #3)
> *
> > +    } else if (this.anchor) {
> > +      this._anchor.removeAttribute("aria-activedescendant");
> >      }
> Do you mean else if (this._anchor)  here ?

To follow up I think it's because _anchor is not the element you add that attribute to.
Thanks for having a look Yura! 

Code wise, the first try push was really rushed and not really intended for code review :)

Please check another try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=99e13643b28bc241dcc1002a32d294cef278d6d9 . It should address most of your code related comments. 
(there might still be an issue of the aria activedescendant not being cleaned up if the active element happens to change between a show/hide of the autocomplete popup, but I would fix this before submitting to code review)

(In reply to Yura Zenevich [:yzen] from comment #3)
> 
> Couple of questions, I'm happy to try again once they are resolved:
> * aria-activedecendant doesn't seem to be removed when move from a state
> where I have something entered in the searchbox (inspector search in this
> case) and the pop-up open to where I delete the value of the search and the
> pop-up is closed

The attribute on hide in the other try push. 

> * not sure if aria-label is really needed (likely not) because list item's
> name should be calculated from it's sub-tree. Unless there's something weird
> being announced from an inner scroll bar.

Unless I set aria-label here, the screen reader prefaces each item with "bullet". Anything else I should use? 

> *
> > +    } else if (this.anchor) {
> > +      this._anchor.removeAttribute("aria-activedescendant");
> >      }
> Do you mean else if (this._anchor)  here ?

This was indeed buggy (although it did not seem to impact the output of the screenreader in my tests). It is fixed in the other try push.
Flags: needinfo?(yzenevich)
s/The attribute on hide/The attribute is removed on hide
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Thanks for having a look Yura! 
> 
> Code wise, the first try push was really rushed and not really intended for
> code review :)
> 
> Please check another try run
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=99e13643b28bc241dcc1002a32d294cef278d6d9 . It should
> address most of your code related comments. 
> (there might still be an issue of the aria activedescendant not being
> cleaned up if the active element happens to change between a show/hide of
> the autocomplete popup, but I would fix this before submitting to code
> review)
> 
> (In reply to Yura Zenevich [:yzen] from comment #3)
> 
> > * not sure if aria-label is really needed (likely not) because list item's
> > name should be calculated from it's sub-tree. Unless there's something weird
> > being announced from an inner scroll bar.
> 
> Unless I set aria-label here, the screen reader prefaces each item with
> "bullet". Anything else I should use? 

Haven't tried the new build just yet, but I think the "bullet" part can be fixed with CSS more elegantly. Since it's being pronounced, the list item bullet rendered in DOM (so screen reader is picking it up). However, I think we do not have the bullets in the autocomplete list (hidden somehow). So I would suggest to just use list-style: none; CSS style for the list of autocomplete items. AFAIK this should fix the issue.
Flags: needinfo?(yzenevich)
Flags: needinfo?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #7)
> (In reply to Julian Descottes [:jdescottes] from comment #5)
> 
> Haven't tried the new build just yet, but I think the "bullet" part can be
> fixed with CSS more elegantly. Since it's being pronounced, the list item
> bullet rendered in DOM (so screen reader is picking it up). However, I think
> we do not have the bullets in the autocomplete list (hidden somehow). So I
> would suggest to just use list-style: none; CSS style for the list of
> autocomplete items. AFAIK this should fix the issue.

Thanks, setting list-style-type: none on the list items does fix the issue. Was sure I tried that before, but I must have messed up. New try push in case : https://treeherder.mozilla.org/#/jobs?repo=try&revision=6993938d1d42
Yes it seems to work as expected. Haven't looked at the code this time but would love to do a final a11y-review for the patch. Thanks!
Flags: needinfo?(yzenevich)
Devtools autocomplete popups are hosted in a different document from the input
being autocompleted. To allow accessibility tools such as screen readers to still
make sense of this widget, a clone of the suggestion list is now inserted in the same
document as the input, and the aria-activedescendant attribute is updated on the input
accordingly.

Review commit: https://reviewboard.mozilla.org/r/66788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66788/
Attachment #8774272 - Flags: review?(bgrinstead)
Comment on attachment 8774272 [details]
Bug 1285591 - fix accessibility in devtools autocomplete using suggestion list clone;

Flagging Yura for a11y review, thanks for checking the implementation!
Attachment #8774272 - Flags: a11y-review?(yzenevich)
Priority: P1 → P3
Whiteboard: [devtools-html] → [reserve-html]
Comment on attachment 8774272 [details]
Bug 1285591 - fix accessibility in devtools autocomplete using suggestion list clone;

https://reviewboard.mozilla.org/r/66788/#review63720

Not sure if it's in the other file but I would also make sure that the following attributes are added:
* aria-haspopup on the input https://www.w3.org/TR/wai-aria/states_and_properties#aria-haspopup
* aria-controls would ideally point to the autocomplete list too https://www.w3.org/TR/wai-aria/states_and_properties#aria-controls

::: devtools/client/shared/autocomplete-popup.js:131
(Diff revision 1)
>    openPopup: function (anchor, xOffset = 0, yOffset = 0, index) {
>      this.__maxLabelLength = -1;
>      this._updateSize();
> +
> +    // Retrieve the anchor's document active element to add accessibility metadata.
> +    this._activeElement = anchor.ownerDocument.activeElement;

It loosk like active element for when the popup opens is the autocomplete input, which makes it fine to have aria-activedescendant set this way since it's on the right element (the above input). Is there any chance that the popup will be open when active element is not an input? In that case there might be a problem. I would be more explicit about aria-activedescendant being set on the input.
Attachment #8774272 - Flags: review?(yzenevich)
s/Is/If
(In reply to Yura Zenevich [:yzen] from comment #12)
> Comment on attachment 8774272 [details]
> Bug 1285591 - fix accessibility in devtools autocomplete using suggestion
> list clone;
> 
> https://reviewboard.mozilla.org/r/66788/#review63720
> 
> Not sure if it's in the other file but I would also make sure that the
> following attributes are added:
> * aria-haspopup on the input
> https://www.w3.org/TR/wai-aria/states_and_properties#aria-haspopup
> * aria-controls would ideally point to the autocomplete list too
> https://www.w3.org/TR/wai-aria/states_and_properties#aria-controls
> 

Will add them, thanks!
(FWIW, these were not set either before migrating to HTMLTooltip, but let's add them here as it seems pretty straightforward)

> ::: devtools/client/shared/autocomplete-popup.js:131
> (Diff revision 1)
> >    openPopup: function (anchor, xOffset = 0, yOffset = 0, index) {
> >      this.__maxLabelLength = -1;
> >      this._updateSize();
> > +
> > +    // Retrieve the anchor's document active element to add accessibility metadata.
> > +    this._activeElement = anchor.ownerDocument.activeElement;
> 
> It loosk like active element for when the popup opens is the autocomplete
> input, which makes it fine to have aria-activedescendant set this way since
> it's on the right element (the above input). Is there any chance that the
> popup will be open when active element is not an input? In that case there
> might be a problem. I would be more explicit about aria-activedescendant
> being set on the input.

The issue with the current implementation is that the autocomplete popup doesn't have a reference to the input. Sometimes the "anchor" element provided to the openPopup method will be the input, sometimes it won't. In my opinion this will require a new optional parameter in the openPopup to allow callers to specify a custom input when needed. 

Here I would like to focus on restoring the a11y features regressed by Bug 1266456 and move non trivial enhancements to a follow-up bug. Is that ok with you?

Yura: ni? for my question above. Also I notice you added an r? flag for yourself on the patch and did not update the a11y review flag. Was this intentional.
Flags: needinfo?(yzenevich)
(In reply to Julian Descottes [:jdescottes] (on PTO, back 4th of August) from comment #14)
> Also I notice you added an r? flag for
> yourself on the patch and did not update the a11y review flag. Was this
> intentional.

Yeah sorry, review board does not allow flags other than r related, all I wanted to do is to preserve the a11y review one.
Flags: needinfo?(yzenevich)
Attachment #8774272 - Flags: review?(yzenevich)
(In reply to Julian Descottes [:jdescottes] (on PTO, back 4th of August) from comment #14)
> (In reply to Yura Zenevich [:yzen] from comment #12)
> > Comment on attachment 8774272 [details]
> > Bug 1285591 - fix accessibility in devtools autocomplete using suggestion
> > list clone;
> > 
> > https://reviewboard.mozilla.org/r/66788/#review63720
> > 
> > Not sure if it's in the other file but I would also make sure that the
> > following attributes are added:
> > * aria-haspopup on the input
> > https://www.w3.org/TR/wai-aria/states_and_properties#aria-haspopup
> > * aria-controls would ideally point to the autocomplete list too
> > https://www.w3.org/TR/wai-aria/states_and_properties#aria-controls
> > 
> 
> Will add them, thanks!
> (FWIW, these were not set either before migrating to HTMLTooltip, but let's
> add them here as it seems pretty straightforward)

Thanks
Comment on attachment 8774272 [details]
Bug 1285591 - fix accessibility in devtools autocomplete using suggestion list clone;

(In reply to Julian Descottes [:jdescottes] (on PTO, back 4th of August) from comment #14)
> > ::: devtools/client/shared/autocomplete-popup.js:131
> > (Diff revision 1)
> > >    openPopup: function (anchor, xOffset = 0, yOffset = 0, index) {
> > >      this.__maxLabelLength = -1;
> > >      this._updateSize();
> > > +
> > > +    // Retrieve the anchor's document active element to add accessibility metadata.
> > > +    this._activeElement = anchor.ownerDocument.activeElement;
> > 
> > It loosk like active element for when the popup opens is the autocomplete
> > input, which makes it fine to have aria-activedescendant set this way since
> > it's on the right element (the above input). Is there any chance that the
> > popup will be open when active element is not an input? In that case there
> > might be a problem. I would be more explicit about aria-activedescendant
> > being set on the input.
> 
> The issue with the current implementation is that the autocomplete popup
> doesn't have a reference to the input. Sometimes the "anchor" element
> provided to the openPopup method will be the input, sometimes it won't. In
> my opinion this will require a new optional parameter in the openPopup to
> allow callers to specify a custom input when needed. 
> 
> Here I would like to focus on restoring the a11y features regressed by Bug
> 1266456 and move non trivial enhancements to a follow-up bug. Is that ok
> with you?

Regarding the active element: though in practice it might act as expected, e.g. active descendant of current user focus is the autocomplete, in reality I think it's a mistake. There are other ways screen reader users navigate other than focus and tabbing, for example via virtual cursor[s]. These virtual cursors are often separate from the keyboard focus.

In this case contextually it will be confusing to find active descendants on elements that autocompletes do not really belong to. I realize this complicates things but it should somehow be addressed to avoid digging ourselves deeper into the workaround hole.

I'll remove the a11y-review for now, if you don't mind, so we could find the suitable solution.
Attachment #8774272 - Flags: a11y-review?(yzenevich)
As discussed with :yzen on IRC, the issues discussed here were already present with the previous implementations (see https://hg.mozilla.org/mozilla-central/file/c4b1eb619c41/devtools/client/shared/autocomplete-popup.js#l309 for instance).

It would be really nice if we could ship at least the fixes for the regressions in Firefox 50, to avoid user impacts. This bug will be dedicated to fix the regressions, and the issues outlined in comment #17 will be addressed in Bug 1289464.
https://reviewboard.mozilla.org/r/66788/#review64040

::: devtools/client/inspector/inspector-search.js:442
(Diff revision 1)
>        }
>      }
>  
>      if (total > 0) {
>        let onPopupOpened = this.searchPopup.once("popup-opened");
> +      this.searchPopup.once("popup-closed", () => {

What if the popup was already hidden at this point?  Won't this event never fire?
https://reviewboard.mozilla.org/r/66788/#review64040

> What if the popup was already hidden at this point?  Won't this event never fire?

With the current implementation: 
- HTMLTooltip still fires the "hidden" event if you call hide() on an already hidden tooltip
- autocomplete-popup fires popup-closed when receiving "hidden" from the HTMLTooltip
=> I think here we should always get the expected event, even if the popup is already hidden.

I should retest this on windows to see if it is really mandatory to keep here, however my local builds crash on startup since yesterday.
I can submit another version that only listens to popup-closed if the popup is currently displayed (will probably switch to Task.async at the same time otherwise it will become unreadable).
(In reply to Julian Descottes [:jdescottes] (on PTO, back 4th of August) from comment #20)
> https://reviewboard.mozilla.org/r/66788/#review64040
> 
> > What if the popup was already hidden at this point?  Won't this event never fire?
> 
> With the current implementation: 
> - HTMLTooltip still fires the "hidden" event if you call hide() on an
> already hidden tooltip

Ah OK, this was the piece I was missing.  It was a little unexpected but I don't think we need to rewrite the function to handle that case.  I still don't really get why this piece is needed though, why do we need to call hidePopup() now?  Maybe instead clearItems should call _clearActiveDescendant, which will be automatically fired by the existing call to setItems.
Comment on attachment 8774272 [details]
Bug 1285591 - fix accessibility in devtools autocomplete using suggestion list clone;

https://reviewboard.mozilla.org/r/66788/#review64240

So except for Comment 21, this looks good to me.  I think it's landable as is if you want to do that work in a follow up, or if you want to push up some changes for that I will review them here
Attachment #8774272 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #21)
> (In reply to Julian Descottes [:jdescottes] (on PTO, back 4th of August)
> from comment #20)
> > https://reviewboard.mozilla.org/r/66788/#review64040
> > 
> > > What if the popup was already hidden at this point?  Won't this event never fire?
> > 
> > With the current implementation: 
> > - HTMLTooltip still fires the "hidden" event if you call hide() on an
> > already hidden tooltip
> 
> Ah OK, this was the piece I was missing.  It was a little unexpected but I
> don't think we need to rewrite the function to handle that case.  I still
> don't really get why this piece is needed though, why do we need to call
> hidePopup() now?  Maybe instead clearItems should call
> _clearActiveDescendant, which will be automatically fired by the existing
> call to setItems.

_clearActiveDescendant is already called by clearItems when doing "this.selectedIndex = -1;" which calls the associated set selectedIndex(), which in this case will also call clearActiveDescendant. IIRC I added this because on the inspector search, the screen reader would repeat "Firefox, iframe, toolbox, iframe [... whatever the ancestor chain was, can't really remember] ... list" before reading the actual item value. 

I think it would be worth spending some more time on this, testing more carefully, and if the issue is still present, try to understand the cause better.

(In reply to Brian Grinstead [:bgrins] from comment #22)
> So except for Comment 21, this looks good to me.  I think it's landable as
> is if you want to do that work in a follow up, or if you want to push up
> some changes for that I will review them here

I prefer to land the current patch and work on the remaining items in a follow up. I will have absolutely no means of testing on Windows + screen reader until I am back, which would be after the merge date for FF50 I believe.
OK that's fine, we can land this now
Comment on attachment 8774272 [details]
Bug 1285591 - fix accessibility in devtools autocomplete using suggestion list clone;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66788/diff/1-2/
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/b27aedfe7ed0
fix accessibility in devtools autocomplete using suggestion list clone;r=bgrins
Duplicate of this bug: 1290222
https://hg.mozilla.org/mozilla-central/rev/b27aedfe7ed0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Priority: P3 → P1
QA Contact: adalucin → petruta.rasa
With latest Aurora 50.0a2 2016-08-08 and NVDA 2016.2.1, I still can't hear what my selection from a list was when pressing Tab, Enter or using the mouse click.

Derek, could you please confirm that latest Aurora 50 or Nightly 51 have a different behavior than 48 Release? Thank you!
Flags: needinfo?(derek.riemer)
Hi,
Something still isn't right. Mainly, it seems that when typing with NVDA, the list suggestions appear, and then disappear after there is only one suggestion left.
STR:
1. type d into the list.
2. now type o. 
Notice how the list disappears.
This manifest ed itself differently last week, in tht the list worked, but the edit field never got focus again after I selected an option. Now it seems the list just flat out is gone after only one suggestion is available.
Flags: needinfo?(derek.riemer)
(In reply to derek.riemer from comment #31)
> Hi,
> Something still isn't right. Mainly, it seems that when typing with NVDA,
> the list suggestions appear, and then disappear after there is only one
> suggestion left.
> STR:
> 1. type d into the list.
> 2. now type o. 
> Notice how the list disappears.
> This manifest ed itself differently last week, in tht the list worked, but
> the edit field never got focus again after I selected an option. Now it
> seems the list just flat out is gone after only one suggestion is available.

Thanks for testing! The list disappears in some cases because only "document" is suggested when typing "do" - depending on the website.

(a) The issue that I still see here is that the selections made with "Tab" key, Up/Down arrows and Enter (or Tab) keys are not recognized by NVDA.
Only when a suggestion is selected using the mouse click, it is read by NVDA.
This is happening on Console and Inspector - HTML Search fields and it is a regression from bug 1266456.

The other autocomplete fields mentioned in comment 2 (Inspector-Rules and Style Editor) don't seem to have NVDA enabled at all. This is happening on Firefox 48.0 too.

Julian, I will fill a new bug for the issue mentioned above (a). Can you please let me know what else I could manually test in order to cover the verification of this bug? Thank you!
Flags: needinfo?(jdescottes)
(In reply to Petruta Rasa [QA] [:petruta] from comment #32)
> (In reply to derek.riemer from comment #31)
> > Hi,
> > Something still isn't right. Mainly, it seems that when typing with NVDA,
> > the list suggestions appear, and then disappear after there is only one
> > suggestion left.
> > STR:
> > 1. type d into the list.
> > 2. now type o. 
> > Notice how the list disappears.
> > This manifest ed itself differently last week, in tht the list worked, but
> > the edit field never got focus again after I selected an option. Now it
> > seems the list just flat out is gone after only one suggestion is available.
> 
> Thanks for testing! The list disappears in some cases because only
> "document" is suggested when typing "do" - depending on the website.
> 

It boils down to the same issue you found, "document" should be read as it gets selected. 

> (a) The issue that I still see here is that the selections made with "Tab"
> key, Up/Down arrows and Enter (or Tab) keys are not recognized by NVDA.
> Only when a suggestion is selected using the mouse click, it is read by NVDA.
> This is happening on Console and Inspector - HTML Search fields and it is a
> regression from bug 1266456.

Indeed it looks like the selected value used to be read upon selection and it is no longer the case. Thanks for catching this.

> 
> The other autocomplete fields mentioned in comment 2 (Inspector-Rules and
> Style Editor) don't seem to have NVDA enabled at all. This is happening on
> Firefox 48.0 too.

Not sure about this, NVDA is reading the suggestions when I test the inspector's rule-view autocompletes. Don't remember if I tried the style editor recently, but it was working as well. If this is also an issue with Firefox 48, it could be linked to your setup? 

While testing I sometimes found that NVDA got confused when switching from Console to Inspector. Maybe you ran into the same issue unknowingly? When closing/re-opening devtools and then going straight to the Inspector, NVDA as always been capable of reading the suggestions in the ruleview autocompletes.

> 
> Julian, I will fill a new bug for the issue mentioned above (a). Can you
> please let me know what else I could manually test in order to cover the
> verification of this bug? Thank you!

I can't think of anything else, but adding ni? to :yzen who might know more test scenarios that are easy to get wrong when doing a11y.
Flags: needinfo?(jdescottes) → needinfo?(yzenevich)
(In reply to Julian Descottes [:jdescottes] from comment #33)
> > The other autocomplete fields mentioned in comment 2 (Inspector-Rules and
> > Style Editor) don't seem to have NVDA enabled at all. This is happening on
> > Firefox 48.0 too.
> 
> Not sure about this, NVDA is reading the suggestions when I test the
> inspector's rule-view autocompletes. Don't remember if I tried the style
> editor recently, but it was working as well. If this is also an issue with
> Firefox 48, it could be linked to your setup? 
> 
> While testing I sometimes found that NVDA got confused when switching from
> Console to Inspector. Maybe you ran into the same issue unknowingly? When
> closing/re-opening devtools and then going straight to the Inspector, NVDA
> as always been capable of reading the suggestions in the ruleview
> autocompletes.

Indeed, it works if I open each section separately. I'll update bug 1294038 with the results.
Thank you!
I'm marking this as verified based on above comments and I'll follow-up up after :yzen's answer.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
removing ni? for now since we talked in IRC
Flags: needinfo?(yzenevich)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.