Closed Bug 1744009 Opened 3 years ago Closed 3 years ago

Try to improve / simplify combobox layout.

Categories

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

defect

Tracking

()

RESOLVED FIXED
98 Branch
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.

Blocks: 1663366

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.

Depends on: 1596852
Depends on: 1744152
Attachment #9253485 - Attachment description: WIP: Bug 1744009 - WIP - Simplify <select> popup code. → Bug 1744009 - Simplify combobox <select> code. r=#layout-reviewers!,mconley!

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'.

Flags: needinfo?(mreschenberg)
Flags: needinfo?(jteh)
Flags: needinfo?(eitan)

(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 :(

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.

Flags: needinfo?(eitan)

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>.

Flags: needinfo?(jteh)
Flags: needinfo?(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.

Flags: needinfo?(emilio) → needinfo?(jteh)

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.

Flags: needinfo?(jteh)

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!

Flags: needinfo?(emilio)

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!

Flags: needinfo?(mreschenberg)

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.

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

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

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

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d331689839ef Deal with destruction a bit better in the animation inspector. r=nchevobbe
Keywords: leave-open
Blocks: 1745688
Severity: -- → S3
Priority: -- → P2

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

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Blocks: 1750395
Blocks: 1750431
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32405 for changes under testing/web-platform/tests
No longer blocks: 1663366
Depends on: 1663366

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

Upstream PR was closed without merging
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: 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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

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.

Flags: needinfo?(emilio)
Regressions: 1750682
Upstream PR merged by moz-wptsync-bot
Regressions: 1750753

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

Flags: needinfo?(emilio)
Target Milestone: --- → 98 Branch
Regressions: 1762873
Regressions: 1765577
Blocks: 1553930
Regressions: 1774378
Regressions: 1777285
Regressions: 1783415
Blocks: 1667494
Regressions: 1811002
Blocks: 1678534
Regressions: 1851397
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: