1519502 - Convert menu bindings to a Custom Element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Similar to menuitem (bug 1519495) the bindings bound to <menu> can be converted to multiple custom elements via prepending/appending children upon CE connection.

• menu-base
• menu
• menu-menubar
• toolbarbutton-dropdown
• menu-iconic
• menu-menubar-iconic

Noted that the toolbarbutton.xml#menu binding is unrelated to this group. We have a problem with our XBL Component Tree page that should be corrected.

raw note (with menu missing from the list since it was copied from the page):

menu-base
We could first move the menu selector from xul.css to menu-base binding. Then all other non-menucaption bindings below this could potentially be collapsed into a single CE, or multiple with [is] attribute
menu-menubar
menu-iconic
Three of these: bookmarksToolbarFolderMenu, menu_unsortedBookmarks, menu_mobileBookmarks
toolbarbutton-dropdown
menu-menubar-iconic

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Unfortunately, they inherit menuitem-base.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The menucaption binding is doable without all the <children> maneuver when menu-base and menuitem-base are implemented.

raw note:

menucaption
This is only used for <optgroup>s on select: https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/toolkit/modules/SelectParentHelper.jsm#340
Should actually be doable now, since all parent bindings just use inherits, properties, methods

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I made a mistake in bug 1518932 — The MenuBaseControl here is also useful in this bug.

https://hg.mozilla.org/integration/autoland/rev/c1032d34b5e0#l13.26

We will want to move it somewhere that can be shared between menulist.js and menu.js here yet somehow only construct it lazily.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #3)

I made a mistake in bug 1518932 — The MenuBaseControl here is also useful in this bug.

https://hg.mozilla.org/integration/autoland/rev/c1032d34b5e0#l13.26

We will want to move it somewhere that can be shared between menulist.js and menu.js here yet somehow only construct it lazily.

Why does it need to be lazy? Is there a perf issue with attaching it as a base class into MozElements object like these: https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/toolkit/content/customElements.js#288?

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Creating a class definition on every document that is only useful for the <menu> and <menulist> usage may not be slow. I was thinking about memory usage, but perhaps that's tiny as well.

Anyway, a lazily Object.defineProperty could do the job, if we really want to.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5)

Creating a class definition on every document that is only useful for the <menu> and <menulist> usage may not be slow. I was thinking about memory usage, but perhaps that's tiny as well.

Anyway, a lazily Object.defineProperty could do the job, if we really want to.

Oh yeah, it wouldn't hurt to use XPCOMUtils.defineLazyGetter. Don't think we'd need to do anything more fancy than that.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Right. We may want to move the namespace of these mixin methods and defined classes also. There will be many!

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Bug 1519502 - Convert menu bindings to a Custom Element

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/904cc7903feb
Convert menu bindings to a Custom Element r=surkov

Comment 10

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1519502, bug 1528268) for Crashtest failures in toolkit/content/tests/chrome/test_popupincontent.xul. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241127128&repo=autoland&lineNumber=4281

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=904cc7903feb74eeb70b415403fa33f8df4c39ff

Backout:
https://hg.mozilla.org/integration/autoland/rev/90a95d56b18df1b2a658dd8b156486e31b3ee0ec

Comment 11

[Brian Grinstead [:bgrins]]

In the process of adding support to sizetopopup parents I accidentally prevented eager render for the markup in that test. Fix incoming.

Comment 12

[Brian Grinstead [:bgrins]]

That test is fixed now as per https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5a10cbd9dcd15634af441c3afaaa8f83839ac2&selectedJob=241204598.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/223a8a0c4eed
Convert menu bindings to a Custom Element r=surkov

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/223a8a0c4eed

Comment 15

[Kazumasa Hasegawa (Kazz)]

Attachment: Top level menu with sub menu not shown

Top level menu with sub menu not shown by this change.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Kazumasa Hasegawa (Kazz) from comment #15)

Created attachment 9059496 [details]
Top level menu with sub menu not shown

Top level menu with sub menu not shown by this change.

Kazumasa, could you test with this build and see if this issue is fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8be840e5002f879e228ddaa2dd29cb48a016714&selectedJob=241429834? I think it might be the same root cause as Bug 1545700.

Comment 17

[Kazumasa Hasegawa (Kazz)]

Could you tell me how?

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Kazumasa Hasegawa (Kazz) from comment #17)

Could you tell me how?

In "Job Details" at the bottom of the page there is a list of artifacts uploaded. One of them is target.zip - if you download it and unzip the file there should be a firefox.exe inside of it that can be ran (ideally using a new profile to avoid downgrading your profile when you go back to your normal firefox... although since you are on nightly anyway that's not as big of an issue).

Comment 19

[Kazumasa Hasegawa (Kazz)]

Commented on https://bugzilla.mozilla.org/show_bug.cgi?id=1545700#c13