1309271 - Accessibility issues when moving focus from chrome elements to content elements

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

STR:
0. Make sure you have Firefox in E10S mode and NVDA running.
1. On this bug, focus any of the comboboxes like "Status" or "Component" or "Version".
2. Without expanding the combobox, just change the value using up or down arrows.

Expected: NVDA should announce the new value.
Actual: NVDA remains silent.

3. Turn E10S off, restart the browser, and repeat steps 1 and 2.

Result: The Value Change is now announced.

This is broken in E10S mode only.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix for 49. Marking this fix optional for 50, since we will not have e10s enabled by default for a11y users until at least 51.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

This is strange: On my local instance, it will call out the values of the combo boxes until I expand the combo box, then shrink it again. After that, it won't call them out anymore unless I remove focus and then restore focus.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to Aaron Klotz [:aklotz] from comment #2)
> This is strange: On my local instance, it will call out the values of the
> combo boxes until I expand the combo box, then shrink it again. After that,
> it won't call them out anymore unless I remove focus and then restore focus.

well, I believe expanded comboboxes are popups and so have their own HWND.  So I'm not totally suprised there is some weirdness here, but I'm not sure exactly what the problem is without debugging.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Jamie are you able to reproduce this one? (And any idea what is going on?)

Comment 5

[James Teh [:Jamie]]

I don't see what Marco sees (comment 0); i.e. if I don't expand the combo box, value changes are reported as expected. I do, however, see what Aaron sees (comment 2); i.e. after expanding and collapsing, I get no value change reports any more.

With e10s disabled:
We only receive events for a single combo box.
When expanding, I get just focus on list item.
When collapsing, I get focus on combo box, state change on combo box.

With e10s enabled:
We seem to be dealing with two combo boxes here: one with a parent id (e.g. -348) and one with a content id (e.g. -16777228).
When expanding, I get state change on parent combo box, focus on list item (the list item also has a parent id).
When collapsing, I get... nothing.
Furthermore, if I press alt twice to activate and then dismiss the menu bar, focus does not get fired on the combo box. So, something has lost track of the focus. The combo box does still have the focused state, though.
Alt+tabbing out and back in again fixes this.

There's another bug here too, but this probably isn't a11y specific. With e10s enabled, you can no longer press alt+upArrow to collapse a combo box. You must press escape or enter. This works as expected with e10s disabled.

Comment 6

[Marco Zehe (:MarcoZ)]

OK, this bug has morphed for me into what Jamie reports in comment #5, too. I'll change the summary.

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to James Teh [:Jamie] from comment #5)
> There's another bug here too, but this probably isn't a11y specific. With
> e10s enabled, you can no longer press alt+upArrow to collapse a combo box.
> You must press escape or enter. This works as expected with e10s disabled.

I've filed this as bug 1314251.

Comment 8

[James Teh [:Jamie]]

Actually, value change events do get fired after closing the combo box. The problem is that focus doesn't get fired on the combo box.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

I need to figure out if this could block.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Because there is a workaround I don't think we would block on only this bug. I'm going to hang a needinfo on MarcoZ so he can chime in when he returns.

Comment 11

[Marco Zehe (:MarcoZ)]

Probably not blocking, but something that should be dealt with very soon, since it's a pretty easy to reproduce user-facing bug.

Comment 12

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #5)
> Furthermore, if I press alt twice to activate and then dismiss the menu bar,
> focus does not get fired on the combo box. So, something has lost track of
> the focus. The combo box does still have the focused state, though.

Actually, I've discovered that this is a more general issue. If you open the menu bar (or context menu) and dismiss it, a focus event is not fired for the focused element. So, this issue is not specific to combo boxes. This only occurs with e10s enabled.

Comment 13

[Marco Zehe (:MarcoZ)]

Adjusting summary to encompass that this affects more than just combo boxes. Effectively for a screen reader user, focus is lost when these events don't fire. This is an immediately user perceivable regression after E10S is enabled.

Comment 14

[Julien Cristau [:jcristau]]

Marking wontfix for 51, since AIUI e10s+a11y is targetting 52.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

We're now targeting 53.

Marco is this a blocker?

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

If it is a blocker, I recommend that we add this to David Parks' queue since it probably involves the way that e10s remotes dropdowns.

Comment 17

[Marco Zehe (:MarcoZ)]

