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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years 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])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Posted image 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

2 years 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

2 years 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

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

Comment 3

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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

2 years 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

2 years 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

2 years 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)

Updated

2 years ago
Blocks: 1387512

Comment 12

2 years ago
Redirecting.
Flags: needinfo?(jaws) → needinfo?(gijskruitbosch+bugs)

Comment 13

2 years 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

2 years 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: 2 years ago2 years ago
Resolution: --- → FIXED
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

2 years 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]
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.