554165 - removal of gBrowser.mStrip breaks common way of accessing tab context menu

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Bug 347930 removed tabbrowser's mStrip member, which is used by several addons to access the tab context menu.

Common patterns of use:
1) toggling gBrowser.mStrip.collapsed
2) accessing tab context menu via:
   - gBrowser.mStrip.firstChild.nextSibling
   - mStrip.getElementsByAttribute("anonid", "tabContextMenu")[0]
   - mStrip.childNodes[1]
3) adding event listeners to the tab strip (gBrowser.mStrip.{add|remove}EventListener)
4) accessing the tab tooltip (mStrip.firstChild)
5) adding event listener to the tab container (gBrowser.mStrip.childNodes[2])

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I don't think there's a good way to maintain 1). I think this only affects a couple of extension, though (tabmixplus and tree-style-tabs being the most prominent ones, I think).

I tried restoring mStrip as a fake JS object as suggested by Dao on IRC. This addressed 2), 3), and 5).

The other alternative is to restore an anonymous hbox to tabbrowser-tabs to contain most of its contents, and expose it as mStrip. This addresses 2)-5).

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: jsobject approach

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: extra box approach (diff -w)

Comment 4

[Dão Gottwald [:dao]]

Tab Mix Plus had been fixed in a dev update one day ofter the patch landed, surprisingly.

I have read through quite a few extensions on mxr, but I haven't seen an extension access the tooltip, so I guess it's not very common. Probably not worth maintaining.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

https://mxr.mozilla.org/addons/search?string=gBrowser.mStrip.firstChild&filter=hookAttr has several hits (all copied from IETab, apparently), and https://mxr.mozilla.org/addons/source/2224/chrome/content/supert/toomanytabs.js#280 is another example.

Comment 6

[Dão Gottwald [:dao]]

It looks like https://mxr.mozilla.org/addons/source/2224/chrome/content/supert/toomanytabs.js#280 still wouldn't do anything useful, as it tries to replace code that isn't actually there.

IETab isn't compatible with 3.6.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> It looks like
> https://mxr.mozilla.org/addons/source/2224/chrome/content/supert/toomanytabs.js#280
> still wouldn't do anything useful, as it tries to replace code that isn't
> actually there.

Apparently it has been abandoned long ago: https://addons.mozilla.org/en-US/firefox/addon/2224

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 434042 [details] [diff] [review]
jsobject approach

>+      <field name="tabContextMenu" readonly="true">
>+        this.tabContainer.contextMenu;
>+      </field>

I think this should be a property. If an extension caused tabContainer to be reconstructed, e.g. by moving it somewhere else, tabContainer.contextMenu would need to be reevaluated.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: jsobject approach

OK, let's go with this then.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 434282 [details] [diff] [review]
jsobject approach

I should test this more first...

Comment 11

[:Gavin Sharp [email: [e-mail]]]

This does fix a few extensions (especially once I fix the add/removeEventListener forwarding to use an intermediate function to avoid xpconnect weirdness). 

Unfortunately it appears that:

document.getAnonymousElementByAttribute(gBrowser, 'anonid', 'tabContextMenu'); 

is even more common, and we can't really easily fix that.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

I'm kind of wavering on whether the mStrip re-addition is still worth doing. I think it probably is...

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 434335 [details] [diff] [review]
patch

>+            childNodes: [null, this.tabContextMenu, this.tabContainer],
>+            firstChild: { nextSibling: this.tabContextMenu },

It might be better to a) make these getters or b) make mStrip a property, for the same reason that tabContextMenu is a property rather than a field.

>+            getElementsByAttribute: function (attr, attrValue) {
>+              if (attr == "anonid" && attrValue == "tabContextMenu")
>+                return [this.self.tabContextMenu];
>+              return null;

null should be [] instead.

>+            addEventListener: function (a,b,c) { this.self.tabContainer.addEventListener(a,b,c); },
>+            removeEventListener: function (a,b) { this.self.tabContainer.removeEventListener(a,b); }

removeEventListener takes three arguments as well.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

Comment 15

[Dão Gottwald [:dao]]

Bug 554279 comment 16 has a valid point, it would be easier to extend the menu (namely with standard XUL overlays) if it wasn't anonymous. But that doesn't block this bug, as the fake mStrip as well as tabContextMenu could be changed to point somewhere else at some later point.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/b95a8d162e8b

I filed bug 554991 about making it non-anonymous.