Remove the style-editor-options-popu XUL menupopup from the StyleEditor and use the JS Menu API instead

RESOLVED FIXED in Firefox 68

Status

enhancement
P3
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: pbro, Assigned: hemakshis, Mentored)

Tracking

unspecified
Firefox 68

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 months ago

We aim to get rid of XUL eventually, which includes removing <menupopup> where we can as well as localization via DTD files.
We can do both of these things in 1 particular part of the StyleEditor quite easily: the style-editor-options-popu menu.

The markup is here: https://searchfox.org/mozilla-central/rev/486b64e4bff86b7988af8c8b80845404ad197533/devtools/client/styleeditor/index.xul#74-84
The goal is to remove this entirely.

Instead, create the menu dynamically from JS whenever needed.
The API we can user here is Menu and MenuItem, in these files:
/devtools/client/framework/menu.js
/devtools/client/framework/menu-item.js

There's an example usage here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/markup/markup-context-menu.js#369

We should do something very similar here for the StyleEditor, and hook up a listener like element.addEventListener("click", this._onMenu); where element would be the options button in the toolbar, and where _onMenu would build the menu content using the API mentioned.

Right now, because the menu is build in XUL, the labels are localized with keys from a DTD file. By doing this, we would have to move the corresponding properties to this file instead.

Reporter

Updated

3 months ago
No longer depends on: 1538167
Reporter

Updated

3 months ago
See Also: → 1538177
Assignee

Comment 1

3 months ago

Hi Patrick!

I would love to work on this issue. Could you assign it to me?

Hemakshi

Flags: needinfo?(pbrosset)
Reporter

Comment 2

3 months ago

Thanks for your interest in fixing this bug. I'll assign it to you now.
Please feel free to drop me a comment here and ask any questions you might have. In the meantime, I'll assume comment 0 is enough to get started.

Assignee: nobody → sachdev.hemakshi
Mentor: pbrosset
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Reporter

Updated

3 months ago
Severity: normal → enhancement
Priority: -- → P3
Assignee

Comment 3

3 months ago

Hi Patrick,

I am really very sorry for the delay, was all caught up last week.
I just looked at the code, and a little confused too.

Where should I add the code for new context menu, there seems to be one StyleEditorUI.jsm file or create a new one like you shared in comment #0?

Flags: needinfo?(pbrosset)
Reporter

Comment 4

3 months ago

Hi, thanks for reaching out. No problem about the delay, there's no big urgency here.
Your question is a good one. There are several solutions to do this. Here's one suggestion:

  • add a new function to /devtools/client/styleeditor/StyleEditorUtil.jsm that is responsible for creating the menu (using the Menu and MenuItem APIs discussed before).
  • Import this function in /devtools/client/styleeditor/StyleEditorUI.jsm, add the right click event listener inside StyleEditorUI and use that new imported function to display the menu.

Does that make sense? Sorry I only briefly looked at this. Hopefully it's enough to get you started. But if you need more help, let me know.

Flags: needinfo?(pbrosset)
Assignee

Comment 5

3 months ago

(In reply to Patrick Brosset <:pbro> from comment #4)

Hi, thanks for reaching out. No problem about the delay, there's no big urgency here.
Your question is a good one. There are several solutions to do this. Here's one suggestion:

  • add a new function to /devtools/client/styleeditor/StyleEditorUtil.jsm that is responsible for creating the menu (using the Menu and MenuItem APIs discussed before).
  • Import this function in /devtools/client/styleeditor/StyleEditorUI.jsm, add the right click event listener inside StyleEditorUI and use that new imported function to display the menu.

Does that make sense? Sorry I only briefly looked at this. Hopefully it's enough to get you started. But if you need more help, let me know.

Yup, it does make sense. Thanks a lot :)

Assignee

Comment 7

2 months ago

Hi Patrick,
I'm successfully being able to return the new Menu on click, but don't know why it's not being displayed in the UI.
Can you please have a look?

Thanks!
Hemakshi

Flags: needinfo?(pbrosset)
Reporter

Comment 8

2 months ago

Hi Hemakshi. This patch looks great. Thanks for the work so far.
I think the reason the menu isn't being displayed is that you build the menu correctly, but then you don't actually open it.
The menu object has a method called menu.popup(x, y, toolbox) to display it.
Let me know if you're able to make this work (you should be able to find examples of this in the code for reference). And then I'll review the code in phabricator.
Thanks

Flags: needinfo?(pbrosset)
Assignee

Comment 9

2 months ago

Hi,
Thanks, it worked :) I have updated the patch. Please have a look at it once :)

Comment 10

2 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50ce9167f1ce
Remove the style-editor-options-popup XUL menupopup from the StyleEditor and use the JS Menu API instead. r=pbro

Comment 11

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.