1487065 - NVDA Screen Reader doesn't read the "Never" option from doorhangers on hover, due to XBL anonymous content in popup-notification

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Emil Ghitta, QA [:emilghitta]]

[Affected versions]:
Firefox 63.0a1 (BuildId:20180829100131).
Firefox 62.0 (BuildId:20180827144429).
Firefox 61.0.2 (BuildId:20180807170231).
Firefox 60.1.0esr (BuildId:20180621121604).

[Affected platforms]:
Windows 10 64bit.

[Preconditions]
Enable NVDA screen reader.

[Steps to reproduce]:
1. Launch Firefox.
2. Access the following webpage: https://twitter.com/.
3. Enter a random username and password.
4. Click the "Log in" button.
5. Hover over the "Never Save" option.

[Expected result]:
NVDA successfully reads the "Never Save" option.

[Actual result]:
NVDA reads the following: "Would you like Nightly to save this login for twitter.com ?".

[Regression range]:
This seems to be an old regression:

However, I didn't managed to track down the actual "culprit" since mozregression keeps skipping taskcluster builds.

Last good revision: 6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8 (2016-08-06)
First bad revision: d42aacfe34af25e2f5110e2ca3d24a210eabeb33 (2016-08-07)

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8&tochange=d42aacfe34af25e2f5110e2ca3d24a210eabeb33

[Notes]
This is reproducible on "Password" and "Notifications" doorhangers.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Bug 1273635 maybe?

Comment 2

[James Teh [:Jamie]]

Confirmed.

The menupopup gets no children in the accessibility tree. I did a bit of investigation and I believe this to be a regression caused by bug 1249930. A doorhanger notification uses XBL children to populate its pop-up menu:
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/notification.xml#544
https://searchfox.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#891
But we don't accept XBL children for XULPopupmenuAccessible:
https://searchfox.org/mozilla-central/source/accessible/xul/XULMenuAccessible.cpp#411

Alex, bug 1249930 doesn't explain *why* this change was made. Can you enlighten me?

Comment 3

[James Teh [:Jamie]]

Nope, I'm wrong about this. Allowing XBL children for XULMenupopupAccessible doesn't fix this. I think I misunderstood this anyway; these probably aren't considered XBL children, since even though they're inherited, they originate from outside the anonymous content.

Comment 4

[James Teh [:Jamie]]

1. If I stick an arbitrary <menuitem> directly inside the menupopup, it does show up in the tree. So, this is definitely somehow related to the fact that these children are inherited via XBL.
2. However, if I move the <children/> tag to the same level as the button, the inherited children do show up in the tree. So, this is somehow related to menupopup.

The only bug I see in the push log from comment 0 that gives me pause is bug 1274381. Unfortunately, I can't back that out to test; there are too many subsequent commits that depend on it that I don't know about.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #4)
> 1. If I stick an arbitrary <menuitem> directly inside the menupopup, it does
> show up in the tree. So, this is definitely somehow related to the fact that
> these children are inherited via XBL.
> 2. However, if I move the <children/> tag to the same level as the button,
> the inherited children do show up in the tree. So, this is somehow related
> to menupopup.
> 
> The only bug I see in the push log from comment 0 that gives me pause is bug
> 1274381. Unfortunately, I can't back that out to test; there are too many
> subsequent commits that depend on it that I don't know about.

It seems plausible that bug 1274381 interferes here, the fact a menuitem node belongs to the two parents - notifcation-popup and its anonymous menupoup must confuse the logic. Given the time the bug exists, it might worth to wait for dexblization project, which will make the things simpler, and should fix the bug.

Comment 6

[James Teh [:Jamie]]

(In reply to alexander :surkov (:asurkov) from comment #5)
> Given the time the bug exists, it might
> worth to wait for dexblization project, which will make the things simpler,
> and should fix the bug.

Unless dexblizing of notifications is happening very soon, I don't think it's reasonable to wait for this. Yes, the bug has existed for some time now. However, IMO, the only reason it hasn't drawn more complaints is that users aren't aware there are any options at all in that menu (or are just so confused by it that they give up). For quite some time, I just thought Firefox no longer provided a "Never Save" option. Now, I understand that it does, but it's just not accessible. That's totally unacceptable for a core browser function users will potentially encounter every day. We should do better. :)

Comment 7

[alexander :surkov (:asurkov)]

I definitely see your point. Just jumping into XBL anonymous tree requires some bravery and time for sure, and also it may be  sad to fix technologies that go away.

