1744009 - Try to improve / simplify combobox layout.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

Bug 1741888 has some reasons for that. Let's see if we can use the e10s code-path everywhere and not have a totally different code-path for non-e10s / parent process.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1744009 - Simplify combobox <select> code. r=#layout-reviewers!,mconley!

Checkpoint, there's a few things that do not work, but this is quite a
massive simplification that we can do by removing the non-e10s select
code-path.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So the patch in its current state should be green, modulo a11y test failures. The a11y test failures come in two flavors, one concerns me a bit and one does not.

The first kind of failure, which I don't think is super-concerning, is just because focus changes from the <select> to the #ContentSelectDropDown in the a11y tests, because now we use the e10s <select> code-path everywhere. Presumably that is fine (is closer to what all users see already) and I'll just try to adapt those tests to pass.

The failure mode that concerns me a little bit more is <option> elements in comboboxes don't show up in the a11y tree anymore (until you open the combobox) because we no longer create frames for them. I don't know if that's acceptable. On one hand, that seems more similar to what you get as a sighted user with a11y disabled, so it might be sensible. On the other hand it is definitely a behavior change. Jamie / Morgan / Eitan, do you have opinions on this one? An example of a test that fails is accessible/tests/mochitest/attributes/test_obj_group.html | Can't get accessible for 'opt1-nosize'.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

The first kind of failure, which I don't think is super-concerning, is just because focus changes from the <select> to the #ContentSelectDropDown in the a11y tests, because now we use the e10s <select> code-path everywhere. Presumably that is fine (is closer to what all users see already) and I'll just try to adapt those tests to pass.

BTW it's a massive pain that an unexpected focus event not only messes up that a11y test, but all subsequent a11y tests :(

Comment 5

[Eitan Isaacson [:eeejay]]

Yeah, that is a pretty major change - not having the options as children in-tree. I'll look into other apps and see what they do. Assuming Jamie will beat me to it in Australia time on Monday though.

Comment 6

[James Teh [:Jamie]]

