Closed Bug 1555060 Opened 1 year ago Closed 1 year ago

Convert tabs binding to a CE

Categories

(Toolkit :: XUL Widgets, task)

60 Branch
task
Not set

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

Attachments

(3 files)

We only have a handful of <tabs> elements, these should be straightforward, with the exception of #tabbrowser-tabs which is big, gnarly, and intertwined with lots of other bits of the browser front end.

Most of the work here is in the tab strip (#tabbrowser-tabs).
The current code is full of assumptions that the dom structure
looks like a <tabs> element with individual <tab> elements as
its immediate and only children. Rather than try to preserve
that structure, this patch adds new properties on tabs and tab
elements (tabs.tabChildren and tab.container) to navigate the
logical <tabs>/<tab> relationships instead of using the dom
directly.

This wip patch is part of the way there, I still have some test failures to green up, but I'd like to get feedback about this approach before pushing the rest of the way through. Gijs and Dão, I suspect you two know the tab strip as well as anybody, are you on board with this approach or is there something else worth pursuing?

Since we don't have f? any more, lets go with ni?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dao+bmo)

(In reply to Andrew Swan [:aswan] from comment #1)

Most of the work here is in the tab strip (#tabbrowser-tabs).
The current code is full of assumptions that the dom structure
looks like a <tabs> element with individual <tab> elements as
its immediate and only children. Rather than try to preserve
that structure, this patch adds new properties on tabs and tab
elements (tabs.tabChildren and tab.container) to navigate the
logical <tabs>/<tab> relationships instead of using the dom
directly.

This makes sense to me.

Flags: needinfo?(gijskruitbosch+bugs)

Commented in phabricator. I have concerns about tabs.tabChildren. Otherwise I think this is fine, as these things are implementation details abstracted away behind gBrowser.tabs and gBrowser.tabContainer -- as long as we keep these two, the changes should be manageable.

Flags: needinfo?(dao+bmo)

These tests are full of too many problems to catalog here.
This patch just rewrites them to make them less insane.

A bunch of existing code assumes that <tab> elements are the immediate
and only children of a <tabs> element, and uses dom apis to traverse
relationships between these elements. To simplify conversion of <tabs>
to a custom element (and hopefully improve readability a bit at the same
time!), introduce new apis:

On <tab>
this.parentNode -> this.container
this.nextElementSibling -> this.container.findNextTab(...)
this.previousElementSibiling -> this.container.findNextTab(...)

On <tabs>
this.firstElementChild -> this.firstTab
this.lastElementChild -> this.lastTab
this.children.length -> this.tabCount
this.children[i] -> this.getItemAtIndex(i)
this.children -> this.allTabs

One subtle difference is that the allTabs property on a <tabs> element
which used to return a live HTMLCollection now returns an Array. Accessing
allTabs is also more expensive than the old .children property, so various
callers have been updated to use .tabCount, .getItemAtIndex, etc as
appropriate.

Attachment #9068088 - Attachment description: Bug 1555060 Convert <tabs> wip → Bug 1555060 Convert <tabs> to a custom element

Here's a linux-only try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=432e78cd2848622b562f0f8ac846c4a49a45d91c

I'll do a broader run and a talos run shortly...

There's a decent amount of orange here but it appears to all be known intermittents (or tier 2 jobs that I didn't look closely at)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee6697de9e25955d523f6dfde89c18d6d01679d

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c71c45fe3e63
Rewrite webextension tabs.move tests r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e21e465f38
Stop using dom structure for <tabs>/<tab> relationships
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5c6deeda8a9
Convert <tabs> to a custom element

At this point it might make sense to postpone until 69 merges to beta on July 8th. When you reland this, please make sure to put the reviewers in the commit messages.

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58831b7309de
Rewrite webextension tabs.move tests r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9198b156f7
Stop using dom structure for <tabs>/<tab> relationships
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f15ea3b3223
Convert <tabs> to a custom element

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

At this point it might make sense to postpone until 69 merges to beta on July 8th. When you reland this, please make sure to put the reviewers in the commit messages.

Sorry, in my haste to wrap this up last week I didn't see this comment until after it was re-landed (and so it also re-landed without the r= strings, ack)
I'll be watching closely for regressions, but I didn't want anybody to get the idea that I was deliberately ignoring the comment...

Flags: needinfo?(aswan)
Regressions: 1563331
Component: XBL → XUL Widgets
Product: Core → Toolkit
Regressions: 1579709
Regressions: 1584498
Blocks: 1422747
Regressions: 1597932
Blocks: 1597932
No longer regressions: 1597932
You need to log in before you can comment on or make changes to this bug.