right click on bookmark star shows both the bookmark edit popup and the context menu

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: Tortino, Assigned: ewright)

Tracking

(Blocks: 1 bug, {regression})

55 Branch
Firefox 57
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 disabled, firefox56 disabled, firefox57 verified)

Details

(Whiteboard: [photon-structure][fixed by bug 1363188])

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8888219 [details]
star right click.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

load a page and right click on the bookmark star


Actual results:

two dialogs open at the same time.


Expected results:

only the bookmark editor should open
Component: Untriaged → Toolbars and Customization

Updated

a year ago
Blocks: 1352120
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: right click on bookmark star shows two dialogs → right click on bookmark star shows both the bookmark edit popup and the context menu
Whiteboard: [photon-structure][triage]
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [photon-structure]

Comment 1

a year ago
This might get fixed "for free" in bug 1374477, but if it doesn't, no point trying to fix it before then because it'll just have to be fixed again after that lands.
Depends on: 1374477
(Assignee)

Updated

a year ago
Assignee: nobody → ewright
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
mozreview-review
Comment on attachment 8893076 [details]
Bug 1382570 - Ignore non-left clicks on the bookmark icon.

https://reviewboard.mozilla.org/r/164070/#review169374

::: browser/base/content/browser-places.js:1945
(Diff revision 1)
>      this.onStarCommand(aEvent);
>    },
>  
>    onStarCommand(aEvent) {
> -    // Ignore clicks on the star if we are updating its state.
> -    if (!this._pendingUpdate) {
> +    // Ignore non-left clicks on the star, or if we are updating its state.
> +    if (!this._pendingUpdate && aEvent.button == 0) {

should we use checkForMiddleClick, and put this as an onCommand instead?

Comment 4

a year ago
(In reply to Erica from comment #3)
> Comment on attachment 8893076 [details]
> Bug 1382570 - Ignore non-left clicks on the bookmark icon.
> 
> https://reviewboard.mozilla.org/r/164070/#review169374
> 
> ::: browser/base/content/browser-places.js:1945
> (Diff revision 1)
> >      this.onStarCommand(aEvent);
> >    },
> >  
> >    onStarCommand(aEvent) {
> > -    // Ignore clicks on the star if we are updating its state.
> > -    if (!this._pendingUpdate) {
> > +    // Ignore non-left clicks on the star, or if we are updating its state.
> > +    if (!this._pendingUpdate && aEvent.button == 0) {
> 
> should we use checkForMiddleClick, and put this as an onCommand instead?

I don't think we should here, it might be useful in bug 1377967 (though I haven't thought about it).


I'm really sorry, but I think that although this didn't get fixed in bug 1374477, it then got fixed in bug 1363188, which is in today's nightly. Can you check if it's fixed for you there? To be fair, it got fixed in a different location, so I'm wondering if we might still need the patch here if there are more callsites...
Flags: needinfo?(ewright)
(Assignee)

Comment 5

a year ago
(In reply to :Gijs from comment #4)

> 
> I'm really sorry, but I think that although this didn't get fixed in bug
> 1374477, it then got fixed in bug 1363188, which is in today's nightly. Can
> you check if it's fixed for you there? To be fair, it got fixed in a
> different location, so I'm wondering if we might still need the patch here
> if there are more callsites...

Yep, tried it, it did. Not a problem :)
Flags: needinfo?(ewright)

Comment 6

a year ago
mozreview-review
Comment on attachment 8893076 [details]
Bug 1382570 - Ignore non-left clicks on the bookmark icon.

https://reviewboard.mozilla.org/r/164072/#review169792

Clearing review per the bug comments, then. :-)
Attachment #8893076 - Flags: review?(gijskruitbosch+bugs)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → disabled
status-firefox56: --- → disabled
status-firefox57: --- → fixed
Resolution: --- → FIXED
Whiteboard: [photon-structure] → [photon-structure][fixed by bug 1363188]
Target Milestone: --- → Firefox 57
Iteration: --- → 57.1 - Aug 15
IMO this is a good patch though, worth taking still.  I can't think of any circumstance where non-left-clicking the bookmark star should trigger the panel.  If it were fixed this way, then the regression reported in this bug wouldn't have happened to begin with.  We're bound to regress it again at some point.

Comment 8

a year ago
(In reply to Drew Willcoxon :adw from comment #7)
> IMO this is a good patch though, worth taking still.  I can't think of any
> circumstance where non-left-clicking the bookmark star should trigger the
> panel.  If it were fixed this way, then the regression reported in this bug
> wouldn't have happened to begin with.  We're bound to regress it again at
> some point.

This is fair.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

a year ago
mozreview-review
Comment on attachment 8893076 [details]
Bug 1382570 - Ignore non-left clicks on the bookmark icon.

https://reviewboard.mozilla.org/r/164072/#review169952
Attachment #8893076 - Flags: review+

Comment 10

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4c8cc6044599
Ignore non-left clicks on the bookmark icon. r=Gijs
Backing this out to see if it fixes the frequent occurrence of https://treeherder.mozilla.org/logviewer.html#?job_id=120810616&repo=autoland which started happening around the time this landed.

https://hg.mozilla.org/integration/autoland/rev/20b1593ade611ac72add7281d08080f7b8424f7d
Flags: needinfo?(jaws)

Comment 12

a year ago
Redirecting.
Flags: needinfo?(jaws) → needinfo?(gijskruitbosch+bugs)

Comment 13

a year ago
The test takes a path where aEvent is a command event, and on command events aEvent.button is undefined, which fails an == 0 check. I updated the patch to check if the event is actually a click, and only check the button in that case.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 14

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5343f4f6bbc
Ignore non-left clicks on the bookmark icon. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/e5343f4f6bbc
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED

Comment 16

a year ago
I can reproduce this bug in Nightly 56.0a1 (2017-07-20) in 64bit Linux

I can verify that this bug is fixed in latest nightly 57.0a1

Build ID 	20170806100257
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170802]

Comment 17

a year ago
I can reproduce this bug in Nightly 56.0a1 (2017-07-20) on Windows 8.1 , 64 Bit ! 

I can verify that this bug is fixed in latest nightly 57.0a1

Build ID 	20170806100257
User Agent 	Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170802]

Comment 18

a year ago
As this bug is verified as fixed in both Linux(comment 16) and windows (comment 17), I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.