1545263 - Adjust usage of the menu-iconic binding which is removed in bug 1519502.

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Richard Marti (:Paenglab)]

Bug 1519502 will probably break TB as it changes how the childs are addressed, see https://hg.mozilla.org/integration/autoland/rev/904cc7903feb

It is now on autoland.

Comment 1

[Geoff Lankow (:darktrojan)]

I think we're mostly okay. Sometimes the menu elements from Lightning don't get the custom element applied, which I don't understand. Only browser_ext_menus.js failed in the Try run I did.

Comment 2

[Magnus Melin [:mkmelin]]

The bindings going away here are are:

• menu.xml#menu-menubar (we don't use it except for toolkit usage)
• menu.xml#menu-menubar-iconic (we don't use it except for toolkit usage)
• menu.xml#menu (we don't use it except for toolkit usage)
• menu.xml#menu-iconic - this one we use (but do not inherit from): https://searchfox.org/comm-central/search?q=menu.xml%23menu-iconic&case=false&regexp=false&path=

Comment 3

[Jorg K (CEST = GMT+2)]

Indeed:

TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_menus.js | uncaught exception - TypeError: menu.render is not a function at @chrome://global/content/elements/menu.js:148:10

Care to fix it?

Comment 4

[:aceman]

The menu.render error can also be seen in mozmill runs, e.g. https://taskcluster-artifacts.net/IjiZ2mhjQOyoPygmo5BxZg/0/public/logs/live_backing.log

Comment 5

[:aceman]

There is even this:
JavaScript error: chrome://global/content/elements/menu.js, line 275: TypeError: this.delayConnectedCallback is not a function

The 'delayConnectedCallback' is not defined in the file, but it seems to be something internal to custom elements. If even that is missing, something is thoroughly hosed.

Comment 6

[Geoff Lankow (:darktrojan)]

I think I have worked out why there are problems with render and delayConnectedCallback functions. This is odd, but it seems to be true.

The problematic elements are from Lightning via the overlay loader. They all seem to be converted to custom elements (that is, connectedCallback runs), I think. Then the garbage collector gets a hold of them and the custom element stuff is stripped away. I tested this by calling Cu.forceGC() before opening any menu. If a reference is kept, e.g. by logging the element to the console, the menu item appears as normal, if not, it isn't. I've tried this with and without calling document.adoptNode, it makes no difference.

Comment 7

[Geoff Lankow (:darktrojan)]

Can any of you Firefox people help with comment 6? We're moving elements from one document to another, so I wonder if there's some crucial reference attached to their original document that should be on the destination document.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Geoff Lankow (:darktrojan) from comment #1)

I think we're mostly okay. Sometimes the menu elements from Lightning don't get the custom element applied, which I don't understand. Only browser_ext_menus.js failed in the Try run I did.

I was noticing this too with some menuitems in about:preferences in the WIP at Bug 1500626. Are you using CDATA by any chance? I noticed that the problem went away with Bug 1545962 (though we should track down what's happening regardless).

Comment 9

[Brian Grinstead [:bgrins]]

I've asked about this in https://bugzilla.mozilla.org/show_bug.cgi?id=1500626#c14.

Comment 10

[Geoff Lankow (:darktrojan)]

No CDATA, but the problematic nodes are coming from another document. Do you see anything that could be GC related, I'm becoming more convinced that's a part of the problem.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Geoff Lankow (:darktrojan) from comment #10)

No CDATA, but the problematic nodes are coming from another document. Do you see anything that could be GC related, I'm becoming more convinced that's a part of the problem.

I think you are right: https://bugzilla.mozilla.org/show_bug.cgi?id=1500626#c26.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1545263-menu-iconic-binding-1.diff

I think this is all we need to do once bug 1546467 is landed.

Comment 13

[Geoff Lankow (:darktrojan)]

Started a Try run on top of that bug which is currently on autoland.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=88344052b8f93b4443bfb35ec3ad45e4aa2ac821

Comment 14

[Geoff Lankow (:darktrojan)]

I've abandoned that Try run, as the mochitest suite has fallen over.

Comment 15

[Geoff Lankow (:darktrojan)]

Fixed the test suite, now let's try Try again.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e36f402c2a2d09d246fde1c5ae7eada69c6e87e9

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: bug1545263_menu-iconic.patch

That would have made the stock icons stop working. To make them work we need to add a class menu-iconic.

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 9060323 [details] [diff] [review]
bug1545263_menu-iconic.patch

Okay, that makes sense. I think you should consider going to one attribute per line where you're changing things anyway.

Comment 18

[Magnus Melin [:mkmelin]]

In this case it's a bit special, since the class will change what kind of menu you have. It's not an "is", but close. So I think it makes sense to be able to see what id that class is applied to.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/58e7ebc66ab9
Adjust usage of the menu-iconic binding which was removed in bug 1519502. r=darktrojan