Try to improve / simplify combobox layout.
Categories
(Core :: Layout: Form Controls, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox98 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 5 open bugs, Regressed 1 open bug)
Details
Attachments
(3 files)
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.
Assignee | ||
Comment 1•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
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'
.
Assignee | ||
Comment 4•3 years ago
|
||
(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•3 years ago
|
||
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•3 years ago
|
||
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>
.
Updated•3 years ago
|
Assignee | ||
Comment 7•3 years ago
|
||
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•3 years ago
|
||
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:
- 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.
- 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.
Assignee | ||
Comment 9•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
Thanks for the ping! I make a try run and didn't see any new issues.
Assignee | ||
Comment 13•3 years ago
|
||
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
Assignee | ||
Comment 14•3 years ago
|
||
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
Assignee | ||
Updated•3 years ago
|
Comment 15•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Comment 16•3 years ago
|
||
bugherder |
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 17•3 years ago
|
||
(ni?me to address some of the review comments asap)
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 18•3 years ago
|
||
ni? to add the test suggested here asap: https://phabricator.services.mozilla.com/D132719#inline-748151
Assignee | ||
Updated•3 years ago
|
Comment 19•3 years ago
|
||
Updated•3 years ago
|
Comment 21•3 years ago
|
||
Backed out 2 changesets (Bug 1744009) for causing reftest failures on select-3.html.
Backout link
Push with failures
Failure Log
Assignee | ||
Updated•3 years ago
|
Comment 23•3 years ago
|
||
Comment 24•3 years ago
|
||
bugherder |
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 25•3 years ago
|
||
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.
Assignee | ||
Comment 27•3 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #25)
Bug 1750753 should take care of that.
Updated•3 years ago
|
Description
•