Sam Foster [:sfoster] (he/him) Reporter
	Description • 7 years ago

In bug 1331725 we want to turn on dom.select_popup_in_parent.enabled to be true in both e10s and non-e10s, so we have a single code path we can build on. Most of the relevant a11y mochitests fail with an exception similar to this:

21 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_select.html | Unique type focus event was handled. 
eventQueue_handleEvent@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:516:13

The tests compare expected vs. actual event sequences and the menulist/menupopup implementation run in non-e10s mode is different enough to fail these tests. I'm not sure if the test failures represents a real regression, or if the new sequence is actually ok and the tests can be updated?

	Marco Zehe (:MarcoZ)
	Comment 1 • 7 years ago

Sam, is there a try build I can throw at my screen reader to test this out? The tests are based on current screen reader behavior/expectations, so we'll need to actually test this to make sure the new sequence actually doesn't throw screen readers off guard before we can decide to update the tests.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 2 • 7 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #1)
> Sam, is there a try build I can throw at my screen reader to test this out?
> The tests are based on current screen reader behavior/expectations, so we'll
> need to actually test this to make sure the new sequence actually doesn't
> throw screen readers off guard before we can decide to update the tests.

At this point, this is literally just a pref change, setting "dom.select_popup_in_parent.enabled" to true. 

But I've also triggered some builds here with this pref: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3b86ce11726c869ae73d97fc961097f7f82296

Out of interest, which platform do you typically use to test this - I'm assuming Windows?

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 3 • 7 years ago

> At this point, this is literally just a pref change, setting
> "dom.select_popup_in_parent.enabled" to true. 
> 
> But I've also triggered some builds here with this pref:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8a3b86ce11726c869ae73d97fc961097f7f82296

To be clear, this is a change to *non-e10s* firefox. e10s already uses the menulist implementation and tests seem to pass fine in that environment.

	Marco Zehe (:MarcoZ)
	Comment 4 • 7 years ago

(In reply to Sam Foster [:sfoster] from comment #2)
> At this point, this is literally just a pref change, setting
> "dom.select_popup_in_parent.enabled" to true. 

OK, that's easy enough. ;)

I see two main differences when testing with NVDA under Windows (note, Mac isn't really accessibility-supported right now, and for Linux, the difference might be similar to what Windows shows):

1. When opening a select with Alt+DownArrow, I get double-speaking. First, the combobox is being repeated, this time with the expanded state, and then, focus moves to the list itself, with the current item selected. In the state with the pref disabled, speaking the combobox again with the expanded state does not happen. This can cause some irritation with users since there are two controls spoken after Alt+DownArrow is pressed, not one, as would be expected from before this change.

2. The more serious problem, though, is tht focus gets lost when pressing Alt+UpArrow or Enter to accept the change and close the popup. Focus does not get returned to the closed combobox. It gets lost somewhere, and users have to press tab or the like to make focus go somewhere useful again. This needs to be fixed since we should never let users get into a limbo state like this. With the pref disabled, after pressing Enter to accept the change from the popup, focus gets returned to the select in its now closed state again. This needs to happen with the pref enabled, too.

> Out of interest, which platform do you typically use to test this - I'm
> assuming Windows?

Correct.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 5 • 7 years ago

Ok thanks for checking that Marco. This test (accessible/tests/mochitest/actions/test_select.html) appears to be a part of mochitest-o, which is a suite that does not have an e10s equivalent, so my flipping dom.select_popup_in_parent.enabled to true represents the first time the tests have been run against the new menulist <select> popup. So my comment #3 is inaccurate - these test are not run under e10s and can be expected to fail. 

The core issue flagged by the test failures actually starts to sound like bug 1309271? If that's the case, I would expect the same set of symptoms as you describe in Comment #4 to reproduce using a <select> in a regular e10s window and out of the box prefs. Can you confirm?

	Marco Zehe (:MarcoZ)
	Comment 6 • 7 years ago

Yes, the issue is very similar to bug 1309271 indeed. Only that until now, it only affected E10S enabled builds. The issue about menu bar and context menus not firing focus events after closing them is similar, focus does not get returned to where it was before either the select popup, menu bar or context menu have been opened.

Why do we want this change for non E10S builds in the first place?

As for this test not having an equivalent, I'll have to defer to Yura if it is feasible to write a browser test for this scenario.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 7 • 7 years ago

(In reply to Marco Zehe (:MarcoZ) from comment #6)
> 
> Why do we want this change for non E10S builds in the first place?

We want to eliminate the 2 code paths to: get the same user experience in both e10s and non-e10s, to reduce testing and maintenance overhead with having 2 version of the same control, and also because we have some incoming changes to the <select> menu in bug 1332301 that would be much simpler implemented on top of a single select implementation.

	Sam Foster [:sfoster] (he/him) Reporter
	Comment 8 • 7 years ago

I'm going to dupe this to bug 1309271, I'm infering from Comment #4 that these are the expected symptoms of known <select> accessibility bugs as implemted with e10s. It doesnt make sense to this pref/test combination until those are resolved.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 9 • 7 years ago

Removign ni, added tests in bug 1332301

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 10 • 7 years ago

sorry i meant bug 1309271