Closed Bug 1545263 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: Paenglab, Assigned: mkmelin)

References

Details

(Whiteboard: [Thunderbird-testfailure: Mochi all])

Attachments

(1 file, 1 obsolete file)

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.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)

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.

Flags: needinfo?(geoff)

The bindings going away here are are:

Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(mkmelin+mozilla)
Summary: Port bug 1519502: Convert menu bindings to a Custom Element → Adjust usage of the menu-iconic binding which is removed in bug 1519502.

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?

Flags: needinfo?(geoff)
Whiteboard: [Thunderbird-testfailure: Mochi all]

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

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.

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.

Flags: needinfo?(geoff)

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.

Flags: needinfo?(vporof)
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(bgrinstead)

(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).

Flags: needinfo?(vporof)
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(bgrinstead)

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.

Flags: needinfo?(bgrinstead)

(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.

Flags: needinfo?(bgrinstead)
Depends on: 1546467
Attached patch 1545263-menu-iconic-binding-1.diff (obsolete) (deleted) — Splinter Review

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

Assignee: mkmelin+mozilla → geoff
Status: NEW → ASSIGNED
Attachment #9060282 - Flags: review?(mkmelin+mozilla)

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

Attached patch bug1545263_menu-iconic.patch (deleted) — Splinter Review

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

Assignee: geoff → mkmelin+mozilla
Attachment #9060282 - Attachment is obsolete: true
Attachment #9060282 - Flags: review?(mkmelin+mozilla)
Attachment #9060323 - Flags: review?(geoff)
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.
Attachment #9060323 - Flags: review?(geoff) → review+

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.

Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: