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

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Lazy Monkey]

Attachment: 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

Comment 1

[:Gijs (he/him)]

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.

Comment 2

[Erica Wright [:ewright]]

Attachment: Bug 1382570 - Ignore non-left clicks on the bookmark icon.

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

Comment 3

[Erica Wright [:ewright]]

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

[:Gijs (he/him)]

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

Comment 5

[Erica Wright [:ewright]]

(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 :)

Comment 6

[:Gijs (he/him)]

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. :-)

Comment 7

[Drew Willcoxon :adw]

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

[:Gijs (he/him)]

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

Comment 9

[:Gijs (he/him)]

Comment on attachment 8893076 [details]
Bug 1382570 - Ignore non-left clicks on the bookmark icon.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c8cc6044599
Ignore non-left clicks on the bookmark icon. r=Gijs

Comment 11

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

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

Comment 12

[:Gijs (he/him)]

Redirecting.

Comment 13

[:Gijs (he/him)]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5343f4f6bbc
Ignore non-left clicks on the bookmark icon. r=Gijs

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e5343f4f6bbc

Comment 16

[Sayed [:LiquidO2]]

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

Comment 17

[Sajedul Islam]

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

[Sayed [:LiquidO2]]

As this bug is verified as fixed in both Linux(comment 16) and windows (comment 17), I am marking this bug as verified fixed.