[de-xbl] convert the tabmail-tabs binding
Categories
(Thunderbird :: Toolbars and Tabs, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(3 files, 8 obsolete files)
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
I think there is no m-c bug for tabbrowser-tabs conversion yet - but will depend on bug 1514926.
|
Reporter | |
Comment 1•5 years ago
|
||
tabbox.xml#tabs is going away in D32855 - bug 1555060
We may have to fork that binding temporarily.
|
|
Reporter | |
Updated•5 years ago
|
|
|
Reporter | |
Comment 3•5 years ago
|
||
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.)
|
|
Reporter | |
Comment 4•5 years ago
|
||
Removing the binding and css to hook it up too.
|
|
Reporter | |
Comment 5•5 years ago
|
||
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.
|
Assignee | |
Comment 6•5 years ago
|
||
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•5 years ago
|
||
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.
|
|
Assignee | |
Comment 8•5 years ago
|
||
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.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 9•5 years ago
|
||
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.
|
|
Assignee | |
Comment 10•5 years ago
|
||
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.
|
|
Assignee | |
Comment 12•5 years ago
|
||
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.
|
|
Assignee | |
Comment 13•5 years ago
|
||
More progress on fixing those tests.
|
||
Comment 14•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
Pushed by mozilla@jorgk.com: 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
|
|
Reporter | |
Comment 17•5 years ago
|
||
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.
|
|
Reporter | |
Comment 18•5 years ago
|
||
If you want something to land later...
|
|
Reporter | |
Comment 19•5 years ago
|
||
For other code changes (in general), should also add "is" in connectedCallback
|
|
||
Comment 20•5 years ago
|
||
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•5 years ago
|
||
Pushed by mozilla@jorgk.com: 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
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Comment 22•5 years ago
|
||
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. ..."
|
|
Assignee | |
Comment 23•5 years ago
|
||
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.
|
|
Assignee | |
Comment 25•5 years ago
|
||
This fixes the tabs overflow toolbar button.
|
|
Reporter | |
Comment 26•5 years ago
|
||
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.
|
|
Reporter | |
Comment 27•5 years ago
|
||
(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.
|
|
Assignee | |
Comment 28•5 years ago
|
||
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?
|
|
Assignee | |
Comment 30•5 years ago
|
||
|
|
Reporter | |
Updated•5 years ago
|
|
|
||
Comment 31•5 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/66aa32e146d4 fix tab overflow for tambail-tab. r=mkmelin
|
|
||
Updated•5 years ago
|
Description
•