Ctrl+click location item loads wrong URL

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Location Bar
P1
normal
RESOLVED FIXED
2 months ago
26 days ago

People

(Reporter: Oriol, Assigned: adw)

Tracking

(Depends on: 1 bug)

53 Branch
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
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.
(Assignee)

Comment 1

2 months ago
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.)
Assignee: nobody → adw
Status: NEW → ASSIGNED
(Reporter)

Comment 2

a month ago
> 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.
Priority: -- → P1
Whiteboard: [fxsearch]
Comment hidden (mozreview-request)
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.
(Reporter)

Comment 6

a month ago
Shift+click opens the page in a new window. This is also affected by this bug, and not fixed by the patch.
Comment hidden (mozreview-request)
Shift key fixed.

Comment 9

a month ago
mozreview-review
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.
(Reporter)

Comment 10

a month ago
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.
Comment hidden (mozreview-request)
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.
(Reporter)

Comment 18

a month ago
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;
        }
(Reporter)

Comment 19

a month ago
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.
Comment hidden (mozreview-request)
You're right, thanks.

Comment 22

a month ago
mozreview-review
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?
Attachment #8858970 - Flags: review?(mak77) → review+

Comment 23

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

Updated

a month ago
Depends on: 1360769
https://hg.mozilla.org/mozilla-central/rev/f6e8b57452bf
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 25

28 days ago
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]

Comment 26

26 days ago
The issue is no longer reproducible on Firefox Nightly 55.0a1 (build ID: 20170503030212).
Test were performed under Windows 10x64.
[bugday-20170503]
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.