(In reply to David Bolter [:davidb] from comment #15)
> Marco is this a blocker?

My assessment from comment #13 still stands. This is a perceivable regression that will hit users in many places. It's not a blocker like a crash is, but in terms of good usability, it definitely is, yes.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to Marco Zehe (:MarcoZ) from comment #17)
> (In reply to David Bolter [:davidb] from comment #15)
> > Marco is this a blocker?
> 
> My assessment from comment #13 still stands. This is a perceivable
> regression that will hit users in many places. It's not a blocker like a
> crash is, but in terms of good usability, it definitely is, yes.

So is there a final call to make here? Is this a a11y+e10s ship blocker? Because it also seems to affect <select> dropdowns (bug 1351051 is presumed a dupe of this one).

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Is this something handyman could look at?

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1309271 failing test part 1

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1309271 failing test part 2

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1309271 failing test part 3

Comment 24

[Jim Mathies [:jimm]]

(In reply to Marco Zehe (:MarcoZ) (on PTO until April 24) from comment #13)
> Adjusting summary to encompass that this affects more than just combo boxes.
> Effectively for a screen reader user, focus is lost when these events don't
> fire. This is an immediately user perceivable regression after E10S is
> enabled.

I think we've expanded this bug scope out too far. Lets concentrate on the select popup issue which is a special case in e10s, and file follow ups for content context menus and the menu bar. I doubt the problems here are the same.

These select popups are generated by the chrome process in response to ipc messaging that scrapes and ships the option data over [1]. Once on the parent side, a xul popup gets created that takes focus away from content. 

I highly doubt these two elements get associated properly in the a11y tree. Thing is though I'm not sure how we solve something like this. David any thoughts? Maybe we can chat about this in triage tomorrow as well.

[1] http://searchfox.org/mozilla-central/source/toolkit/modules/SelectContentHelper.jsm

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Yes let's chat at our sync up. In the meantime Trevor might have ideas.

Comment 26

[Trevor Saunders (:tbsaunde)]

(In reply to Jim Mathies [:jimm] from comment #24)
> (In reply to Marco Zehe (:MarcoZ) (on PTO until April 24) from comment #13)
> > Adjusting summary to encompass that this affects more than just combo boxes.
> > Effectively for a screen reader user, focus is lost when these events don't
> > fire. This is an immediately user perceivable regression after E10S is
> > enabled.
> 
> I think we've expanded this bug scope out too far. Lets concentrate on the
> select popup issue which is a special case in e10s, and file follow ups for
> content context menus and the menu bar. I doubt the problems here are the
> same.
> 
> These select popups are generated by the chrome process in response to ipc
> messaging that scrapes and ships the option data over [1]. Once on the
> parent side, a xul popup gets created that takes focus away from content. 
> 
> I highly doubt these two elements get associated properly in the a11y tree.
> Thing is though I'm not sure how we solve something like this. David any
> thoughts? Maybe we can chat about this in triage tomorrow as well.

on linux I suspect that's correct though its not clear it matters?  On windows popups get their own HWND, so they kind of get their own accessible tree, but I think the normal relationships with the rest of the tree should hold.  So yeah we might need to fix this to make windows work.  On the other hand it might be that a11y is still creating accessibles for the select in the child process, and they get proxied to the parent? that could lead to weird things like two accessibles for the same select.  I never really looked at how it was managing to work, but I guess that's the first thing to find out.

Comment 27

[Jim Mathies [:jimm]]

TBD: split the context and menu bar bugs out.

Comment 28

[Jim Mathies [:jimm]]

(In reply to Marco Zehe (:MarcoZ) (on PTO until April 24) from comment #0)
> STR:
> 0. Make sure you have Firefox in E10S mode and NVDA running.
> 1. On this bug, focus any of the comboboxes like "Status" or "Component" or
> "Version".
> 2. Without expanding the combobox, just change the value using up or down
> arrows.
> 
> Expected: NVDA should announce the new value.
> Actual: NVDA remains silent.

I've reproduced the originally reported issue using Narrator. I'll do some more testing to confirm. 

> Actually, I've discovered that this is a more general issue. If you open the
> menu bar (or context menu) and dismiss it, a focus event is not fired for
> the focused element. So, this issue is not specific to combo boxes. This
> only occurs with e10s enabled.

I filed bug 1357547 to confirm menubar focus is working. Now that I read this over again, I realize that this issue probably nothing to do with menu bars, it has to do with a focus event that should fire on some other element when popup menus are destroyed. I'll do some testing on this to try and come up with a good test case. I'll probably split this out.

Comment 29

[Jim Mathies [:jimm]]

Attachment: dropdown test case

Comment 30

[Ryan VanderMeulen [:RyanVM]]

e10s+a11y bug IIUC, so setting flags accordingly

Comment 31

[Jim Mathies [:jimm]]

Ok, I did some additional testing. I thought we had a non-e10s bug here, but now I'm not sure. There is definitely an e10s bug though related to focus switching from chrome to content. This really has nothing to do with select elements, it's just that since the drop downs for selects are managed by chrome, they trigger this bug.

STR:

1) Open Windows Narrator
2) Open Firefox Nightly
3) Open the form select test case attached here
4) Tab so that a focus ring is displayed on the select element in content
5) alt-tab away from Firefox, then alt-tab back

result: Narrator should announce the select element
reason: focus is on content and the select when you switch to Firefox

6) navigate through the select using up and down arrows. 

