Stop using nsDeckFrame in XUL <tabpanels>
Categories
(Toolkit :: UI Widgets, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox107 | --- | fixed |
People
(Reporter: ntim, Assigned: emilio)
References
(Depends on 2 open bugs)
Details
Attachments
(2 files, 1 obsolete file)
From bug 1559192:
MozTabpanels (https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/toolkit/content/widgets/tabbox.js#151) could then either inherit from customElements.get("deck") or implement the same logic as deck.
|
||
Comment 1•4 years ago
|
||
Please flag someone from the a11y team (probably me) when working on this, as it could have a11y implications. I suspect the existing a11y tests should pick up any problems, but best to be safe. Thanks.
|
Reporter | |
Comment 2•4 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #0)
From bug 1559192:
MozTabpanels (https://searchfox.org/mozilla-central/rev/1133b6716d9a8131c09754f3f29288484896b8b6/toolkit/content/widgets/tabbox.js#151) could then either inherit from customElements.get("deck") or implement the same logic as deck.
Inheriting from <deck> seems pretty feasible at first sight. <tabpanels> has very slight differences in methods that are overridden, but these could be put directly in <deck>. The rest is mostly extra methods.
(In reply to James Teh [:Jamie] from comment #1)
Please flag someone from the a11y team (probably me) when working on this, as it could have a11y implications. I suspect the existing a11y tests should pick up any problems, but best to be safe. Thanks.
Sure, will do. I looked further and I found these 2 potentially relevant to this bug:
- https://searchfox.org/mozilla-central/search?q=symbol:_ZN22nsAccessibilityService17DeckPanelSwitchedEPN7mozilla9PresShellEP10nsIContentP8nsIFrameS6_&redirect=false
- https://searchfox.org/mozilla-central/rev/fd853f4aea89186efdb368e759a71b7a90c2b89c/accessible/generic/Accessible.cpp#330
It seems like it's mostly about getting/firing the right offscreen states, which I expect visibility: collapse or hidden, or even display: none should hopefully do.
The rest of the nsDeckFrame a11y code can be removed in bug 1689817.
|
|
||
Comment 3•4 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #2)
It seems like it's mostly about getting/firing the right offscreen states, which I expect visibility: collapse or hidden, or even display: none should hopefully do.
There are two worrying things here:
- If we use visibility: hidden/collapse or display: none, I think that will cause the tabpanel (and all of its descendants) to be removed from the a11y tree. I think that's going to include the tab document. If I'm right, that's very bad. While we normally want hidden content to be removed from the a11y tree, we don't want this for background tabs, since that would cause a11y clients to lose any state they maintained for those documents. This doesn't currently happen with nsDeckFrame.
- Even for CSS changes which cause content to be marked as off-screen for a11y (but don't remove it from the a11y tree), we don't fire an offscreen state change. Normally, this isn't needed, but it is definitely needed for tabpanels so a11y clients know which tab is active. So, we're going to need a specific notification or event for tabpanel changes, à la nsAccessibilityService::DeckPanelSwitched.
|
|
||
Comment 4•4 years ago
|
||
Note that these issues are covered by tests:
- accessible/tests/mochitest/events/test_statechange_tabpanels.xhtml
- accessible/tests/mochitest/states/test_visibility.xhtml
|
|
Reporter | |
Comment 5•4 years ago
|
||
Depends on D46489
|
||
Updated•2 years ago
|
|
|
||
Updated•2 years ago
|
|
Assignee | |
Comment 6•2 years ago
|
||
Gijs for front-end bits, layout for the new CSS properties and the
removal of nsDeckFrame / nsStackLayout, Jamie and Morgan for the a11y
changes.
As discussed in the bug, the main tricky part here is handling a11y
correctly. For <deck>, that's trivial (just use visibility: hidden to
hide the panels visually, while removing the unselected panels from the
a11y tree).
For <tabpanels> however we need to do something special. We do want to
hide stuff visually, but we want to preserve the contents in the a11y
tree.
For that, the easiest fix is introducing a new privileged CSS property
(-moz-subtree-hidden-only-visually), which takes care of not painting
the frame, but marks stuff offscreen in the accessibility tree. This is
not intended to be a property used widely.
Other than that, the changes are relatively straight-forward, though
some of the accessible/mac changes I could get a sanity-check on.
|
|
Assignee | |
Comment 7•2 years ago
|
||
This is callable from JS, and nothing guarantees that style has been
updated properly before. Without this, my previous patch causes failures
in browser/base/content/test/tabdialogs/browser_tabdialogbox_focus.js,
because the nsDeckFrame used to listen to attribute changes
synchronously and now it's style-based, so if you moveFocus right after
revealing it we'd end up with the wrong answer.
|
|
Assignee | |
Updated•2 years ago
|
|
||
Comment 9•2 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 10•2 years ago
|
||
There's this one test_tabbox timeout that I need to debug on macOS but otherwise this should be green-enough: https://treeherder.mozilla.org/jobs?repo=try&revision=66a019032a6b965319cd4216f651151b1eb5e504
|
|
Assignee | |
Updated•2 years ago
|
|
||
Comment 11•2 years ago
|
||
|
||
Comment 12•2 years ago
|
||
Backed out changeset 2f74f8f2ed19 (Bug 1689816) for causing reftest failures on skip-ink-multiline-position.html.
Backout link
Push with failures <--> R1
Failure Log
|
|
Assignee | |
Comment 13•2 years ago
|
||
That's bug 1770273 but in another test :/
|
|
||
Comment 14•2 years ago
|
||
|
||
Comment 15•2 years ago
|
||
| bugherder | ||
Description
•