Add add/remove context menus to the different items in the page action menu to add/remove them from the location bar, adding 'save to pocket' by default

VERIFIED FIXED in Firefox 57

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: adw)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-structure])

Attachments

(1 attachment)

See spec at https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/230998673 .

Basically, we want to:

- show the pocket item by default (in addition to the star)
- provide context menu options to remove items from the location bar
- provide context menu options in the menu to add options to the location bar
Flags: qe-verify+
Whiteboard: [photon-structure]
Priority: -- → P2
QA Contact: gwimberly
Assignee: nobody → adw
Status: NEW → ASSIGNED
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Depends on: 1374477
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
For-review patch with a test.  The test checks the context menus in both the panel and urlbar.  This patch also moves the click listener on the two bookmark star images up to the #star-button-box and add adds the context-menu-related attributes to #star-button-box too, instead of duplicating them on each image.  Hope that doesn't screw up tests.  We'll see.
Another thing is that currently right-clicks on the star button in the urlbar are incorrectly treated as left-clicks.  This fixes that too.  I think that was regressed in the bug that added the star button animation.
Forgot some awaits in the test.
Oh boy, looks like there were try failures on Windows because the mouse happened to be left over the urlbar popup in a subsequent test?
Let's try synthesizing a mousemove over the urlbar at the end of the test, plus a focus for good measure.
Try looks good this time.
Comment on attachment 8892706 [details]
Bug 1363188 - Add an add/remove context menu to page actions in the urlbar.

https://reviewboard.mozilla.org/r/163692/#review169130

::: browser/base/content/test/urlbar/browser_page_action_menu.js:488
(Diff revision 3)
> +      type: "contextmenu",
> +      button: 2,
> +    });
> +    await contextMenuPromise;
> +
> +    // The context menu should show "Add from Address Bar".  Click it.

Nit: "Add to Address Bar" :-)

::: browser/base/content/test/urlbar/head.js:233
(Diff revision 3)
> +function promisePanelEvent(panelIDOrNode, eventType) {
>    return new Promise(resolve => {
> -    gPageActionPanel.addEventListener(name, () => {
> +    let panel = typeof(panelIDOrNode) != "string" ? panelIDOrNode :
> +                document.getElementById(panelIDOrNode);
> +    if (!panel ||
> +        (eventType == "popupshowing" && panel.state == "open") ||

Nothing passes `popupshowing`. Did you mean `popupshown` ? Do we need this check?
Attachment #8892706 - Flags: review?(gijskruitbosch+bugs) → review+
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
(In reply to :Gijs from comment #13)
> ::: browser/base/content/test/urlbar/head.js:233
> (Diff revision 3)
> > +function promisePanelEvent(panelIDOrNode, eventType) {
> >    return new Promise(resolve => {
> > -    gPageActionPanel.addEventListener(name, () => {
> > +    let panel = typeof(panelIDOrNode) != "string" ? panelIDOrNode :
> > +                document.getElementById(panelIDOrNode);
> > +    if (!panel ||
> > +        (eventType == "popupshowing" && panel.state == "open") ||
> 
> Nothing passes `popupshowing`. Did you mean `popupshown` ? Do we need this
> check?

Ah yeah, I meant popupshown, thanks.  I copied and pasted this from browser_PageActions.js, so I fixed that there, too.
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47cc4909098d
Add an add/remove context menu to page actions in the urlbar. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/47cc4909098d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Blocks: 1387512
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.