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

()

Firefox
Location Bar
P1
normal
VERIFIED FIXED
4 months ago
12 days 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])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
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
(Reporter)

Updated

4 months ago
Flags: qe-verify+
Whiteboard: [photon-structure]

Updated

4 months ago
Priority: -- → P2
QA Contact: gwimberly
(Assignee)

Updated

3 months ago
Assignee: nobody → adw
Status: NEW → ASSIGNED

Updated

3 months ago
Iteration: --- → 55.6 - May 29
Priority: P2 → P1

Updated

3 months ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12

Updated

2 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
(Assignee)

Updated

2 months ago
Depends on: 1374477

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Updated

a month ago
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24

Updated

a month ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

22 days ago
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.
(Assignee)

Comment 3

22 days ago
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.
(Assignee)

Comment 4

22 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05cc7325d3f2
Comment hidden (mozreview-request)
(Assignee)

Comment 6

22 days ago
Forgot some awaits in the test.
(Assignee)

Comment 7

22 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8f6e9c49c55
(Assignee)

Comment 8

22 days ago
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 hidden (mozreview-request)
(Assignee)

Comment 10

22 days ago
Let's try synthesizing a mousemove over the urlbar at the end of the test, plus a focus for good measure.
(Assignee)

Comment 11

22 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=686cec4747c0
(Assignee)

Comment 12

22 days ago
Try looks good this time.
(Reporter)

Comment 13

22 days ago
mozreview-review
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+

Updated

22 days ago
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Comment hidden (mozreview-request)
(Assignee)

Comment 15

22 days ago
(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

22 days ago
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

Comment 17

21 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47cc4909098d
Status: ASSIGNED → RESOLVED
Last Resolved: 21 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Blocks: 1382570
Blocks: 1387512
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.