I'm mostly concerned about Windows, which is where we have to deal with legacy pain. That said, Windows native combo boxes don't expose options when they're collapsed. That doesn't mean a11y clients (I'm particularly wondering about the JAWS screen reader) don't rely on this for the web. However, I checked Chrome and it doesn't seem to expose options in collapsed combo boxes either, so presumably, JAWS doesn't rely on it now (if it ever did).

That said, does this mean that the options are no longer exposed beneath the <select> element in the a11y tree even when it's expanded, but are instead only exposed inside the <menulist> created outside of the document? That seems like it could possibly be a problem for JAWS, especially since Chrome does expose these options as being inside the <select>.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yeah that's right, when it's expanded they would show up in the menulist, but not in the content process. To be clear we could make it happen, and we could potentially also keep our current behavior, it's just a bunch of extra complexity which is probably error prone. Do you know how could I test jaws? I'm guessing it probably isn't problematic, since focus right now moves to the parent process' menulist, but I'd like to check before going through all the tests and fixing them up.

Comment 8

[James Teh [:Jamie]]

There's a bunch of weird code associated with this in the a11y module too, so I agree it'd be nice if we could remove it.

You can download a demo of JAWS. The simple test is tabbing to the combo box and ensuring that up and down arrows report the focused item as expected. What I'm more concerned about is:

1. When the combo box is dismissed after choosing an option, does JAWS report the correct value for the combo box; e.g. if you use the up and down arrows to move through the document (JAWS virtual cursor)? My concern is that even though the focused option might be reported correctly, JAWS's representation of the document might not get updated correctly. It's unlikely to be a problem, but I have no idea how they've implemented tracking of combo box values.
2. If you move to a combo box using the arrow keys (JAWS virtual cursor) and press alt+downArrow, does it expand correctly?

I'd be happy to test this at some point myself, but I'm probably not going to have a chance to do this for the next two days and then I'm on PTO for the rest of the week.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ok, I'll try to test this myself, thanks! I don't plan to land this before the merge so worst case if I don't manage to do it I'll send for review the a11y test fixes noting that, and you can review whenever you have the time. Thank you again!

Comment 10

[Morgan Reschenberg [:morgan]]

un-NI'ing me, since it looks like you got the info you need from eitan and jamie :) feel free to re-NI if that's incorrect!

Comment 11

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

Hey Magnus,

I'm not 100% certain if this would effect any <select> dropdowns that Thunderbird is using, but cc'ing you just in case.

Comment 12

[Magnus Melin [:mkmelin]]

Thanks for the ping! I make a try run and didn't see any new issues.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1744009 - Deal with destruction a bit better in the animation inspector. r=nchevobbe

My patch above causes select change events in the parent process to be a
bit more async so we hit an exception here like:

TEST-UNEXPECTED-FAIL | devtools/client/inspector/animation/test/browser_animation_pause-resume-button_end-time.js | A promise chain failed to handle a rejection: can't access property "store", this.inspector is null - stack: get state@resource://devtools/client/inspector/animation/animation.js:218:5
setAnimationStateChangedListenerEnabled@resource://devtools/client/inspector/animation/animation.js:562:31
setAnimationsPlaybackRate@resource://devtools/client/inspector/animation/animation.js:516:12
Async*onChange@resource://devtools/client/inspector/animation/components/PlaybackRateSelector.js:70:30
invokeGuardedCallbackImpl@resource://devtools/client/shared/vendor/react-dom.js:74:10
invokeGuardedCallback@resource://devtools/client/shared/vendor/react-dom.js:111:29
invokeGuardedCallbackAndCatchFirstError@resource://devtools/client/shared/vendor/react-dom.js:125:25
executeDispatch@resource://devtools/client/shared/vendor/react-dom.js:346:42
executeDispatchesInOrder@resource://devtools/client/shared/vendor/react-dom.js:365:20
executeDispatchesAndRelease@resource://devtools/client/shared/vendor/react-dom.js:462:29
executeDispatchesAndReleaseTopLevel@resource://devtools/client/shared/vendor/react-dom.js:470:10
forEachAccumulated@resource://devtools/client/shared/vendor/react-dom.js:444:8
runEventsInBatch@resource://devtools/client/shared/vendor/react-dom.js:598:21
runExtractedEventsInBatch@resource://devtools/client/shared/vendor/react-dom.js:606:19
handleTopLevel@resource://devtools/client/shared/vendor/react-dom.js:4272:30
batchedUpdates$1@resource://devtools/client/shared/vendor/react-dom.js:15752:12
batchedUpdates@resource://devtools/client/shared/vendor/react-dom.js:1882:12
dispatchEvent@resource://devtools/client/shared/vendor/react-dom.js:4351:19
receiveMessage@resource://gre/actors/SelectChild.jsm:272:21
receiveMessage@resource://gre/actors/SelectChild.jsm:475:21
JSActor query*handleEvent@resource://gre/actors/SelectParent.jsm:340:21
EventListener.handleEvent*_registerListeners@resource://gre/actors/SelectParent.jsm:389:11
open@resource://gre/actors/SelectParent.jsm:224:10
receiveMessage@resource://gre/actors/SelectParent.jsm:792:28
JSActor query*showDropDown@resource://gre/actors/SelectChild.jsm:128:16
SelectContentHelper@resource://gre/actors/SelectChild.jsm:64:8
handleEvent@resource://gre/actors/SelectChild.jsm:451:29
synthesizeMouseAtPoint@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:644:13
synthesizeMouse@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:562:10
synthesizeMouseAtCenter@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:735:10
clickOnPlaybackRateSelector@chrome://mochitests/content/browser/devtools/client/inspector/animation/test/head.js:303:14
@chrome://mochitests/content/browser/devtools/client/inspector/animation/test/browser_animation_pause-resume-button_end-time.js:43:9
Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1091:34
Tester_execTest@chrome://mochikit/content/browser-test.js:1131:11
nextTest/<@chrome://mochikit/content/browser-test.js:939:14
SimpleTest.waitForFocus/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1041:13

Other code around is already dealing with an already-destroyed inspector
so do the same here.

Depends on D132719

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1744009 - Accessibility fixes for new combobox layout code. r=Jamie,eeejay,morgan

In terms of the C++ code, this patch does basically one thing, which is
allowing creating option / optgroup accessibles without a frame for
comboboxes, and tracking mutations like layout does.

It seems this should be straight-forward, but handling mutations got a
bit complicated. We don't want to forcibly re-create accessibles, so we
want to re-use the PruneOrInsertSubtree logic that ContentInserted uses.

But determining whether we need to create the accessible requires
having flushed styles, so I added a ScheduleAccessibilitySubtreeUpdate
API to trigger that from WillRefresh once style and layout are
up-to-date.

The rest of the test updates should be sort of straight-forward. They
reflect two changes:

• <option> accessibles are leaves now (so they don't have text
  children). Note that we still have the right native name and so on,
  using the same logic we use to render the label.

• In 1proc tests, the focus no longer goes to the <option>, and uses
  the same code-path that e10s does (moving focus to a <menulist> in
  the parent process). Since that wasn't easy to test for (afaict) and
  we have browser tests to cover that
  (browser_treeupdate_select_dropdown.js, etc), I've decided to just
  remove the tests that relied on the previous code-path, as they were
  testing for a codepath that users weren't hitting anyways.

I've tested this with JAWS and Orca and behavior seems unchanged to my
knowledge.

Depends on D133097

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d331689839ef
Deal with destruction a bit better in the animation inspector. r=nchevobbe

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d331689839ef

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(ni?me to address some of the review comments asap)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

ni? to add the test suggested here asap: https://phabricator.services.mozilla.com/D132719#inline-748151

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/619389788775
Simplify combobox <select> code. r=mconley,dholbert
https://hg.mozilla.org/integration/autoland/rev/3e44e31d3d12
Accessibility fixes for new combobox layout code. r=eeejay

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32405 for changes under testing/web-platform/tests

Comment 21

[Marian-Vasile Laza]

Backed out 2 changesets (Bug 1744009) for causing reftest failures on select-3.html.
Backout link
Push with failures
Failure Log

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78130d73ca75
Simplify combobox <select> code. r=mconley,dholbert
https://hg.mozilla.org/integration/autoland/rev/d917b43d4458
Accessibility fixes for new combobox layout code. r=eeejay

Comment 24

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/78130d73ca75
https://hg.mozilla.org/mozilla-central/rev/d917b43d4458

Comment 25

[Geoff Lankow (:darktrojan)]

The pop-up for these <select>s (in a parent-process browser) is offset by the size of the window decorations, as seen here:
https://firefoxci.taskcluster-artifacts.net/eqDqlAmmTZqRctdZiMOnrA/0/public/test_info/mozilla-test-fail-screenshot_vbp6z9pf.png

It's causing a test to fail, presumably because a mouse event isn't hitting the right target any more.

Our window decorations have changed, so this issue wouldn't have shown up when Magnus tested it.

Comment 26

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 27

[Emilio Cobos Álvarez (:emilio)]

(In reply to Geoff Lankow (:darktrojan) from comment #25)
Bug 1750753 should take care of that.