924723 - Right- or middle-clicking on the star icon in the location bar should not bookmark the current page

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Christopher Butcher]

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.66 Safari/537.36

Steps to reproduce:

Reproducible: Always
Right- or middle-click on the star icon in the location bar.


Actual results:

A bookmark is created and the star icon becomes highlighted.


Expected results:

A bookmark should not be created, nor should the star icon become highlighted when right- or middle-clicked.
Note that right-clicking on the Reload button next to it does not trigger a reload.

Comment 1

[Christopher Butcher]

Attachment: Proposed patch

Comment 2

[Christopher Butcher]

Attachment: screenshot-of-problem.png

Comment 3

[[:Aleksej]]

wfm: 2013-04-24-03-09-17-mozilla-central-firefox-23.0a1.en-US.linux-x86_64
australis: 2013-04-25-03-08-45-mozilla-central-firefox-23.0a1.en-US.linux-x86_64
…
australis: 2013-05-11-03-11-23-mozilla-central-firefox-23.0a1.en-US.linux-x86_64
bug: 2013-05-12-03-09-08-mozilla-central-firefox-23.0a1.en-US.linux-x86_64

Comment 4

[[:Aleksej]]

(I don't know if it was introduced at the end or in the middle/beginning)

You need to request a review of your patch from some specific developer to get it in.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 814686 [details] [diff] [review]
Proposed patch

Review of attachment 814686 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the checkin flag until this has been reviewed.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 814686 [details] [diff] [review]
Proposed patch

Review of attachment 814686 [details] [diff] [review]:
-----------------------------------------------------------------

it's a valid bug, though I think you should fix the caller, the onclick handler here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#557 to be something like "if (event.button === 0) BookmarkingUI.onCommand(event);"
This should not happen in UX though, even if looks like a broken merge may have reintroduced the star-icon on the urlbar?

Comment 7

[[:Aleksej]]

(In reply to Marco Bonardo [:mak] from comment #6)
> This should not happen in UX though, even if looks like a broken merge may
> have reintroduced the star-icon on the urlbar?

If you are talking about the mentions of "australis" in my comment 3, that means "the star button is a big one outside of the urlbar, and this bug doesn't happen with it".

Comment 8

[Christopher Butcher]

Attachment: star-bookmark2.patch

Now checking for left-button click in caller, as per [:mak]'s suggestion.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 817570 [details] [diff] [review]
star-bookmark2.patch

In the future, you can just set the checkin-needed keyword on the bug. Also, I shortened the commit message a bit to better fit with the usual standards followed.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/fcf4875ad29e

Comment 11

[Phil Ringnalda (:philor)]

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