Open Bug 1364415 Opened 7 years ago Updated 27 days ago

leave location bar suggestions open when I middle click or ctrl click an item

Categories

(Firefox :: Address Bar, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: designakt, Unassigned)

References

Details

(Keywords: polish, ux-natural-mapping, Whiteboard: [qx:spec][fxsearch])

Attachments

(1 file, 1 obsolete file)

From websites people are used to have tabs be opened in the background with their current context stay the same, if they open a link with a middle-click or a Ctrl/Cmd+click. This same behavior is now being implemented for bookmark menus and should also be the default behavior in the location bar.

Currently the location bar adheres to opening a new tab, but does this in the foreground, destroying the search people started, and potentially requireing people to enter their search query again if the saw a second result they wanted to open. Additionally that re-search comes with a risk of changed search results - not including the second result one wanted to open.

See bug 260611 for more talk on the topic. (https://bugzilla.mozilla.org/show_bug.cgi?id=260611)
Priority: -- → P3
Whiteboard: [qx:spec] → [qx:spec][fxsearch]
Normal urls should open tab in background and leave menu open when ctrl-clicked etc. (as in bookmark menus), but does this need to support "actions"? If so, what is the desired behavior? Ctrl-clicking "Switch to tab" could conceivably be useful to open a new tab in background with that same url, but I doubt anyone would typically want to "Search [google]" in the background; they'd want to see the results. "Visit" doesn't seem like a frequent use case for opening in background either. (Can't remember offhand if there are any other "actions".)
(In reply to custom.firefox.lady [:tawn] from comment #1)
> but I doubt anyone would typically want to "Search [google]" in
> the background; they'd want to see the results. "Visit" doesn't seem like a
> frequent use case for opening in background either. (Can't remember offhand
> if there are any other "actions".)

You're wrong at that. Many times I search for the same term on different search engines. So if someone is a clicker (I prefer alt+enter) the easiest way to do that is to open one search in the background and then edit the search keyword (repeat for all desired search engines).

(Can't say anything about switch to tab. It's a horrible feature that I disable since it appeared.)
Not extensively tested; looking for general feedback on method. For simplicity, I am just extracting the url and using it to open a new tab, while leaving the urlbar dropdown open. (Actual menu-closing hapens in cpp; this is a js workaround.)
Attachment #9033605 - Flags: feedback?(mak77)
I don't think we should implement this in the existing code, we should rather try to implement this for the Quantum Bar code that we aim to enable this year, replacing the crazy js+cpp+toolkit situation with a more manageable codebase. You can enable that code by flipping the browser.urlbar.quantumbar pref, and the code lives in browser/components/urlbar. Clearly some of the code is still shared with the old binding, but there's no cpp autocomplete controller in the middle, so the implementation may be a little bit less hackish.
Attachment #9033605 - Flags: feedback?(mak77)
also note, that's a WIP codebase, lots of things are still broken.

(In reply to Marco Bonardo [::mak] from comment #4)

I don't think we should implement this in the existing code, we should
rather try to implement this for the Quantum Bar code that we aim to enable
this year...

Due to Bug 1519589 this cannot be fully implemented yet. Would you prefer a patch for a partial implementation (Ctrl-click) soonish, or would it be better to wait for the quantumbar code to be more complete?

Flags: needinfo?(mak77)

Nevermind, looks like Bug 1519589 is being fixed much quicker than I expected.

Flags: needinfo?(mak77)

Not sure why some sections of code in the file uses
this.closePopup();
while others directly use
this.view.close();
so I just went with whatever was already there.

Found that clicking e.g. the 'home button' while the dropdown is open does not close it (it previously did on dev edition 65.0b12), but that also occurs without this patch.

Attachment #9033605 - Attachment is obsolete: true
Attachment #9039467 - Flags: review?(mak77)

Uploaded patch a month ago. Feedback?

Flags: needinfo?(mak77)

(In reply to custom.firefox.lady [:tawn] from comment #8)

Not sure why some sections of code in the file uses
this.closePopup();
while others directly use
this.view.close();
so I just went with whatever was already there.

closePopup is a compatibility API for the old nsIAutoCompletePopup interface, it's just for plugging the new code... internally UrlbarInput should use this.view.close() imo, and closePopup should clarify that in its javadoc (we should also have a bug about changing it once we can remove the old code).

Flags: needinfo?(mak77)
Comment on attachment 9039467 [details] [diff] [review]
Make Ctrl-click & middle-click open background tab and leave urlbar dropdown open.

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

Not as trivial as I was hoping, there's a bit of risk in changing the focus/revert behavior before Quantum Bar is shipped, because it may cause subtle bugs that confuse QA... I'd probably wait for QB to become the default before proceeding. Shouldn't take too long though, I suspect we'll flip on the pref in 68.

::: browser/components/urlbar/UrlbarInput.jsm
@@ +292,5 @@
>    pickResult(event, result) {
>      this.setValueFromResult(result);
>  
> +    let modifKey = AppConstants.platform === "macosx" ? event.metaKey
> +                                                      : event.ctrlKey;

The code changed quite a bit, couldn't you just use "where" now?

@@ +318,5 @@
>        case UrlbarUtils.RESULT_TYPE.TAB_SWITCH: {
> +        if (!PlacesUIUtils.openInTabClosesMenu && (event.button == 1 || modifKey)) {
> +          where = "tabshifted";
> +          break;
> +        }

What's the purpose here? didn't you already set tabshifted in whereToOpen? a comment would help.
I suspect in the tab switch case we should not keep the view open, anyway... Unless it's being overridden.

@@ -760,5 @@
>  
> -    // Focus the content area before triggering loads, since if the load
> -    // occurs in a new tab, we want focus to be restored to the content
> -    // area when the current tab is re-selected.
> -    browser.focus();

shouldn't we still focus() here in the default case?

@@ +999,5 @@
> +    // area when the current tab is re-selected.
> +    let browser = this.window.gBrowser.selectedBrowser;
> +    browser.focus();
> +    if (!PlacesUIUtils.openInTabClosesMenu) {
> +      this.handleRevert();

I must think further about the implications here, it looks a bit scary from a spoofing point of view, but it still a minor thing (one must close the popup to actually use content, anyway).
Attachment #9039467 - Flags: review?(mak77)

Just to note, this should also have the tests extended/fixed/new tests.

Has canonize been taken into consideration in the decision to implement this? Ctrl-Enter will react differently on the first item in the result list than the remaining items. (Users triggering canonize will be sent somewhere other than the displayed result.) Might it be better to implement this as Alt-click/Enter (Alt-Enter already defaults to opening url in new tab & it would eliminate code to check for macosx) or restrict this bug to mouse-only?

(In reply to Marco Bonardo [::mak] from comment #11)

::: browser/components/urlbar/UrlbarInput.jsm
@@ +292,5 @@

>    pickResult(event, result) {
>      this.setValueFromResult(result);
>  
> +    let modifKey = AppConstants.platform === "macosx" ? event.metaKey
> +                                                      : event.ctrlKey;

The code changed quite a bit, couldn't you just use "where" now?

We could use where == "tabshifted", but that would also keep the view open for Alt+Shift+Enter (though it should be perfect if we'd go with Alt-click/Enter instead of Ctrl-click/Enter).

I suspect in the tab switch case we should not keep the view open ...
Unless it's being overridden.

Let's do that for now then; ctrl/alt/shift already override & whether middle-click should be added to _toggleActionOverride can be decided later.

@@ -760,5 @@

> > -    // Focus the content area before triggering loads, since if the load
> > -    // occurs in a new tab, we want focus to be restored to the content
> > -    // area when the current tab is re-selected.
> > -    browser.focus();

shouldn't we still focus() here in the default case?

I was actually on the fence about this. There isn't a need to wait until popuphidden to change focus in the default case. OTOH, since the default case closes the view, doing them all on popuphidden makes the code a bit simpler. If you're aware of some reason the focus change in the default case should occur before the view closes, or just prefer it that way, fine; I don't feel strongly either way.

Flags: needinfo?(mak77)

(In reply to custom.firefox.lady [:tawn] from comment #13)

Has canonize been taken into consideration in the decision to implement this? Ctrl-Enter will react differently on the first item in the result list than the remaining items. (Users triggering canonize will be sent somewhere other than the displayed result.) Might it be better to implement this as Alt-click/Enter (Alt-Enter already defaults to opening url in new tab & it would eliminate code to check for macosx) or restrict this bug to mouse-only?

That's a good question, this bug is a lot more complex than expected. The resulting matrix of possible outcomes becomes quite large, and I'm not so sure it's still manageable. I'm wondering if we shouldn't re-evaluate the actual need and request, if this is something many users want, or just a niche feature that we may decide to leave behind for too much complexity.
We don't even have a document for the interaction matrix, I can imagine it would be a nightmare for QA.

The bookmarks menus case was much simpler because there were less combinations.

Can we put this on the backburner for now? I feel like we should re-evaluate the whole interaction matrix first, we have too many keys doing the same thing and interacting each other, with canonization, switch-tab override, open in new tab, open in new background tab and open in new window all on shift/ctrl/alt modifiers and their combination.
At this point I wonder if the view should have a "sticky" feature instead of relying just on key combo.

Your suggestion to only do it for the mouse may also be ok, but we'd be left forever in an inconsistent state.

Flags: needinfo?(mak77)

(In reply to Marco Bonardo [::mak] from comment #14)

We don't even have a document for the interaction matrix, I can imagine it would be a nightmare for QA.
...
Can we put this on the backburner for now? I feel like we should re-evaluate the whole interaction matrix first, we have too many keys doing the same thing and interacting each other, with canonization, switch-tab override, open in new tab, open in new background tab and open in new window all on shift/ctrl/alt modifiers and their combination.

After giving this further thought, IMO it would be much simpler to implement "Leave the menu/view open when opening a new background tab if openInTabClosesMenu pref set to false" (as opposed to current bug title). So we can just do if (!openInTabClosesMenu && where == "tabshifted") eliminating need for OS checking and simply relying on preexisting 'where' code. Then nothing in that matrix changes except whether the view stays open.

Don't know if you or UX would be ok with that; we can postpone if you want.

From websites people are used to have tabs be opened in the background with their current context stay the same, if they open a link with a middle-click or a Ctrl/Cmd+click. This same behavior is now being implemented for bookmark menus and should also be the default behavior in the location bar.

The same behaviour should be true for the back / forward button & the reload button and the right-click menu of those buttons. Sometimes you want to open several tabs in the background without having to reopen the menu over and over again. The home button already has the behaviour.

Chrome has this feature on middle click and alt+shift+enter, so perhaps a parity-chrome flag should be added.

Also, as for the discussion on canonization: There's already a pref browser.urlbar.ctrlCanonizesURLs, why not just follow that?

See Also: → 1673588
See Also: → 1753863
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.