Closed
Bug 1428991
Opened 6 years ago
Closed 6 years ago
Remove the usage of the tabmail-tabs class
Categories
(Thunderbird :: Theme, enhancement)
Thunderbird
Theme
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 59.0
People
(Reporter: Paenglab, Assigned: Paenglab)
Details
Attachments
(2 files, 3 obsolete files)
184.03 KB,
text/html
|
Details | |
11.30 KB,
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
I also renamed the tooltip to reflect the ID and not the class.
Comment 2•6 years ago
|
||
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"
Assignee | ||
Comment 3•6 years ago
|
||
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.
Comment 4•6 years ago
|
||
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®exp=false&path=
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
The problem would be extensions which now use tabcontainer and then fail when the ID would be tabmail-tabs now.
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
FX removed because they don't need to be compatible with extensions. See comment 0.
Comment 9•6 years ago
|
||
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.
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
I could also only change the CSS files to use the ID instead the class.
Comment 12•6 years ago
|
||
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.
Comment 13•6 years ago
|
||
Aceman can you please check the use of "tabcontainer" on https://dxr.mozilla.org/addons.
Flags: needinfo?(acelists)
Comment 14•6 years ago
|
||
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)
Assignee | ||
Comment 15•6 years ago
|
||
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?
Comment 16•6 years ago
|
||
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®exp=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 :-(
Comment 17•6 years ago
|
||
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.
Comment 18•6 years ago
|
||
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.
Comment 19•6 years ago
|
||
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.
Assignee | ||
Comment 20•6 years ago
|
||
(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
Comment 21•6 years ago
|
||
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.
Comment 22•6 years ago
|
||
Setting NI so I won't forget to try to look at DXR again. Right now it's not working again.
Flags: needinfo?(jorgk)
Comment 23•6 years ago
|
||
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.
Comment 24•6 years ago
|
||
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)
Assignee | ||
Comment 25•6 years ago
|
||
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)
Comment 26•6 years ago
|
||
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.
Assignee | ||
Comment 27•6 years ago
|
||
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 28•6 years ago
|
||
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+
Assignee | ||
Comment 29•6 years ago
|
||
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+
Comment 30•6 years ago
|
||
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.
Assignee | ||
Comment 31•6 years ago
|
||
I also tried the <!-- ... --> syntax without luck.
Comment 32•6 years ago
|
||
OK, try looks good. I will add a comment *before* then.
Comment 33•6 years ago
|
||
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: 6 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 59.0
You need to log in
before you can comment on or make changes to this bug.
Description
•