Closed Bug 1519502 Opened 2 years ago Closed 1 year ago

Convert menu bindings to a Custom Element

Categories

(Toolkit :: XUL Widgets, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: timdream, Assigned: bgrins)

References

Details

(Whiteboard: [xbl-available])

Attachments

(2 files)

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

Unfortunately, they inherit menuitem-base.

Depends on: 1519495

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

Summary: Convert "menu-*" bindings to custom elements → Convert menu bindings to custom elements
Priority: -- → P3
Depends on: 1522290

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.

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

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.

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

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

Depends on: 1527105
Depends on: 1527680
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Summary: Convert menu bindings to custom elements → Convert menu bindings to a Custom Element
Attachment #9043454 - Attachment description: Bug 1519502 - Convert menu bindings to custom elements → Bug 1519502 - Convert menu bindings to a Custom Element
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/904cc7903feb
Convert menu bindings to a Custom Element r=surkov

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

Flags: needinfo?(bgrinstead)
Depends on: 1528268
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/223a8a0c4eed
Convert menu bindings to a Custom Element r=surkov
Regressions: 1545700
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

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

Flags: needinfo?(kazz)

Could you tell me how?

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

Regressions: 1545983
Regressions: 1546036
No longer regressions: 1546036
Depends on: 1546060
Regressions: 1546060
No longer depends on: 1546060
No longer regressions: 1546060
Type: enhancement → task
No longer blocks: 1487555
You need to log in before you can comment on or make changes to this bug.