Closed Bug 1412170 Opened 7 years ago Closed 7 years ago

Integrate WebExtension page action context menus with the Photon page action context menu

Categories

(Firefox :: Address Bar, enhancement, P1)

Product:
Firefox
Component:
Address Bar
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files)

Bug 1412170 - Integrate WebExtension page action context menus with the Photon page action context menu: Photon page action changes.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1412170 - Integrate WebExtension page action context menus with the Photon page action context menu: WebExtensions changes.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
Bug 1395387 is the main bug that reconciles WebExtension page actions and Photon page actions, but we left context menus as a follow-up, and that's this bug.

Aaron, do you have any thoughts on how to integrate an extension's context menu with our current "Add/Remove from Address Bar" context menu on page actions?  I'm thinking just appending a separator and then our "Add/Remove" item, like:

Extension menu menu item 1
Extension menu menu item 2
...
Extension menu menu item n
---------------------------
Add/Remove from Address Bar
Flags: needinfo?(abenson)
Appending that to the context menu sounds reasonable to me.

I'm just curious if there's a limit to the number of extension menu items that can be placed in the context menu and if they can have icons associated with them?
Flags: needinfo?(abenson)
(In reply to Aaron Benson from comment #1)
> I'm just curious if there's a limit to the number of extension menu items
> that can be placed in the context menu and if they can have icons associated
> with them?

Yes, currently the limit is only 6 top-level items, and each menu item can have an icon.

(See: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus , https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-menus.js#20)
Jared, would you mind reviewing the first patch here since you've worked on page actions before?  I'd ask Gijs but he's away.  The main thing this does is make it possible to open the context menu on disabled actions in the urlbar.  That's currently not possible because of `pointer-events: none`.  We want the context menu to be openable because the "Add/Remove from Location Bar" context menu item should always be accessible.  The other main thing this does is to rename the context menu.  It shouldn't really have "panel" in its name since it's used for the urlbar buttons too.
Shane, this adds back popupshowing handling to ext-pageAction.js.  Everything else -- the menu building/modification parts -- just work when pointed at the new context menu, which is cool.
Comment on attachment 8923026 [details]
Bug 1412170 - Integrate WebExtension page action context menus with the Photon page action context menu: WebExtensions changes.

https://reviewboard.mozilla.org/r/194232/#review199314
Attachment #8923026 - Flags: review?(mixedpuppy) → review+
(In reply to Drew Willcoxon :adw from comment #6)
> Shane, this adds back popupshowing handling to ext-pageAction.js. 
> Everything else -- the menu building/modification parts -- just work when
> pointed at the new context menu, which is cool.

Nice!
Comment on attachment 8923025 [details]
Bug 1412170 - Integrate WebExtension page action context menus with the Photon page action context menu: Photon page action changes.

https://reviewboard.mozilla.org/r/194230/#review199658

::: browser/base/content/browser.css:1446
(Diff revision 1)
>    }
>  }
>  
>  .urlbar-page-action[disabled] {
> -  pointer-events: none;
>    -moz-user-focus: ignore;

Should we drop this too? Keeping this here will not allow for keyboard navigation + context-menu key to open the context menu for Add/Remove button.
Attachment #8923025 - Flags: review?(jaws) → review+
Yes, you're probably right.  Thanks, Jared.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b46ed411edf
Integrate WebExtension page action context menus with the Photon page action context menu: Photon page action changes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/a9a646fe54ec
Integrate WebExtension page action context menus with the Photon page action context menu: WebExtensions changes. r=mixedpuppy
Hey, a linter error that's actually useful.
I added an onRemovedFromWindow callback to PageActions.Action, to go along with the existing onBeforePlacedInWindow callback.  ext-pageActions uses it to remove the popupshowing listener that it adds in onBeforePlacedInWindow.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1759c0cbd1a
Integrate WebExtension page action context menus with the Photon page action context menu: Photon page action changes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/7f0fcaa42ba6
Integrate WebExtension page action context menus with the Photon page action context menu: WebExtensions changes. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/a1759c0cbd1a
https://hg.mozilla.org/mozilla-central/rev/7f0fcaa42ba6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Hi Drew, 

Any suggested steps to be able to verify this? Thanks!
Flags: needinfo?(adw)
You can install any webextension that adds a page action with a context menu.  Here's one for example: https://addons.mozilla.org/en-US/firefox/addon/crxviewer/

1. Install that extension
2. Open https://addons.mozilla.org/en-US/firefox/addon/crxviewer/ (or select its tab if it's already open)
3. Verify that a yellow "CRX" icon appears in the urlbar
4. Right-click it
5. Verify that a context menu opens and looks like this:

   Hide this button
   ----------------
   Don't Show in Address Bar
   ----------------
   Manage Extension...

6. Open the page action menu (the "..." button in the urlbar)
7. Verify that a menu item with the CRX icon appears there too
8. Right-click it
9. Verify that the same context menu appears
Flags: needinfo?(adw)
One other thing.  I don't know what build you'll be testing with, but the "Don't Show in Address Bar" and "Manage Extension..." context menu items were added in bug 1413574.  If your build doesn't have that bug's fix, then the context menu will look a little different, but the important thing is that it should have the "Hide this button" item and a separator below it, since that item comes from the extension.
Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Beta 58.0b4 (64-bit).
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Depends on: 1435992
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: