1554630 - [de-xbl] convert the tabmail-tabs binding

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, task)

Description

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/5a670c59f9004ef9be4874cfbfe57ec2ef3b260f/mail/base/content/tabmail.xml#2015

I think there is no m-c bug for tabbrowser-tabs conversion yet - but will depend on bug 1514926.

Comment 1

[Magnus Melin [:mkmelin]]

tabbox.xml#tabs is going away in D32855 - bug 1555060

We may have to fork that binding temporarily.

Comment 2

[Magnus Melin [:mkmelin]]

Bug 1555060 almost landed.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1554630_tabmail-tabs.patch

This is start of the conversion. Didn't try it out yet (need to pull an rebuild), but at least <children> needs fixing. But there might be a lot...
(No time to look further atm.)

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1554630_tabmail-tabs.patch (WIP)

Removing the binding and css to hook it up too.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1554630_tabmail-tabs.patch

Not finished, but at least the main UI shows up now, and as long as you stay in the one tab, thing seem to work. Tabbing doesn't (really) work. At least not getting back to what you tabbed from. Etc...

Instead of adding structure in connectedCallback, have it in the markup (like browser.xhtml).

Sent to try to see how bad the test failures are: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ed8cba9af88bcc9c120fa799df981045bf867716

I'm not likely to be able to work on this until monday, so someone feel free to take over.

If the try looks half decent, it could potentially be landed to fix the bustage. I wouldn't expect very substantial changes to be needed, but of course all the details add up.

Comment 6

[Alessandro Castellani [:aleca]]

Thanks for taking care of this so quickly.
I'll continue from here starting from tomorrow.
Who do you want me to ping for reviews if you're not available?

Comment 7

[Jorg K (CEST = GMT+2)]

Quickly? Bug 1555060 has been in the making for a month now, with reviews coming in since June 12th. Proactive would have been to have the patches ready here for immediate landing. Today we shipped a totally broken Daily, and as a sheriff I now need to conduct extra try runs with bug 1555060 backed out to see whether anything else is breaking while TB is completely hosed due to this :-(

EDIT: Besides, no one else can work unless they jump through hoops since Mozmill is completely orange.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: bug1554630_tabmail-tabs (aleca WIP).patch

I'm sorry Jorgk, this situation is mostly my fault.
I got sidetracked by many other tasks and this one slipped away. I should have set a priority and reminders as soon as it was assigned to me. It won't happen again.

This is a WIP patch where the tabs are created in the right place and switching between tabs works.
Still, lots of errors and issues to find and fix.

I'll keep working on it.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: bug1554630_tabmail-tabs (aleca WIP).patch

More progress, getting real close.
The tabs are properly rendered and opened, switching between tabs fully works, and they can be closed with right click -> close.
I need to fix the missing close button and the drag&drop. I'll do a try push afterwards to fix all the mozmill tests.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: bug1554630_tabmail-tabs (aleca WIP).patch

Here's my last WIP for today.
Everything should work as expected, and the majority of the tests should be fixed.

There's still a lot of clean up and optimization that needs to be done, and obviously a full review.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: bug1554630_tabmail-tabs (aleca WIP).patch

wrong file...

Comment 12

[Alessandro Castellani [:aleca]]

Here's a try-push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a88707c2fff013dabd2491f1665615fd933a44d7
Lots of failures still, but a bit of green is coming back.
I will continue tomorrow.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: bug1554630_tabmail-tabs (aleca WIP).patch

More progress on fixing those tests.

Comment 14

[Richard Marti (:Paenglab)]

When the tabs overflow, the alltabs button does not appear. When I unhide it through Inspector I see only three menuitems with undefined as text.

Comment 15

[Jorg K (CEST = GMT+2)]

Latest try looks good, only failing Mozmill testTodayPane.js now with:
EXCEPTION: Expression "{"class":"agenda-container-box"}" returned null. Anonymous == true
on line 112. No idea how that relates to the tab changes. I'll switch it off for now. Maybe Geoff can fix it.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/99a134f08efe
convert the tabmail-tabs binding to custom element. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/68fe7b1e877f
disable testTodayPane.js. rs=bustage-fix

Comment 17

[Magnus Melin [:mkmelin]]

Thanks for fixing it up Alex! The path in testTodayPane.js just need to be adjusted since tab is now wrapped in an arrowscrollbox. I'll upload a patch soon.

Comment 18

[Magnus Melin [:mkmelin]]

Attachment: bug1554630_tabmail-tab_todaypanetestfix.patch

If you want something to land later...

Comment 19

[Magnus Melin [:mkmelin]]

For other code changes (in general), should also add "is" in connectedCallback

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9075067 [details] [diff] [review]
bug1554630_tabmail-tab_todaypanetestfix.patch

Thanks for patch. The I'm really the wrong guy to review this (given that I tried to fix this myself last night and failed), but it works, so let's take it.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0a4f2477de1d
ajust testTodayPane.js to account for tabmail-tab now being wrapped in an arrowscrollbox. r=jorgk DONTBUILD

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9075028 [details] [diff] [review]
bug1554630_tabmail-tabs (aleca WIP).patch

Is there any review necessary on the main patch here?

Also note comment #14: "When the tabs overflow, the alltabs button does not appear. ..."

Comment 23

[Alessandro Castellani [:aleca]]

I'll take care of this today and fix the latest quirky issues.
I'll do a personal review and then ask for a full review to Magnus.

Comment 25

[Alessandro Castellani [:aleca]]

Attachment: bug1554630_tabmail-tabs_tabsoverflow.patch

This fixes the tabs overflow toolbar button.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9075217 [details] [diff] [review]
bug1554630_tabmail-tabs_tabsoverflow.patch

Review of attachment 9075217 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is correct. Looks like it was just copied over from suite/.
But e.g. there is no UpdatedScrollButtonsDisabledState event anywhere.

Comment 27

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #22)

Is there any review necessary on the main patch here?

We both did parts, so let's call it reviewed.

Comment 28

[Alessandro Castellani [:aleca]]

I was wondering what the UpdatedScrollButtonsDisabledState was for.
The additions to the underflow and overflow events make the alltabs-button behave properly.
Also that part is incorrect?

Comment 29

[Magnus Melin [:mkmelin]]

Those look ok I think.

Comment 30

[Alessandro Castellani [:aleca]]

Attachment: bug1554630_tabmail-tabs_tabsoverflow.patch

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/66aa32e146d4
fix tab overflow for tambail-tab. r=mkmelin