result: proper narration
 
7) press the alt key moving focus to the menu bar
8) press the al ket again, moving focus back to the select element
9) navigate through the select using up and down arrows. 

result: no narration

As Marco pointed out, we're missing some sort of an a11y event that points Narrator back to content after the menu bar hides. You can repo this using any action that moves focus to chrome elements and then back to content.

This is only reproducible in e10s mode. non-e10s works as you would expect it.

Comment 32

[Jim Mathies [:jimm]]

Hey Marco, do you know what type of accessible event Firefox should fire when focus moves from one element to another? For example, when focus moves from the menu bar to a content element?

Comment 33

[Marco Zehe (:MarcoZ)]

definitely focus, and with the menu bar opening and closing, also a menupopup event of some sort that Windows likes to fire. IIRC, there are events for menubar active, menu active, menu inactive, menubar inactive etc.

Comment 34

[Jim Mathies [:jimm]]

(In reply to Marco Zehe (:MarcoZ) from comment #33)
> definitely focus, and with the menu bar opening and closing, also a
> menupopup event of some sort that Windows likes to fire. IIRC, there are
> events for menubar active, menu active, menu inactive, menubar inactive etc.

So you're thinking that we're not fireing proper DOM events? I was thinking this was a bug in accessible related events.

Comment 35

[Marco Zehe (:MarcoZ)]

(In reply to Jim Mathies [:jimm] from comment #34)
> (In reply to Marco Zehe (:MarcoZ) from comment #33)
> > definitely focus, and with the menu bar opening and closing, also a
> > menupopup event of some sort that Windows likes to fire. IIRC, there are
> > events for menubar active, menu active, menu inactive, menubar inactive etc.
> 
> So you're thinking that we're not fireing proper DOM events? I was thinking
> this was a bug in accessible related events.

That is the part that still needs investigation. From the looks and feel of it, we are not firing an accessible focus event once focus returns to content. Or that focus event, DOM or whatever, never reaches the chrome part, and thus the accessible event doesn't get fired. I don't know.

Comment 36

[Jim Mathies [:jimm]]

Attachment: events

We're definitely missing some focus related events when DOM focus shifts back to the combo box from the menu bar.

Comment 37

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #36)
> Created attachment 8864235 [details]
> events
> 
> We're definitely missing some focus related events when DOM focus shifts
> back to the combo box from the menu bar.

Actually, this doesn't appear to be focus related. I'm logging focus manager events in e10s and non-e10s and I don't see anything firing for bringing up the menu bar using the alt key. Yet, a11y does fire accessibility events here, and there are missing events for the e10s case.

still digging..

Comment 38

[Jim Mathies [:jimm]]

focus changing in content test case from smaug:

data:text/html,<style>input { border: red 1px solid; } :focus { border: green 1px solid;}</style><script>var c = 0; setInterval(function() { document.getElementsByTagName("input")[c % 2].focus(); ++c; }, 500);</script><input><input>

Comment 39

[Jim Mathies [:jimm]]

Hey Neil, this bug was originally filed as an accessibility bug, but it looks like it may be in the focus manager code. The test case in comment 38 is useful in reproducing. (run test case, enter alt to move focus from content to the menu bar, notice that focus keeps changing in content.) I'd like to try and fix this, curious if you have any feedback or suggestions on where to look.

Comment 40

[Neil Deakin]

I'm unclear what I should be seeing with that testcase. When the menubar is opened, the focus should remain in the <input> element. The accessibility concept of focus should be on the menubar or one of its menus.

The original steps in comment 0 don't relate to the e10s popup created in the parent process; the navigation when the popup is not open is done entirely in layout/forms/ code. But later comments suggest that the actual issue only happens when the popup is opened and then closed? Perhaps some accessibility event needs to fire when the popup is closed?

Comment 41

[alexander :surkov (:asurkov)]

popups are indeed special, as far as I know AT don't rely on focus events solely, and listen for popup_start/end events as well.

Jamie, could you give more background on it please?

Comment 42

[Jim Mathies [:jimm]]

(In reply to Neil Deakin (not available until Aug 9) from comment #40)
> I'm unclear what I should be seeing with that testcase. When the menubar is
> opened, the focus should remain in the <input> element.

When testing the test case and alt-menu bar in non-e10s, the browser behaves very differently. In non-e10s, the menu bar hides as soon as focus changes in content. If I'm quick, and I use the down arrow to bring up the menu bar menu, focus continues to change in content, but the menu bar menu keeps what appears to be dom focus which content is ignored.

With e10s, dom focus never moves to the menu bar so the menu bar never hides, and dom focus in content keeps flipping back and forth.

My take is this: if the menu bar is displayed, content should lose dom focus and all a11y events associated with content focus should end. dom focus shifts to chrome until the menu bar hides.

It feels like both e10s and non-e10s modes are currently broken.

Olly, any thoughts here?

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jim Mathies [:jimm] from comment #42)
> My take is this: if the menu bar is displayed, content should lose dom focus
> and all a11y events associated with content focus should end. dom focus
> shifts to chrome until the menu bar hides.
This sounds reasonable to me.

Comment 44

[Jim Mathies [:jimm]]

Attachment: patch

Comment 45

[Jim Mathies [:jimm]]

Attachment: patch

This fixes an issue where a11y focus does not get transferred back to content as a result of keyboard nav away from the menu bar.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=316d70b6c09fc60c55b365409be3b5b92e950c77

Comment 46

[Jim Mathies [:jimm]]

missing header fix - 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf50c6cc2612c4d5f52ea97a90bee8957d8d1ee8

Comment 47

[Jim Mathies [:jimm]]

Attachment: wip

latest wip that builds and passes tests.

I  still want to investigate a couple things. This makes for a suitable solution to the focus shifting issue though.

Comment 48

[Jim Mathies [:jimm]]

Hey Marco, would you mind testing these Windows builds [1] for any of the focus issues reported in this bug?

[1] https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-8aacb5ba4602d503ebebc36c5ac01632f1598171/try-win64/

Comment 49

[Jim Mathies [:jimm]]

full treeherder test run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8aacb5ba4602d503ebebc36c5ac01632f1598171&selectedJob=103829892

Comment 50

[Marco Zehe (:MarcoZ)]

Yes, this works for me. Focus transitions properly into and out of comboboxes when I alt+down arrow or alt+up arrow into and out of them, and it also properly transitions between the menu bar, or an opened menu, and back when I use the alt key to open or close. Same with Escape to close the menu. Same goes for the context menu (Shift+F10). Also, focus transition is quite smooth.

Comment 51

[Jim Mathies [:jimm]]

Comment on attachment 8873632 [details] [diff] [review]
wip

There may be an underlying unresolved issue where the accessibility library in the content process thinks it has DOM focus when it doesn't. Smaug's focus shifting test case posted here seems to highlight that issue. That seems like a more general problem compared to the notification problem we have here - which this patch fixes.

Comment 52

[alexander :surkov (:asurkov)]

Comment on attachment 8873632 [details] [diff] [review]
wip

Review of attachment 8873632 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to the best of my ipc knowledge

::: accessible/base/FocusManager.cpp
@@ +191,5 @@
>    }
>    mActiveItem = aItem;
>  
> +  // If mActiveItem is null, we might need to shift a11y focus to a remote
> +  // element.

a bit more detailed comment might be good here, something like this: "... shift a11y focus back to a tab document, for example, when combobox popup is closed, then the focus should be moved back to the combobox"

Comment 53

[Jim Mathies [:jimm]]

Attachment: patch

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43d1e5140752
Fire accessibility focus events from focused content elements when focus changes from chrome to content. r=surkov

Comment 55

[Jim Mathies [:jimm]]

Attachment: review follow up

Sorry forgot to hit qref and so I didn't get this comment change posted. While in there I cleaned a little bit of the code up too.

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43d1e5140752

Comment 57

[alexander :surkov (:asurkov)]

Comment on attachment 8874954 [details] [diff] [review]
review follow up

Review of attachment 8874954 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/base/FocusManager.cpp
@@ +191,5 @@
>    }
>    mActiveItem = aItem;
>  
> +  // If mActiveItem is null we may need to shift a11y focus from chrome
> +  // to an element in a remote browser.

still it'd be nice to mention about comboboxes and their popups living in different processes, it's sort of subtle technical detail, and not everyone might be aware of it.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/221a20df9bc5
Patch to cleanup nesting and updated comments per review nits. r=surkov

Comment 59

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/221a20df9bc5