However you don't have to wait for the whole project. It could be the deXBLization of related code is on the way. Brian, does anything happen around doorhanger notification XBL bindings these days? Are there any plans?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #7)
> I definitely see your point. Just jumping into XBL anonymous tree requires
> some bravery and time for sure, and also it may be  sad to fix technologies
> that go away.
> 
> However you don't have to wait for the whole project. It could be the
> deXBLization of related code is on the way. Brian, does anything happen
> around doorhanger notification XBL bindings these days? Are there any plans?

Because of all the slotted children for popup-notification (5, which is actually the most out of any binding), I was planning to wait for Shadow DOM support in chrome (Bug 1465592) before attempting it.

It's probably possible to implement this with normal DOM and manually rearrange children during connectedCallback, but I expect it'd require rewriting CSS and JS that expect to be able to append stuff directly under the parent or into the various slotted nodes. I haven't looked into how hard that would be.

By the way, does this type of use-case work correctly from an accessibility standpoint with Shadow DOM?

Comment 9

[Brian Grinstead [:bgrins]]

AFAICT most popupnotification DOM access is running through a single JS API (PopupNotifications.jsm), so that at least makes it easier to change around how it works (either with normal DOM or shadow DOM).

Comment 10

[James Teh [:Jamie]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> By the way, does this type of use-case work correctly from an accessibility
> standpoint with Shadow DOM?

Given bug 1487312, probably not. However, we need to fix that bug for web content anyway, and if there are plans to de-XBL notifications in favour of shadow DOM, it'd be nicer if we could tackle both notifications and web content with the same fix.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to James Teh [:Jamie] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > By the way, does this type of use-case work correctly from an accessibility
> > standpoint with Shadow DOM?
> 
> Given bug 1487312, probably not. However, we need to fix that bug for web
> content anyway, and if there are plans to de-XBL notifications in favour of
> shadow DOM, it'd be nicer if we could tackle both notifications and web
> content with the same fix.

Yeah, I agree with that. But just to be clear on the timeline - we are probably looking at version 65 before we are able to ship Shadow DOM in chrome (that's when the prefs are slated to be removed), unless if something changes with Bug 1465592. I think there are some directionality things to sort out as well based on https://bugzilla.mozilla.org/show_bug.cgi?id=1465592#c8.

Comment 12

[Matthew N. [:MattN]]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> AFAICT most popupnotification DOM access is running through a single JS API
> (PopupNotifications.jsm), so that at least makes it easier to change around
> how it works (either with normal DOM or shadow DOM).

Many doorhangers have custom content (usually via <popupnotificationcontent>[1]) and then the elements inside that are manipulated in various ways so you'll need to be careful to handle those cases.

[1] https://dxr.mozilla.org/mozilla-central/search?q=popupnotificationcontent

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to James Teh [:Jamie] from comment #10)
> > (In reply to Brian Grinstead [:bgrins] from comment #8)
> > > By the way, does this type of use-case work correctly from an accessibility
> > > standpoint with Shadow DOM?
> > 
> > Given bug 1487312, probably not. However, we need to fix that bug for web
> > content anyway, and if there are plans to de-XBL notifications in favour of
> > shadow DOM, it'd be nicer if we could tackle both notifications and web
> > content with the same fix.
> 
> Yeah, I agree with that. But just to be clear on the timeline - we are
> probably looking at version 65 before we are able to ship Shadow DOM in
> chrome (that's when the prefs are slated to be removed), unless if something
> changes with Bug 1465592. I think there are some directionality things to
> sort out as well based on
> https://bugzilla.mozilla.org/show_bug.cgi?id=1465592#c8.

By the way, I'm asking in Bug 1465592 to see if we can start shipping SD in the chrome from a platform side. Regardless, sounds like we should wait for bug 1487312 before shipping any of them to avoid accessibility breakage on mutation, though.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> By the way, I'm asking in Bug 1465592 to see if we can start shipping SD in
> the chrome from a platform side.

Got the sign off to turn this on in chrome, so we won't need to wait for 65. 

> Regardless, sounds like we should wait for
> bug 1487312 before shipping any of them to avoid accessibility breakage on
> mutation, though.

And Emilio started looking into a fix for Bug 1487312 so we can handle mutations properly.

Comment 15

[James Teh [:Jamie]]

The fixes for bug 1487312 and bug 1487311 have now landed, so we should be in reasonably good shape with regard to this kind of thing being ported to shadow DOM.

Comment 16

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #15)
> The fixes for bug 1487312 and bug 1487311 have now landed, so we should be
> in reasonably good shape with regard to this kind of thing being ported to
> shadow DOM.

Brian, given the above, are you able to provide any thoughts as to when de-XBLisation of doorhangers might happen?

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to James Teh [:Jamie] from comment #16)
> (In reply to James Teh [:Jamie] from comment #15)
> > The fixes for bug 1487312 and bug 1487311 have now landed, so we should be
> > in reasonably good shape with regard to this kind of thing being ported to
> > shadow DOM.
> 
> Brian, given the above, are you able to provide any thoughts as to when
> de-XBLisation of doorhangers might happen?

Styling the anonymous content from document sheets isn't possible yet - the CSS Shadow Parts spec aims to add that (I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1505489 to enable this for chrome callers). Emilio, do you have a feeling on the amount of work needed for that on the platform side?

We could put to together at least a first-pass with Shadow DOM to make sure it works functionally (as we haven't yet shipped SD in a chrome widget). Or alternatively we could go forward with changing to light DOM and be careful with Matt's point in https://bugzilla.mozilla.org/show_bug.cgi?id=1487065#c12. Paolo's done some similar work with the tab notifications in Bug 1471403 / 1496827 so he might have some ideas here as well.

If this would save you from having to fix accessibility within XBL then we could put this near the top of the list (can't promise it would happen until the new year though given the all hands and related travel).

Comment 18

[James Teh [:Jamie]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> If this would save you from having to fix accessibility within XBL then we
> could put this near the top of the list

It would, and that would be appreciated if possible.

> (can't promise it would happen until
> the new year though given the all hands and related travel).

That's fine. If we were to fix the XBL a11y issue, we wouldn't get to it until next year either.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Styling the anonymous content from document sheets isn't possible yet - the
> CSS Shadow Parts spec aims to add that (I filed
> https://bugzilla.mozilla.org/show_bug.cgi?id=1505489 to enable this for
> chrome callers). Emilio, do you have a feeling on the amount of work needed
> for that on the platform side?

It's non-trivial, and I have a bunch of other stuff to do first before getting to that. 

I've added it to the layout roadmap to be discussed next week though, so may be able to prioritize it :-)

In any case, depending on the needs / rush, it may be worth styling the Shadow DOM via custom properties (which is the usual way to do that without CSS Shadow Parts). I don't know which kind of customization doorhangers need though.

Comment 20

[Brian Grinstead [:bgrins]]

Going to give this a try this without Shadow DOM, using manual slotting in the normal DOM

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: Bug 1487065 - Implement popup-notification as a Custom Element

Comment 22

[Brian Grinstead [:bgrins]]

So I've got a patch at https://phabricator.services.mozilla.com/D17699 that doesn't use XBL or Shadow DOM, but instead uses normal DOM and manually "slots" elements into the right place before the popup is shown. I've got a few tests still to fix, but it's green enough that I'd like to confirm it fixes the reported issue here. Could you please confirm that it does?

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #22)

So I've got a patch at https://phabricator.services.mozilla.com/D17699 that doesn't use XBL or Shadow DOM, but instead uses normal DOM and manually "slots" elements into the right place before the popup is shown. I've got a few tests still to fix, but it's green enough that I'd like to confirm it fixes the reported issue here. Could you please confirm that it does?

FYI: if Bug 1523429 has already stuck when you test, then the latest version on phab should apply. Otherwise, https://phabricator.services.mozilla.com/D17699?vs=on&id=55875&whitespace=ignore-most#toc would be best (it's the previous version that includes the same changes as Bug 1523429).

Comment 24

[James Teh [:Jamie]]

(In reply to Brian Grinstead [:bgrins] from comment #22)

So I've got a patch at https://phabricator.services.mozilla.com/D17699 that doesn't use XBL or Shadow DOM, but instead uses normal DOM and manually "slots" elements into the right place before the popup is shown. I've got a few tests still to fix, but it's green enough that I'd like to confirm it fixes the reported issue here. Could you please confirm that it does?

It does indeed fix the issue here. Tested with NVDA using both the keyboard and mouse tracking. Thank you very much!

Comment 25

[Tim Spurway [:tspurway]]

brian did you intend this to be landed and uplifted to 66?

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Tim Spurway [:tspurway] from comment #25)

brian did you intend this to be landed and uplifted to 66?

I don't think this will be upliftable.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04b57ffa53ce
Implement popup-notification as a Custom Element r=MattN

Comment 28

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/04b57ffa53ce

Comment 29

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 67.0a1 (BuildId:20190210213844) on Windows 10 64bit with NVDA enabled.