Closed Bug 1689816 Opened 2 years ago Closed 2 months ago

Stop using nsDeckFrame in XUL <tabpanels>

Categories

(Toolkit :: XUL Widgets, task)

task

Tracking

()

RESOLVED FIXED
107 Branch
Tracking Status
firefox107 --- fixed

People

(Reporter: ntim, Assigned: emilio)

References

(Depends on 2 open bugs, Regressed 1 open bug)

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.

Depends on: 1678547
Blocks: 1689817

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.

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

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.

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

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

Note that these issues are covered by tests:

  • accessible/tests/mochitest/events/test_statechange_tabpanels.xhtml
  • accessible/tests/mochitest/states/test_visibility.xhtml
Depends on: 1690357

Depends on D46489

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Attachment #9200774 - Attachment is obsolete: true

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.

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.

Keywords: leave-open

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

Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f74f8f2ed19
Implement <tabpanels> and <deck> without XUL layout. r=Gijs,Jamie,morgan,preferences-reviewers,mconley,TYLin

Backed out changeset 2f74f8f2ed19 (Bug 1689816) for causing reftest failures on skip-ink-multiline-position.html.
Backout link
Push with failures <--> R1
Failure Log

Flags: needinfo?(emilio)
Regressions: 1792479
Regressions: 1792481
Regressions: 1792489

That's bug 1770273 but in another test :/

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0f86ac41bc7
Implement <tabpanels> and <deck> without XUL layout. r=Gijs,Jamie,morgan,preferences-reviewers,mconley,TYLin
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch
Regressions: 1792719
Regressions: 1792664
You need to log in before you can comment on or make changes to this bug.