1910348 - Tabbox code needs to correctly reference the parent tabs container

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Sarah Clements [:sclements]]

This came up during review of bug 1899336, where a pre-existing bug was discovered in that this.parentNode isn't referencing the tabbrowser-tabs container as it should so that means this.parentNode.selectedItem in on_mousedown won't ever be truthy because parentNode is pointing to either arrowscrollbox or the new vertical-pinned-tabs-container. Updating this seems like it'd break assumptions in 11y and chrome tests.

We should also update this.closest("tabs")._selectNewTab(this); to this.container._selectNewTab(this); (since tab.js has a container getter that references tabbrowser-tabs). This will break chrome tabbox tests so they'll need to be changed.

Comment 1

[Dão Gottwald [:dao]]

(In reply to Sarah Clements [:sclements] from comment #0)

This came up during review of bug 1899336, where a pre-existing bug was discovered in that this.parentNode isn't referencing the tabbrowser-tabs container as it should so that means this.parentNode.selectedItem in on_mousedown won't ever be truthy because parentNode is pointing to either arrowscrollbox or the new vertical-pinned-tabs-container.

Note that this was just an example for where things would fall apart. this.parentNode needs to be fixed throughout that class.

Updating this seems like it'd break assumptions in 11y and chrome tests.

It shouldn't, see below.

We should also update this.closest("tabs")._selectNewTab(this); to this.container._selectNewTab(this); (since tab.js has a container getter that references tabbrowser-tabs). This will break chrome tabbox tests so they'll need to be changed.

I'm not sure why this would break tests either; I don't think it should. Like I said tabbox.js / MozTab doesn't currently implement container and that will need to change.

Comment 2

[Tim Giles [:tgiles]]

Sarah and Dao, we're triaging this bug and we're a bit confused about the bug description.

• Is this issue impacting tabbox in horizontal and vertical tab orientations?
• Are users impacted by this this.parentNode issue? Trying to figure out scope and severity

Thanks!

Comment 3

[Dão Gottwald [:dao]]

(In reply to Tim Giles [:tgiles] from comment #2)

Sarah and Dao, we're triaging this bug and we're a bit confused about the bug description.

• Is this issue impacting tabbox in horizontal and vertical tab orientations?
• Are users impacted by this this.parentNode issue? Trying to figure out scope and severity

Hard to say. MozTabbrowserTab overrides MozTab's on_mousedown, so that use of parentNode there doesn't currently matter in practice. Perhaps more problematic is MozTab's use of container which it doesn't implement. That's likely breaking some features in non-tabbrowser tabboxes.

This is also relevant for the tab groups project where grouped tabs won't be direct children of <tabs> anymore. I'll pull this into our project.

Comment 4

[Dão Gottwald [:dao]]

Attachment: Bug 1910348 - Tabbox code needs to correctly reference the parent tabs container. r=tgiles

Comment 5

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #3)

(In reply to Tim Giles [:tgiles] from comment #2)

Sarah and Dao, we're triaging this bug and we're a bit confused about the bug description.

• Is this issue impacting tabbox in horizontal and vertical tab orientations?
• Are users impacted by this this.parentNode issue? Trying to figure out scope and severity

Hard to say. MozTabbrowserTab overrides MozTab's on_mousedown, so that use of parentNode there doesn't currently matter in practice.

Nevermind, MozTabbrowserTab's on_mousedown actually ends up calling super.on_mousedown, so this will be a problem for pinned tabs in vertical mode (as they're in their own container) and tab groups.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a57a549212e4
Tabbox code needs to correctly reference the parent tabs container. r=reusable-components-reviewers,mstriemer

Comment 7

[Iulian Moraru]

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