Convert menu bindings to a Custom Element

RESOLVED FIXED in Firefox 68

Status

()

task
P3
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: timdream, Assigned: bgrins)

Tracking

(Blocks 2 bugs)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [xbl-available])

Attachments

(2 attachments)

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

Updated

4 months ago
Priority: -- → P3

Updated

4 months ago
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.

Assignee

Comment 4

4 months ago

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

Assignee

Comment 6

4 months ago

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

Assignee

Updated

3 months ago
Depends on: 1527105
Assignee

Updated

3 months ago
Depends on: 1527680
Assignee

Updated

2 months ago
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
Blocks: 1487555

Comment 9

a month ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/904cc7903feb
Convert menu bindings to a Custom Element r=surkov
Assignee

Comment 11

a month ago

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)
Assignee

Updated

a month ago
Depends on: 1528268

Comment 13

a month ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/223a8a0c4eed
Convert menu bindings to a Custom Element r=surkov

Updated

a month ago
Regressions: 1545700

Comment 14

a month ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Assignee

Comment 16

a month ago

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

Assignee

Comment 18

a month ago

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

Updated

a month ago
Regressions: 1545983

Updated

a month ago
Regressions: 1546036
Assignee

Updated

a month ago
No longer regressions: 1546036
Depends on: 1546060
Regressions: 1546060
Assignee

Updated

a month ago
No longer depends on: 1546060
No longer regressions: 1546060
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.