Closed Bug 430577 Opened 12 years ago Closed 1 year ago

ctrl/cmd + click on bookmarks toolbar folder should not open folder

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 65
Tracking Status
firefox65 --- verified

People

(Reporter: mattsmeltz, Assigned: Gijs)

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042304 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042304 Minefield/3.0pre

When I command click on a bookmark toolbar folder, the bookmarks open in tabs, but the menu also opens.

Reproducible: Always

Steps to Reproduce:
1. command click on bookmarks toolbar folder
Actual Results:  
bookmarks open in tabs and folder menu opens

Expected Results:  
bookmarks open in tabs
I see the same using Vista HP SP1 and this build:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008042311 Minefield/3.0pre Firefox/3.0 ID:2008042311
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Version: unspecified → Trunk
Hardware: PC → All
Since this is a very visible bug to laptop users (or anyone without a middle mouse button), I nominated to see if drivers think this is worthy to be fixed before final release.
Flags: blocking-firefox3?
Summary: cmd + click on bookmarks toolbar folder should not open folder → ctrl/cmd + click on bookmarks toolbar folder should not open folder
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
This was bothering me so I thought I'd try my hand at a patch.  It works for me on Windows but it should probably be tested on Mac to make sure that cmd+click works and on Linux to make sure that it doesn't break drag-and-drop of folders.
Attachment #604716 - Flags: review?
It has been a week so I'm asking for help.  Does anybody have any advice on how to get someone to take a look at this?
Hi James! Thanks for the patch! I'll find someone to take a look.
Comment on attachment 604716 [details] [diff] [review]
Don't open dropdown when opening new tabs

Hi, when requesting reviews you should target someone, or it may be basically invisible in the middle of the many bugmails we get.  Usually you can check other fixed bugs in the same component to get an idea on possible reviewers.
I'll look into this.
Attachment #604716 - Flags: review? → review?(mak77)
Comment on attachment 604716 [details] [diff] [review]
Don't open dropdown when opening new tabs

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

So, looks like you are working on an outdated tree, all the mousedown and mouseup stuff has gone recently, so you actually need to add back the mousedown event and handle your case.
On the other side, this also means you can avoid most of this ifdef madness, so the new patch should be simpler.
Apart this the approach looks fine to me.
Feel free to reask for review to me once you have an updated patch, and thanks for your contribution.

::: browser/components/places/content/browserPlacesViews.js
@@ +1648,5 @@
> +#else
> +      let modifKey = aEvent.ctrlKey || aEvent.shiftKey;
> +#endif
> +      if (modifKey)
> +      {

please brace in line with the ifs
Attachment #604716 - Flags: review?(mak77) → feedback+
Updated patch, addressed review comment.
Attachment #604716 - Attachment is obsolete: true
Attachment #609040 - Flags: review?(mak77)
Comment on attachment 609040 [details] [diff] [review]
Don't open dropdown when opening new tabs, version 2

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

Actually, this has the side effect of regressing ctrl+mousedown, move down to an entry, release, that on Mac may be an expected action (I know that at least mousedown, move down, release is).

I see that BEH_onClick has code that handles closing popups, but doesn't handle toolbarbuttons with type="menu"... 
Handling in BEH_onClick doesn't regress the above gesture, though may cause flicker.  Handling in the view regresses the gesture, but doesn't flicker.

Mano, do you know if we did that to support mousedown+move+release, or it just works by luck? The code here is quite old.

::: browser/components/places/content/browserPlacesViews.js
@@ +1528,5 @@
> +      let modifKey = aEvent.ctrlKey || aEvent.shiftKey;
> +#endif
> +      if (modifKey) {
> +        // Do not open the popup since BEH_onClick is about to
> +        // open the bookmarks in new tabs

s/open the bookmarks in new tabs/open all child uri nodes in tabs./
Attachment #609040 - Flags: feedback?(mano)
Comment on attachment 609040 [details] [diff] [review]
Don't open dropdown when opening new tabs, version 2

As discussed over IRC, click-n-hold is only well defined for modifier-less clicks. In apps which don't care about modifiers, accel+click-n-hold also works, but that doesn't mean much.

Safari, by the way, makes the same trade-off.
Attachment #609040 - Flags: feedback?(mano) → feedback+
Comment on attachment 609040 [details] [diff] [review]
Don't open dropdown when opening new tabs, version 2

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

Fine then, r=me with the comment change suggested at the end of comment 9.  Thank you.
Attachment #609040 - Flags: review?(mak77) → review+
Changed comment as requested.  Hopefully ready for checkin.
Attachment #609040 - Attachment is obsolete: true
Attachment #615256 - Flags: review+
What's currently needed on this bug? (Someone's patch was r+'d 7 years ago, but it never landed. Diff of code in the patch looks pretty much like current code.)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to custom.firefox.lady [:tawn] from comment #13)
> What's currently needed on this bug? (Someone's patch was r+'d 7 years ago,

Ugh. This is just sad. I'll ping mhoye to see if there's a structured way we can look for this type of bug.

> but it never landed. Diff of code in the patch looks pretty much like
> current code.)

I have no idea, but in case it's just landing this patch, I've updated it (we no longer preprocess this file, plus I noticed we could use event.getModifierState() instead of the conditional for mac vs. everything else) and put it in phabricator for review. I'm not sure how quickly this will land because people are traveling and at all-hands this week.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mhoye)
Trypush:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3858c13854025d7fb080b526c6e28240fceb7635

(Accidentally includes some printing test changes, so ignore leaks there (bug 1503887))
Assigning this to me so it doesn't get lost again.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs (he/him) from comment #16)
> Trypush:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3858c13854025d7fb080b526c6e28240fceb7635
> 
> (Accidentally includes some printing test changes, so ignore leaks there
> (bug 1503887))

Clearly my idea to use getModifierState() more widely doesn't work when we don't implement it on command events (but do on mouse ones...). Reverted that change (but kept it in the area of the patch because it works there). I'll file a separate bug for getting accelKey / getModifierState() to work on (command) events because hardcoding all these assumptions everywhere gets (a) tiring and (b) probably doesn't obey the relevant prefs for this stuff (yes... we have hidden prefs about which key gets treated as 'accel').

New trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a304bcb78583b808df443de4f7d7302a407c555e
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e134df617e0c
Don't open dropdown on bookmarks toolbar folder when opening new tabs, r=mak
https://hg.mozilla.org/mozilla-central/rev/e134df617e0c
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Hello,  

I reproduced this issue using Fx 64.0, build ID: 20181206201918, on MacOS.

I can confirm this issue is verified as fixed using Fx 65.0b5, build ID:  20181217180946, on Windows 10 x32, macOS X 10.13 and Ubuntu 18.04 x64.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mhoye)
You need to log in before you can comment on or make changes to this bug.