1363188 - 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

Status: VERIFIED FIXED

(Firefox :: Address Bar, enhancement, P1)

Description

[:Gijs (he/him)]

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

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1363188 - Add an add/remove context menu to page actions in the urlbar.

Review commit: https://reviewboard.mozilla.org/r/163692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163692/

Comment 2

[Drew Willcoxon :adw]

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.

Comment 3

[Drew Willcoxon :adw]

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.

Comment 4

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05cc7325d3f2

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8892706 [details]
Bug 1363188 - Add an add/remove context menu to page actions in the urlbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163692/diff/1-2/

Comment 6

[Drew Willcoxon :adw]

Forgot some awaits in the test.

Comment 7

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f6e9c49c55

Comment 8

[Drew Willcoxon :adw]

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?

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8892706 [details]
Bug 1363188 - Add an add/remove context menu to page actions in the urlbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163692/diff/2-3/

Comment 10

[Drew Willcoxon :adw]

Let's try synthesizing a mousemove over the urlbar at the end of the test, plus a focus for good measure.

Comment 11

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=686cec4747c0

Comment 12

[Drew Willcoxon :adw]

Try looks good this time.

Comment 13

[:Gijs (he/him)]

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?

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 8892706 [details]
Bug 1363188 - Add an add/remove context menu to page actions in the urlbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163692/diff/3-4/

Comment 15

[Drew Willcoxon :adw]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47cc4909098d
Add an add/remove context menu to page actions in the urlbar. r=Gijs

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47cc4909098d

Comment 18

[u554753]

Verified on Windows, Mac, and Ubuntu.