Closed Bug 1309271 Opened 8 years ago Closed 7 years ago

Accessibility issues when moving focus from chrome elements to content elements

Categories

(Core :: Disability Access APIs, defect)

52 Branch
x86
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox49 --- disabled
firefox50 --- disabled
firefox51 - disabled
firefox52 - disabled
firefox-esr52 --- disabled
firefox53 --- disabled
firefox54 --- disabled
firefox55 --- fixed

People

(Reporter: MarcoZ, Assigned: jimm)

References

(Blocks 2 open bugs, )

Details

(Keywords: multiprocess, regression, Whiteboard: aes+)

Attachments

(6 files, 4 obsolete files)

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.
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.
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.
(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.
Whiteboard: aes+
Jamie are you able to reproduce this one? (And any idea what is going on?)
Flags: needinfo?(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.
Flags: needinfo?(jamie)
OK, this bug has morphed for me into what Jamie reports in comment #5, too. I'll change the summary.
Summary: [E10S a11y] Firefox not firing an ValueChange event for select @size="1" comboboxes when changing the value → [E10S a11y] Firefox not firing an ValueChange event for select @size="1" comboboxes after opening and closing it
(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.
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.
Summary: [E10S a11y] Firefox not firing an ValueChange event for select @size="1" comboboxes after opening and closing it → [E10S a11y] Firefox not firing focus event for select @size="1" combobox after opening and closing it
I need to figure out if this could block.
Flags: needinfo?(dbolter)
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.
Flags: needinfo?(dbolter) → needinfo?(mzehe)
Probably not blocking, but something that should be dealt with very soon, since it's a pretty easy to reproduce user-facing bug.
Flags: needinfo?(mzehe)
Whiteboard: aes+
(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.
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.
Summary: [E10S a11y] Firefox not firing focus event for select @size="1" combobox after opening and closing it → [E10S a11y] Firefox not firing focus event for select @size="1" combobox, menu bar and context menu after opening and closing it
Whiteboard: aes+
Marking wontfix for 51, since AIUI e10s+a11y is targetting 52.
Whiteboard: aes+ → aes?
We're now targeting 53.

Marco is this a blocker?
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.
(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.
Flags: needinfo?(mzehe)
(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).
Flags: needinfo?(dbolter)
Is this something handyman could look at?
Flags: needinfo?(dbolter) → needinfo?(jmathies)
(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
Flags: needinfo?(jmathies) → needinfo?(dbolter)
Summary: [E10S a11y] Firefox not firing focus event for select @size="1" combobox, menu bar and context menu after opening and closing it → Select popups not represented properly in the accessible tree
Whiteboard: aes? → aes+
Yes let's chat at our sync up. In the meantime Trevor might have ideas.
Flags: needinfo?(dbolter) → needinfo?(tbsaunde+mozbugs)
(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.
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(jmathies)
TBD: split the context and menu bar bugs out.
Blocks: 1357547
(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.
Attached file dropdown test case
Flags: needinfo?(jmathies)
Depends on: 1357782
Depends on: 1359970
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.
Keywords: multiprocess
Summary: Select popups not represented properly in the accessible tree → Accessibility issues when moving focus from chrome elements to content elements
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?
Flags: needinfo?(mzehe)
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.
Flags: needinfo?(mzehe)
(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.
(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.
Attached file events (obsolete) —
We're definitely missing some focus related events when DOM focus shifts back to the combo box from the menu bar.
(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..
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>
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.
Flags: needinfo?(enndeakin)
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?
Flags: needinfo?(enndeakin)
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?
Assignee: nobody → jmathies
(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?
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
Attached patch patch (obsolete) — Splinter Review
Attachment #8864235 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8873478 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #8873488 - Attachment is obsolete: true
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/jmathies@mozilla.com-8aacb5ba4602d503ebebc36c5ac01632f1598171/try-win64/
Flags: needinfo?(mzehe)
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.
Flags: needinfo?(mzehe)
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.
Attachment #8873632 - Flags: review?(surkov.alexander)
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"
Attachment #8873632 - Flags: review?(surkov.alexander) → review+
Attached patch patchSplinter Review
Attachment #8873632 - Attachment is obsolete: true
Attachment #8874796 - Flags: review?(jmathies)
Attachment #8874796 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Attached patch review follow upSplinter Review
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.
Attachment #8874954 - Flags: review?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/43d1e5140752
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8874954 - Flags: review?(surkov.alexander) → review+
Pushed by jmathies@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/221a20df9bc5
Patch to cleanup nesting and updated comments per review nits. r=surkov
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: