1. Open Firefox with a clean profile 2. Open a new tab, go to "example.com", close the tab 3. Open a new tab, go to "example.org", close the tab 4. Type "exam" to the location bar. 5. Location bar's autocompletion suggest either "example.com" or "example.org" 6. The location list below the location bar will show both "example.com" or "example.org" 7. Place the mouse over the other entry, e.g. if the location bar suggests "example.com", hover "example.org" 8. Press the Control key 9. Click with the mouse Expected result: The hovered entry opens in a new tab What went wrong? The suggested entry opens in a new tab Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f40e24f40b4c4556944c762d4764eace261297f5&tochange=bc086e9044e6537d18d385f417248cdb4c14c3af Either bug 1295458 or bug 1345834, I suspect the former.

Thanks for filing this. I haven't looked at the code yet, but I think I might know what's happening. The new-tab path is using the selected result, not the clicked result. (Although now that I type that, well yeah, that's the bug.)

> The new-tab path is using the selected result, not the clicked result. I think the problem is not that the wrong one is used, the problem is that they should be the same. That is, hovering some entry should select it. This Ctrl+click problem what the bigger annoyance, but I am also having problems about pressing up or down arrows after hovering an entry.

(In reply to Oriol from comment #2) > That is, hovering some entry should select it. The scope of the change is exactly so that hovering does NOT select anymore, so that's not a problem. It's likely some cases are not yet properly handled with the new behavior.

This was a little tricky to track down. I wondered why normal-clicking results loads them fine, but Ctrl-clicking them doesn't. What's the difference? The code comment in the patch explains what's going on. The root problem is here in the listbox implementation: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml#975 If the accel key is not held down while you click, then the listbox sets its selectedIndex to the index of the item you click. That means that the autocomplete controller sees the "selected" result as being the one that you clicked, even though now in the urlbar we are separating the selected result from the clicked one. The autocomplete controller sets the input's value to the selected index, so when the urlbar handles the command and uses this.value as the url to load, everything works. But if you hold down the accel key while you click, then the listbox does not set its selectedIndex. Which is why in this bug the truly selected index (the blue result) is getting used.

Shift+click opens the page in a new window. This is also affected by this bug, and not fixed by the patch.

Shift key fixed.

Comment on attachment 8858970 [details] Bug 1356641 - Ctrl+click location item loads wrong URL. https://reviewboard.mozilla.org/r/130970/#review134768 The code in listbox seems to do the right thing for single selection: http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/toolkit/content/widgets/listbox.xml#1010 and the urlbar richlistbox has selType = "" so it should go through that code path. Could you please point me to the listbox code that sets/doesn't-set the selection? it may actually be just an oversight in listbox code, I don't see why it wouldn't want to set a selection on a single selection list. I wonder if the problem is that when there's a modified the selection happens on click rather than on mousedown? that would sound strange though. I need some clarifications to proceed here. ::: browser/base/content/urlbarBindings.xml:421 (Diff revision 2) > + // autocomplete controller. The controller then sets the input's > + // value to the value at selectedIndex (in this case), which means > + // that at this point for normal clicks, the input's value is the > + // value of the clicked item. > + // > + // But when the Ctrl key (meta key on Macs) is held down, the nit: I'd just compact as But when a modifier key (shift, ctrl or meta) is held down... And remove the "Same for the Shift key" at the end.

I see a problem with this way to solve the issue: 1. Press a modifier key 2. Mousedown some item. It is not selected, because there is a modifier key. 3. Release the modifier key 4. Mouseup. The item is not selected, because there is no modifier key. Result: you navigate to the initially selected item (the first one) Expected result: you navigate to the clicked item Another problem caused by bug 1295458: 1. Mousedown some item (without modifier key). It is selected. 2. Move to mouse to another item. 3. Mouseup. It is not selected. Result: You navigate to the mousedown item Expected result: You navigate to the mouseup item. I think this should work like this: 1. Mousedown selects the item, whether there are modifier keys or not. Really, I can't see how one is supposed to drag url items, so the check in https://dxr.mozilla.org/mozilla-central/rev/20dff607fb88ee69135a280bbb7f32df75a86237/toolkit/content/widgets/listbox.xml#984 should just be disabled for the urlbar so that modifier keys work. Any alternative is just a hack. 2. Mousedown makes mouseenter another item select that other item 3. Mouseup navigates to the selected item. No hacks to change the selected item. This seems what Chrome does, btw.

The listbox's click handler is called *after* the autocomplete controller's EnterMatch. It's: (1) listbox mousedown (2) nsAutoCompleteController::EnterMatch (3) listbox click EnterMatch is the thing that fills the input with the selected value, so the listbox's click handler doesn't even enter into the picture for this problem. I linked to the problematic mousedown code in my earlier comment BTW: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/listbox.xml#975

(That reply was to Marco's comment 9)

(In reply to Oriol from comment #10) > I see a problem with this way to solve the issue: That's true. > Really, I can't see how one is supposed to drag url items, so the check True too, but the urlbar isn't the only listbox in Firefox, and I didn't want to mess with the listbox code for fear of breaking something unrelated. We could probably override mousedown in autocomplete-richlistbox though. Ideally the interaction would work like you suggest. So I'm OK with doing that. I'd like to see what Marco thinks though.

I think it's worth seeing if we can override mousedown, autocomplete needs are quite different from other richlistbox needs.

OK, this overrides mousedown on richlistitem. This is better. It also implements drag handling so that mousing down on one item and then dragging to another item selects the second item, like Oriol suggests.

(In reply to Drew Willcoxon :adw from comment #16) > OK, this overrides mousedown on richlistitem. autocomplete-richlistitem I mean.

Thanks, works great! Just a problem that I hadn't noticed when I proposed this: you can mousedown some item, move the mouse outside the list, and mouseup. Then move the mouse back over the list, it will think you are still holding down the button. So, instead of storing a boolean, I think you could use https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons if (!control || !(event.buttons & 1) || control.disabled) { return; }

I see the wheel button can also be used to open a new tab, so let mouseDown = event.buttons & 0b101; /* primary or middle buttons */ if (!control || !mouseDown || control.disabled) { return; } Or maybe also include the right button, which can select (but not navigate to) items: let mouseDown = event.buttons & 0b111; /* primary, secondary, or middle buttons */ The 4th and 5th buttons just go back or forward in history and have no effect in the urlbar, so I would not include them.

You're right, thanks.

Comment on attachment 8858970 [details] Bug 1356641 - Ctrl+click location item loads wrong URL. https://reviewboard.mozilla.org/r/130970/#review137214 It's a bit disappointing not having a test, but let's fix the regression. Could you please file a followup for a test?

Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f6e8b57452bf Ctrl+click location item loads wrong URL. r=mak

I have reproduced this bug with Nightly 55.0a1 (2017-04-14) on Windows 8.1 , 64 Bit ! This bug's fix is Verified with latest Nightly ! Build ID 20170501030204 User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 [testday-20170428]

The issue is no longer reproducible on Firefox Nightly 55.0a1 (build ID: 20170503030212). Test were performed under Windows 10x64. [bugday-20170503]

Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170602030204) on Mac OS X 10.12 and on Ubuntu 16.04 x64.