Closed Bug 1428991 Opened 3 years ago Closed 3 years ago

Remove the usage of the tabmail-tabs class

Categories

(Thunderbird :: Theme, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(2 files, 3 obsolete files)

Like the FX bug 1408362we should use the ID instead of the class because we have only one tabcontainer in the whole application and a class makes here no sense.

But we should leave the class as it is because extensions could break when they use the class instead of the ID.
Attached patch tabmail-tabs_class.patch (obsolete) — Splinter Review
I also renamed the tooltip to reflect the ID and not the class.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8940975 - Flags: review?(jorgk)
OK, but FF changed it like this:
-.tabbrowser-tabs {
+#tabbrowser-tabs {

Why are you proposing:
-.tabmail-tabs {
-  -moz-binding: url("chrome://messenger/content/tabmail.xml#tabmail-tabs");
+#tabcontainer {
+  -moz-binding: url("chrome://messenger/content/tabmail.xml#tabcontainer");
 }

Why change the ID, can't you just leave the same ID?

Then you can leave the tooltip IDs as they are and also remove this hunk:
-  <binding id="tabmail-tabs"
+  <binding id="tabcontainer"
I made this to be consistent with the IDs. "tabmail-tabs" is the class in XUL file and in XML file it's the ID. After the patch "tabcontainer" would be the ID in both files.
This appears quite messy. Looking at FF's browser.xul they have
  <tabs id="tabbrowser-tabs"
and they removed class="tabbrowser-tabs" from that element.

We call the equivalent thing "tabcontainer" leading to tabcontainer="tabcontainer" which is tabcontainer="tabbrowser-tabs" for FF.

What we should be doing is to align with FF and use id="tabmail-tabs". I don't think it's a big change, see:
https://searchfox.org/comm-central/search?q=tabcontainer&case=true&regexp=false&path=
Comment on attachment 8940975 [details] [diff] [review]
tabmail-tabs_class.patch

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

I don't think this is quite right yet.

::: mail/base/content/messenger.xul
@@ +245,5 @@
>              id="tabcontainer"
>              align="end"
>              setfocus="false"
>              onclick="document.getElementById('tabmail').onTabClick(event);"
>              class="tabmail-tabs"

Well, the idea was to eliminate that class, right? But you left it in when FF removed it.
Attachment #8940975 - Flags: review?(jorgk)
The problem would be extensions which now use tabcontainer and then fail when the ID would be tabmail-tabs now.
You'd have to change
calendar/test/mozmill/testTodayPane.js
mail/test/mozmill/tabmail/test-tabmail-customize.js
and I can run those tests for you or you do a try run to be on the safe side.

Let me look at the extensions.
FX removed because they don't need to be compatible with extensions. See comment 0.
Looks like https://dxr.mozilla.org/addons isn't working right now so I can't check.

OK, if you want minimal change, then I'd do this:
Put
id="tabcontainer"  // For consistency, this should be "tabmail-tabs" but we can't change it
                   // due to add-ons that might be using it.
class="tabmail-tabs"  // Unused, only maintained for add-ons.

I wouldn't change the tooltip ID from a good name to a bad name and I wouldn't change the binding ID from a good name to a bad name.
It's really unfortunately that this patch would promote the bad ID even further:
-  .tabmail-tabs:not(:-moz-lwtheme) {
+  #tabcontainer:not(:-moz-lwtheme) {
I'm not so against leaving the class for add-ons to use, but we should really check whether the ID is used or could be changed.
I could also only change the CSS files to use the ID instead the class.
Yes, but as I said in comment #10, the bad name would be more widely used then. Let me check the add-on data when it works again so perhaps we can change the ID.
Aceman can you please check the use of "tabcontainer" on https://dxr.mozilla.org/addons.
Flags: needinfo?(acelists)
There are about 95000 hits of 'tabcontainer' in addons. It seems to be a popular property of gBrowser.
There are some that use "tabcontainer" as id or class for some of their elements.
And some do apply css on #tabcontainer.

What exactly are we looking for?
Flags: needinfo?(acelists)
The tabs element (the one that holds all tabs) has in TB the "tabcontainer" ID and not "tabmail-tabs" like FX has "tabbrowser-tabs". This would be more logical. Now it's the question if we could change it or if we break too many extensions.

I saw also that "tabcontainer" is also used in FX JS code. So we would also see FX extensions using this. Is where a possibility to filter only TB extensions?
From IRC yesterday:
20:54:35 - jorgk: [snip] so how many add-ons use the string tabcontainer
20:55:27 - jorgk: well, they need to use this as ID
20:55:42 - jorgk: so not as attribute.
20:55:59 - jorgk: since there is also a XUL attribute by that name.
20:57:32 - jorgk: But tabcontainer sadly is the name of the mail tabs container whereas FF use "tabbrowser-tabs" and I'd like to use "tabmail-tabs"
20:57:46 - aceman: let me see

So to rephrase this: We need to know which add-ons use the id of "tabcontainer" to access the this element:
https://dxr.mozilla.org/comm-central/rev/007155df7eb96eaa992085feb7e086d61d96e577/mail/base/content/messenger.xul#245

From what I can see, we use it three times in the tests:
https://searchfox.org/comm-central/search?q=tabcontainer&case=true&regexp=false&path (scroll to the bottom)
Textual Occurrences
	calendar/test/mozmill/testTodayPane.js
133	id("tabcontainer")/{"first-tab":"true","type":"folder"}/
	mail/test/mozmill/tabmail/test-tabmail-customize.js
45	mc.rightClick(mc.eid("tabcontainer"));
61	let tabbar = mc.e("tabcontainer");

Never mind, my https://dxr.mozilla.org/addons works again. The search needs to be case-sensitive (getting rid of gBrowser.tabContainer), so at least the search needs to be @tabcontainer yielding about 2000 results. I'll work on it and attach a list later, DXR seems quite temperamental :-(
Paenglab, I do not understand it. You say TB has "tabcontainer" ID and we want to change it to tabmail-tabs. But the patch renames in the opposite direction.

So I was not sure what to look for. For addons, that look for our "tabcontainer", or those that define their own (which we may then clash with).

Yes, the addon dxr is unreliable. It gave me errors yesterday and only started giving search results now in the morning.
The patch removes the use of class tabmail-tabs to using the id, so for example:
-  .tabmail-tabs:not(:-moz-lwtheme) {  <== class
+  #tabcontainer:not(:-moz-lwtheme) {  <== ID.

It would be nicer if the id were tabmail-tabs, so then we would do:
-  .tabmail-tabs:not(:-moz-lwtheme) {  <== class
+  #tabmail-tabs:not(:-moz-lwtheme) {  <== ID.

Aceman, thanks for your help, I'll take it from there. As you said, DXR doesn't work well. When I had finally scrolled to the end of the 2000 matches and wanted to copy the results for further processing, FF crashed.
So why does the patch not convert to #tabmail-tabs ? You want to see first if addons expect the #tabcontainer that exists in TB for now.

The FF crash is interesting. I also observed some weirdness today when checking the dxr results, like FF partly freezing for a while, I could witch to other tabs, but their contents were empty. Only after a while things got to normal and the tabs re-rendered correctly. It could be it is some problem in FF core, that just manifests differently on platforms. But FF should not crash in the new multi-process world.
(In reply to :aceman from comment #19)
> So why does the patch not convert to #tabmail-tabs ? You want to see first
> if addons expect the #tabcontainer that exists in TB for now.

Exactly
Exactly. Richard wants to kill the class .tabmail-tabs as FF did and switch to the id. But I'm opposed to using the bad id even more widely making it even harder to ever go to a good id. So we need to see how many add-ons we'd break.

Before FF crashed I spotted at least one add-on using the tabcontainer id and that was a TB add-on since other matches had "mail" somewhere. I'll have to revisit the page, but haven't had any time for it this morning.
Setting NI so I won't forget to try to look at DXR again. Right now it's not working again.
Flags: needinfo?(jorgk)
I worked out that the correct query string needs to be;
  @'"tabcontainer"'
@ to force case-sensitive and single quotes to enclose the double quotes. on C-C's DXR that works well:
https://dxr.mozilla.org/comm-central/search?q=%40%27%22tabcontainer%22%27&redirect=false

Sadly no luck on add-ons DXR so far. On #developers they told me that it's been broken for a long time, but this morning I got some results before it decided not to work any more.
OK, with the search string @'"tabcontainer"' I got 289 results. Sadly FF eventually crashes since there are four result lines which are mega-long, about 20 MB per line. I managed to save the page, but even Notepad++ freezes when opening the file. So I used |cut -b 0-2000| to cut after the first 2000 characters and then removed the overlong lines by hand.

The result is enclosed, no guarantee that it's complete, in fact it looks incomplete since it only lists 87 add-ons and 100 time the character ♢.

In those incomplete results I can see at least three or four affected add-ons:
addons/424270/modules/utils.js
674 tabContainer = mail3PaneWindow.document.getElementById( "tabcontainer" );

addons/464538/chrome/content/script/main.js
5 //var tab = document.getElementById("tabcontainer")  <== comment

addons/475804/chrome/content/overlay.js
70 for (var i = 0; i < document.getElementById("tabcontainer").childNodes.length; i++)
72 if (document.getElementById("tabcontainer").childNodes[i].id == tab.id.replace("searchaslist","searchasfacet")) {
74 document.getElementById("tabcontainer").childNodes[i].hidden = false;
80 var target = document.getElementById("tabcontainer");
106 var target = document.getElementById("tabcontainer");
112 
while(document.getElementById("tabcontainer").childNodes[document.getElementById("tabmail").tabContainer.selectedIndex].hidden == true) {
130 var target = document.getElementById("tabcontainer");
136 while(document.getElementById("tabcontainer").childNodes[document.getElementById("tabmail").tabContainer.selectedIndex].hidden == true) {
171 var tabcont = document.getElementById("tabcontainer");

addons/685599/js/script.js
737 document.getElementById("tabcontainer").style.display = "block";
744 document.getElementById("tabcontainer").style.display = "none";

Looking these up in my list I find:
424270 - https://addons.mozilla.org/addon/424270 - https://addons.mozilla.org/en-GB/firefox/addon/znotes/ (400 users)
464538 - https://addons.mozilla.org/addon/464538 - https://addons.mozilla.org/en-GB/firefox/addon/dm-sync/ (9 users)
475804 - https://addons.mozilla.org/addon/475804 - https://addons.mozilla.org/en-GB/firefox/addon/search-as-list/ (2400 users)
685599 - not in my list.

So my conclusion is: Change the ID *now*. None of these add-ons have been maintained recently, the most recent update was to "Search as list" two years ago.

Due to the various M-C changes affecting add-ons it is unlikely that these add-ons still function, so changing the class here and now will just be an easy update. I can contact the author of "Search as list" personally.

Opinions?

P.S.: DXR only seems to work once early in the mornings, so I'll try to look again tomorrow to see whether more add-ons are affected.
Flags: needinfo?(jorgk)
Attached patch tabmail-tabs_class.patch (obsolete) — Splinter Review
Okay, this renames tabcontainer to tabmail-tabs.

Setting r? in case you think, it's okay to rename.
Attachment #8940975 - Attachment is obsolete: true
Attachment #8941905 - Flags: review?(jorgk)
Yes, I'd approve this if you fix the test listed and the end of comment #16. I also suggest a try run for one platform only, say Mac on TC, since it's fastest and only Mozmill, so:
hg qnew -m "try: -b o -p macosx64 -u mozmill" try

Can we please add a comment as shown in comment #9:
class="tabmail-tabs"  // Unused, only maintained for add-ons.

What do you think about removing that class? I didn't search for the string on DXR. Is it more likely for add-ons to use the class? Themes perhaps? Otherwise, I'd remove it, too, but it's less of an issue.
Attached patch tabmail-tabs_class.patch (obsolete) — Splinter Review
Ah yes, sorry was in porting all the removals and forgot about this.

Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c304fb9a330e52a2a703a5e7f6e09d73ef7b36db
Attachment #8941905 - Attachment is obsolete: true
Attachment #8941905 - Flags: review?(jorgk)
Attachment #8941918 - Flags: review?(jorgk)
Comment on attachment 8941918 [details] [diff] [review]
tabmail-tabs_class.patch

Thanks. Send any complaining add-on authors my way ;-)

r+ assuming that the try passes.
Attachment #8941918 - Flags: review?(jorgk) → review+
It seems a comment inside a element in XUL isn't possible. Removed the comment about the class.

New try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=472fc5a7917b114d6b194be838b60a41e0e31425
Attachment #8941918 - Attachment is obsolete: true
Attachment #8941947 - Flags: review+
Grrr, yes, you can, but it's normal HTML style comment. Anyway, if the try is OK, I can experiment with adding a comment locally.
I also tried the <!-- ... --> syntax without luck.
OK, try looks good. I will add a comment *before* then.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b5669673cf9a
Remove the usage of the tabmail-tabs class. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
You need to log in before you can comment on or make changes to this bug.