Closed Bug 1394304 Opened 7 years ago Closed 6 years ago

Support always open urlbar result in a new tab

Categories

(Firefox :: Address Bar, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox57 --- wontfix
firefox60 --- fixed

People

(Reporter: TYLin, Assigned: yurivkhan)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [specs in comment 83, tests required for landing])

Attachments

(4 files, 6 obsolete files)

2.16 KB, patch
adw
: review+
Details | Diff | Splinter Review
1.42 KB, patch
adw
: review+
Details | Diff | Splinter Review
2.61 KB, patch
adw
: review+
Details | Diff | Splinter Review
5.68 KB, patch
adw
: review+
Details | Diff | Splinter Review
I've been using Tab Mix Plus addon for one simple customization: always open urlbar results and search box results in a new tab. However, the addon is still not compatible for Firefox 57. 

Forutunally, we have pref "browser.search.openintab" for search box. I'd like to add a pref like "browser.urlbar.openintab" for url bar. I feel it might useful to other users as well as addon developers to add UI for it.
Note in bug 1123442 there was a proposal to change the urlbar to support middle-click to open in background/foreground. IMO this might be a better way of supporting something similar, rather than a pref that's unlikely to be discovered.

In any case, whatever functionality we end up with should get a test, so that it doesn't accidentally get broken in future.
I'll be more than happy to see bug 1123442 bringing keystroke consistency across all the UI components. My solution may be suboptimal, but it opens a door for customizations.
I'm OK with this idea I think, but I'd like to make sure Marco is too.
Flags: needinfo?(mak77)
Regarding background/foreground, I agree with https://bugzilla.mozilla.org/show_bug.cgi?id=1123442#c24 that browser.tabs.loadInBackground should be respected in general. But that's bug 1123442.

For this bug, I think the urlbar may want to just respect browser.search.openInTab. Since we are going towards unification, the users will find a consistent behavior when the search bar "goes away".
Thus, rather than adding a new pref, I'd just respect the search one.
Flags: needinfo?(mak77)
btw, I'm ok with the idea in general.
Comment on attachment 8901671 [details]
Bug 1394304 - Use pref to control whether to open url bar results in a new tab.

https://reviewboard.mozilla.org/r/173094/#review179758

Ting-Yu, could you update your patch to use the pref that Marco suggests?  Also, a couple of other changes, and then I should be able to r+ your next patch:

* We cache pref values in this binding: see the observe method, and how initial pref values are cached in the binding's constructor.  Personally I'm not so sure that it's a non-neglibible perf win, but we should follow convention here, so please do the same.

* Please cuddle the `else`, as is our style, i.e.:

> } else {
Attachment #8901671 - Flags: review?(adw)
I've updated my patch to address comment #8.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Comment on attachment 8901671 [details]
Bug 1394304 - Use pref to control whether to open url bar results in a new tab.

https://reviewboard.mozilla.org/r/173094/#review180194

Thanks!

::: browser/base/content/urlbarBindings.xml:78
(Diff revision 3)
>          this._defaultPrefs = Components.classes["@mozilla.org/preferences-service;1"]
>                                         .getService(Components.interfaces.nsIPrefService)
>                                         .getDefaultBranch("browser.urlbar.");
>  
> +        Services.prefs.addObserver("browser.search.openintab", this);
> +        this.browserSearchOpenintab = Services.prefs.getBoolPref("browser.search.openintab");

Nit: Please camel-case the "open in tab" part: browserSearchOpenInTab.
Attachment #8901671 - Flags: review?(adw) → review+
Comment on attachment 8901671 [details]
Bug 1394304 - Use pref to control whether to open url bar results in a new tab.

https://reviewboard.mozilla.org/r/173094/#review180194

> Nit: Please camel-case the "open in tab" part: browserSearchOpenInTab.

Fixed. Thanks for the review!
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7a2fb849cfa
Use pref to control whether to open url bar results in a new tab. r=adw
https://hg.mozilla.org/mozilla-central/rev/b7a2fb849cfa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1396072
Depends on: 1396059
Sorry for bugspam, but this new "feature" is really annoying and hasn't been fied with the latest update today:
I thought something was broken, when suddenly each time I _reused_ a tab by typing a URL and hitting enter yet a _new_ tab opened _at the end of all my tabs_ (so maybe 20 tabs away from where I just hit enter). Worse, even when I just focusing the URL in order to reload the tab and hit enter it duplicates the tab.
A new pref as YF suggests (bug 1396059) would be very desirable.
Aryx, could you please backout this patch and reopen the bug?

The first problem is that it doesn't work for mouse events, so it's half broken.
But mostly the idea itself doesn't seem to work well in the location bar that now has also taken over search capabilities. Indeed we should first re-evaluate the use-cases this is covering, since it's possible we may want to move differently, like having the pref only affect search results.
I must note that the workarounds exist, ALT+ENTER, middle-click, ctrl-click can open in new tabs in most cases.

It was my mistake to take this too lightly, in the end the change is very simple and off-hand seems to make sense, but the consequences are far more complex, and thus requires some deeper considerations.
Flags: needinfo?(aryx.bugmail)
I've backout my patch in https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb0997350fd. Sorry for causing the regressions.
Status: RESOLVED → REOPENED
Flags: needinfo?(aryx.bugmail)
Resolution: FIXED → ---
Assignee: tlin → nobody
Target Milestone: Firefox 57 → ---
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #18)
> Sorry for causing the regressions.

It's fine, don't worry. After all, Nightly is also about collecting feedback on changes.

For the future we need to figure out the right strategy, but for that we need to understand the use-case for users who enabled this feature, what are the common use-cases for you to always open the Location Bar results in a new tab?
(In reply to Marco Bonardo [::mak] from comment #19)
> For the future we need to figure out the right strategy, but for that we
> need to understand the use-case for users who enabled this feature, what are
> the common use-cases for you to always open the Location Bar results in a
> new tab?

I usually open everything in a new tab like results of url bar, search box, and bookmarks on the bookmarks toolbar. I could open an empty new tab (command+T) before open other things of course, but saving a keystroke is more convenient. Also, I have some pinned tabs like Gmail. If I'm browsing them and decide to open other sites, those pinned tabs won't be overridden.
(In reply to Marco Bonardo [::mak] from comment #19)
I often use the search bar for writing stuff to search for after reading, or while reading, which also means I want to keep the main page intact while doing the search.

If I am reading and searching for terms or things related to articles I am reading, it's really handy to be able to think of what to search by reading the article while writing the search query at the same time.

Searching in the main URL bar isn't ideal since I might want to copy the link later, or manually edit the link, assuming a search from there would open a new tab.
Assignee: nobody → tlin
I might not be the best person to continue working on this, so unassigned myself.
Assignee: tlin → nobody
I would really like to see the option for FF57, during the site development I very often use urlbar to navigate through newly developed sites just by tweaking URL and thanks to TabMix Plus it is opened in new tab. In FF57 I have to think about pressing Alt+Enter which makes work very annoying as I am using this function since it was available. Opening few hundreds of tabs every day and realizing I just did not press Alt before Enter and load new page over half filled form or debugged page is very frustrating.

Anyway many thanks for browser.tabs.loadBookmarksInTabs it made my live much easier and I can see the light at the end of the FF57 tunnel.
The ability to reuse a page could be needed to start a search as well as go to a URL, so the use case is the same.  Why have a preference for one but not the other?

Why not just invert the modifier: Use alt / middle click / option to open in the current tab when this preference is set?

Prior to FF 57 I pruned my addons to the ones essential for my workflow. Piro's extension to open URLBar links in tabs is the one thing I don't have a replacement for in FF 57.  

I can live with option-return as a crutch but find it annoying.
Blocks: 1226546
Seeing as there seems to be a discussion on whether this should be done, I'm going to chime in. If you browse the forums you will quickly realise that opening everything in a new tab(yes, even duplicating an existing tab by hitting enter on a loaded one) is by far the most direly missed feature of Tab Mix Plus, along with multi-row tabs, which is achievable via userchrome.css.

The ultimate reason for this is as follows: on one hand computers are more than powerful enough to not be crippeled by an extra tab or two and on the other, closing said tabs is alot more ergonomical than accidentally overwriting a tab then having to middle-click on back to bring that tab back, which also messes with tab order if one also has "browser.tabs.insertRelatedAfterCurrent" set to false.
This is the sort of issue that severely messes with one's workflow, which in my case, has been ongoing for more than 10 years.

A few considerations:
- if "browser.search.openintab" is set to true, clicking the search arrow in the search bar should have the same behavior as hitting enter and also open a new tab(should I report a bug?)
- if this moves forward, opening a new tab should be prevented if a "new tab" has focus.
- reversing the alt/ctrl modifiers seemns to me like the most sensible choice as per comment 25, but not middle-click, which should preserve behavior throughout the browser.
Hi all - I'm not a developer, but I've been involved with this feature for quite a while - pushing for a bug entry on Chrome which eventually led to a Won't Fix (which led to my abandonment of Chrome). I'm now using Waterfox because I frankly find Firefox (or Chrome) unusable without this feature.

Here is my input on this - my preferences are bizarre to those of you who hit Alt-Enter with no problem, just as your preferences to hit Alt-Enter are bizarre to me. It's all about muscle memory. So my comment upon reading all the comments above trying to develop a use case, is to try to mimic what Tab-Mix-Plus does with this, and leave it at that. Is it strange to hit enter on a current URL and have it load in a new tab? Sure, to you. But I never, ever, do that, and when I do that's the expected behavior so I don't think twice about it. Instead, I hit the refresh button when I want to reload. (I think TMP doesn't reload if it's an old version of a page that's loaded from, say, session restore).

My point is that the behavior you would be programming in an option will seem odd to you, but if you implement it like TMP, it will not seem odd at all to those who use TMP. (I realize that there will be issues with tab order, etc., to deal with).

Just my .02 from someone who has struggled with this issue for 6 or 7 years now. I hope it's helpful, and I appreciate your efforts.
I must add my voice to risches cryout.
If I type something in the search bar or URL bar and hit Enter, it's most likely that I'm going to do SOMETHING NEW, hence, open a NEW TAB as default action makes more sense than opening that request in currently focused tab.

Well, Firefox itself is inconsistent in this behavior. When you click any button from Menu (Options, Addons, etc...) it opens the corresponding page in a new tab completly ignoring how Open requests from other sources are handled and of course correctly ignoring how "browser.search.openintab" is set as it concerns search box.
For you who woud like to preserve reuse of current tab as default action without a preference allowing an override something to consider: should not these Menu actions then reuse focused tab as well? And when not, why would you force this behaviour to other similar actions like opening a new URL or search, or even something heavily discussed on other places as well - bookmarks?

You probably should along browser.search.openintab create and respect its alternatives - new browser.bookmarks.openintab and browser.url.openintab variables - or let the one be respected for all these actions and let Menu-clicks respect it too.
FYI: As a workaround I've updated my addon "New Tab from Location Bar" based on WebExtensions.
https://addons.mozilla.org/firefox/addon/new-tab-from-location-bar/
(In reply to YUKI "Piro" Hiroshi from comment #30)
> FYI: As a workaround I've updated my addon "New Tab from Location Bar" based
> on WebExtensions.
> https://addons.mozilla.org/firefox/addon/new-tab-from-location-bar/

I tried this and it works OK. However, it seems to reload the current tab first (which I suspect is an artifact of the problem Tab Mix Plus is having - that WebExtensions don't let you grab the contents of the urlbar BEFORE the load). It's passable for now and I'm trying out 57 now, but I'm wondering if an alternate solution to the builtin option would be to modify the WebExtension to allow for more functionality. Of course, there may be security reasons why we don't want addons to see and modify URLs before a site loads, but that's over my pay grade. Thanks to Piro for giving this a first shot!
I agree, it's a workaround. I was wondering how Piro managed to do this as the API is missing. It's the best we can have at the moment and it's not acceptable as a long term solution.

Kudos to Piro nonetheless!
Piro's webextension is using browser.webNavigation.onCommitted which is passed the "transitionType" value. This is good because you can see if the URL was typed, clicked, bookmark, etc. This is bad, because onCommitted is TOO LATE to stop the navigation from happening. All you can do is force the tab back to the previous page, and pop open the typed URL in a new tab. That's why anyone trying it out is seeing what looks like a reload.

The solution in my mind, as I pointed out here: bug 1393349 comment 1 would be to provide the transitionType to browser.webNavigation.onBeforeNavigate. Then you could STOP the navigation in progress and open a new tab for typed URLs.

But, like Piro, I've also been trying to find a workaround solution in the meantime, since this bug (and others) aren't getting a lot of attention right now. I have a webextension using browser.webRequest.onBeforeRequest that doesn't have the page reload problem, but does have problems with links from external sources and HTTP 30X redirects. It opens too many new tabs in those cases. I might try coordinating with Piro to combine our efforts.
(In reply to Joshua Cantara from comment #33)
> Piro's webextension is using browser.webNavigation.onCommitted which is
> passed the "transitionType" value. This is good because you can see if the
> URL was typed, clicked, bookmark, etc. This is bad, because onCommitted is
> TOO LATE to stop the navigation from happening. All you can do is force the
> tab back to the previous page, and pop open the typed URL in a new tab.
> That's why anyone trying it out is seeing what looks like a reload.
> 
> The solution in my mind, as I pointed out here: bug 1393349 comment 1 would
> be to provide the transitionType to browser.webNavigation.onBeforeNavigate.
> Then you could STOP the navigation in progress and open a new tab for typed
> URLs.
> 
> But, like Piro, I've also been trying to find a workaround solution in the
> meantime, since this bug (and others) aren't getting a lot of attention
> right now. I have a webextension using browser.webRequest.onBeforeRequest
> that doesn't have the page reload problem, but does have problems with links
> from external sources and HTTP 30X redirects. It opens too many new tabs in
> those cases. I might try coordinating with Piro to combine our efforts.

Please do! While Piro's method does save some time(the result is similar to middle clicking "back" after the unwanted navigation) , the main grievances with the lack of this feature still stand: you lose scroll position and form data. "browser.webRequest.onBeforeRequest" would be able to prevent this, correct?
Regardless, thank you Piro for your valuable effort. Your add-ons are awesome.
Blocks: 1420969
Blocks: 1311472
We plan to land bug 1420969 soon, which will expose the browser.search.openintab pref to developers via the WebExtensions API. This has been a highly requested API, but with the URL bar (awesomebar) acting as the default search bar after release 57, I do not think developers will get what they expect.

Reading through the comments, it appears that the patch did exactly what was intended - anything typed into the URL bar of an existing tab opens in a new tab.  If this is not the behavior we want, how do we get to that point? Once bug 1420969 lands, I expect this to gain a lot more visibility.
As I read the comments above (I didn't do a code review), the patch tied the browser.search.openintab pref to the urlbar/awesomebar, so people who set the pref expecting it to work only in search box also had it work in the main URL bar. The simple solution to this was to add a new pref browser.urlbar.openintab and make it behave like the pref (perhaps fixing mouse events in both). But instead it was backed out and dropped. I don't know how awesomebar as default search (I thought it always was, go figure) will affect that pref or how users see it- it seems to me that this is a much bigger code review question. I can say as an old user that I view them as separate but new users may not.
It's unclear what the users expectation is, is it to open anything from the urlbar into a new tab, or opening searches from the urlbar in a new tab? The 2 things are quite different and would be supported by different prefs.
Additionally the patch here had 2 problems:
1. reconfirming an already open page, just to reload it, was opening a duplicated tab
2. it was not working with mouse clicks

Personally I still think the right thing would be to obey the original browser.search.openintab pref rather than introduce a new one, but only for search results. That would also solve (1).
But we need some kind of confirmation that the expected outcome is opening only searches in new tabs.
(In reply to Marco Bonardo [::mak] from comment #37)
> It's unclear what the users expectation is, is it to open anything from the
> urlbar into a new tab, or opening searches from the urlbar in a new tab? The
> 2 things are quite different and would be supported by different prefs.
> Additionally the patch here had 2 problems:
> 1. reconfirming an already open page, just to reload it, was opening a
> duplicated tab
> 2. it was not working with mouse clicks
> 
> Personally I still think the right thing would be to obey the original
> browser.search.openintab pref rather than introduce a new one, but only for
> search results. That would also solve (1).
> But we need some kind of confirmation that the expected outcome is opening
> only searches in new tabs.

Searches in urlbar should respect browser.search.openintab and new URL needs to have separate pref, that is what is required, ideally it should allow to set if it should open only URL from different domain in new tab (this option was very handy it TabMix Plus addon) but even just simple all new url in urlbar will be better than nothing...
The fact is opening urls in a new tab by default sounds like the kind of thing that may cause a lot of misbehaviors and filed bugs. I'm not sure we want to enter that path lightly...
(In reply to Marco Bonardo [::mak] from comment #39)
> The fact is opening URLs in a new tab by default sounds like the kind of
> thing that may cause a lot of misbehaviors and filed bugs. I'm not sure we
> want to enter that path lightly...

That's why things should be kept simple. On the one hand, I don't believe that average users make that much of a distinction between searches and direct navigation from the URL bar in the age of Chrome and, on the other, it makes sense to me that an action on an element should always have the same result from a UX standpoint. So there should only be one about:config pref(off by default) that opens everything from both the URL bar and search bar in a new tab, except if an empty new tab has focus. Another thing to consider is that users who would enable said pref would very much rather have "false positives"(point 1 in comment 37) than lose their scroll position and form data by accidentally navigating from a tab. I speak both from personal experience and from a lot of discussions on forums with users that are missing TMP since Quantum.
At least provide the API to do it through an extension…
From the overall simplicity standpoint, I agree with comment 40: single preference controlling the search bar, searches from the address bar, and URLs in address bar. That conveniently matches my personal workflow.

However, I recognize there are many users who are accustomed to pressing Enter in the address bar for a reload. Maybe make that an exception (“if the current url is about:blank or about:newtab or the same as the new url, open in the same tab”).

Also, when the preference is set to open new tabs, Alt+Enter should override that to open in existing tab.
I think Firefox itself should implement a preference to open everything (searches, urls, suggestions, etc) in a new tab, but at the very least it should be possible to do with an add-on. Right now there's no clearly supported way to do this with the WebExtensions API.

Piro came up with a method, and I came up with one, and neither works without a lot of guessing and edge case handling. And, because we're using existing APIs in a novel way, there's no guarantee that a future change to Firefox won't break them.
(In reply to Marco Bonardo [::mak] from comment #37)
> It's unclear what the users expectation is, is it to open anything from the
> urlbar into a new tab, or opening searches from the urlbar in a new tab? The
> 2 things are quite different and would be supported by different prefs.
> Additionally the patch here had 2 problems:
> 1. reconfirming an already open page, just to reload it, was opening a
> duplicated tab
> 2. it was not working with mouse clicks
> 
> Personally I still think the right thing would be to obey the original
> browser.search.openintab pref rather than introduce a new one, but only for
> search results. That would also solve (1).
> But we need some kind of confirmation that the expected outcome is opening
> only searches in new tabs.

I third the single pref to open everything. It can be a different pref than the current search box pref. It probably should be a different pref. But if the question asked here is what the "users" want, you have to define who the user is. If it is a "normal" user, this pref will seem all broken, just like it does to you. 

But if it is users looking for a pref otherwise hidden in about:config, then it's probably somebody looking to replicate Tab-Mix-Plus, and that user expects EVERYTHING in the URL bar to open on a new tab. That is, problem 1 in the comment 37 quoted here was not a problem - it was behaving as expected! It is only a problem if you think that "reconfirming" an already open page is how it is supposed to work. But if you want this pref, you simply don't want that - that's what the refresh button is for.
(In reply to Joshua Cantara from comment #43)
> I think Firefox itself should implement a preference to open everything
> (searches, urls, suggestions, etc) in a new tab, but at the very least it
> should be possible to do with an add-on. Right now there's no clearly
> supported way to do this with the WebExtensions API.
> 
> Piro came up with a method, and I came up with one, and neither works
> without a lot of guessing and edge case handling. And, because we're using
> existing APIs in a novel way, there's no guarantee that a future change to
> Firefox won't break them.

Where is your extension?
(In reply to Geobert Quach from comment #45)
> Where is your extension?

I submitted my ideas to Piro, and he incorporated them in his add-on: https://addons.mozilla.org/en-US/firefox/addon/new-tab-from-location-bar/

In the options, set the "blocking method" to webRequest to try my method, set it to webNavigation to try Piro's. Neither is perfect due to the lack of an API method designed for this specific purpose.
We need product and UX guidance here. Most context is captured in comment 17, comment 35 and comment 37.
Flags: needinfo?(philipp)
Flags: needinfo?(jmoradi)
(In reply to Marco Bonardo [::mak] (Away 16 Dec - 2 Jan) from comment #37)
> It's unclear what the users expectation is, is it to open anything from the
> urlbar into a new tab, or opening searches from the urlbar in a new tab? The
> 2 things are quite different and would be supported by different prefs.
> Additionally the patch here had 2 problems:
> 1. reconfirming an already open page, just to reload it, was opening a
> duplicated tab
> 2. it was not working with mouse clicks

Perhaps clarity could be gained by looking at how the feature worked in Tab Mix Plus, since the people asking for this feature have generally referenced that past behavior as what they desired. To spell out what that option did:
1) I generally don't want to take up real estate for a search bar, so I don't care about that behavior
2) If I type in a search in the address bar and hit enter, open the results in a new tab.
3) If I type in a part of a website from my history and hit enter after firefox auto-completes it, it opens in a new tab.
4) If I click on the down arrow for address bar history and click on a link directly, open it in a new tab.

Notable exceptions:
1) If my current active tab is already a "new tab", then use it instead of opening another new one.
2) If I hit alt-enter in the address bar instead of enter, use the existing tab. (Possibly used to refresh current tab)
(In reply to ndemmon from comment #49)

Thank you for summarizing the expectations. (To be pedantic, there are also issues of clicking and middle-clicking the URL bar suggestions (seriously, who in the world does that?), anyway, they are for all intents and purposes equivalent to Enter and Alt+Enter, respectively.)

I have a working patch, based on the one in attachment 8901671 [details] by :TYLin, that implements this behavior. It is currently conditioned on the existing preference, browser.search.openintab. I will put it up for review if allowed. I can also modify it to use a different preference if that is deemed desirable.
Flags: needinfo?(philipp) → needinfo?(epang)
(In reply to Yuri Khan from comment #50)
> (In reply to ndemmon from comment #49)
> 
> Thank you for summarizing the expectations. (To be pedantic, there are also
> issues of clicking and middle-clicking the URL bar suggestions (seriously,
> who in the world does that?), anyway, they are for all intents and purposes
> equivalent to Enter and Alt+Enter, respectively.)
> 
> I have a working patch, based on the one in attachment 8901671 [details] by
> :TYLin, that implements this behavior. It is currently conditioned on the
> existing preference, browser.search.openintab. I will put it up for review
> if allowed. I can also modify it to use a different preference if that is
> deemed desirable.

Hi Yuri, would it be possible to have a build with the patch in it to try?
Flags: needinfo?(epang) → needinfo?(yurivkhan)
(In reply to Eric Pang [:epang] UX from comment #51)

> Hi Yuri, would it be possible to have a build with the patch in it to try?

I have an Ubuntu PPA at https://launchpad.net/~yurivkhan/+archive/ubuntu/fireforks that contains an Ubuntu 16.04 build, based on the one from https://launchpad.net/~mozillateam/+archive/ubuntu/firefox-next + this patch + a few patches unrelated to the URL bar. Will that be okay for you?

If absolutely needed, I could probably arrange a build from mozilla-central + only this patch, also for Ubuntu 16.04. Other Ubuntu versions are possible but complicated. I do not have technical capability to build for Windows or macOS.
Flags: needinfo?(yurivkhan) → needinfo?(epang)
I also agree that a single pref makes sense here.
But I would like to test the functionality when ready.

@yuri Unfortunately I won't be able to run the ubuntu build since I'm on macOS
when your patch is ready can you push to try? This should create a mac build I can test.

If you can NI me when ready that would be great! Thanks!
@Eric I filed bug 1428981 for Level 1 commit access, but I will need someone with Level 2 or higher to vouch for me.
(In reply to Yuri Khan from comment #54)
> @Eric I filed bug 1428981 for Level 1 commit access, but I will need someone
> with Level 2 or higher to vouch for me.

Thanks Yuri!  Panos, is this something you can help with?
Flags: needinfo?(epang) → needinfo?(past)
Done, thanks Yuri!
Flags: needinfo?(past)
Okay, it looks like the build is ready.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=767f49ffed24019ed9b5980a14c66085ba9e3bc7

To see the requested behavior, set the browser.search.openintab pref to true.
Flags: needinfo?(epang)
(In reply to Yuri Khan from comment #57)
> Okay, it looks like the build is ready.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=767f49ffed24019ed9b5980a14c66085ba9e3bc7
> 
> To see the requested behavior, set the browser.search.openintab pref to true.


Thanks Panos and Yuri!

Just tried out the patch.

From a UX perspective I feel this works great since it brings back the desired functionality from Tab Mix Plus based on comment 49 from ndemmon (which seems to be the original bug in comment 1) 
 
>1) I generally don't want to take up real estate for a search bar, so I don't care about that behavior
>2) If I type in a search in the address bar and hit enter, open the results in a new tab.
>3) If I type in a part of a website from my history and hit enter after firefox auto-completes it, it opens in a new tab.
>4) If I click on the down arrow for address bar history and click on a link directly, open it in a new tab.

Also, being behind a pref keeps it from being risky to land while giving users a way to get the functionality back.

I'm on the fence about how the middle click no longer opens the page on a new tab.  In one sense this makes sense since the functionality is reversed.  What was done on Tab Mix Plus? - we should probably follow what was done in the addon.

Javaun, what do you think?
Flags: needinfo?(epang)
Thank you all for the hard work on this. With respect to middle-click, Tab Mix Plus includes functionality to assign tasks to middle click. Popular options are undo close tab and duplicate tab. My view is that this change should leave middle-click unchanged from whatever the published functionality is. TMP users won't be expecting it to do something different if they select this pref.
(In reply to Eric Pang [:epang] UX from comment #58)

> Also, being behind a pref keeps it from being risky to land

One possible concern is that this same pref is used by the search box. A user who has toggled it to get new tabs from the search box might be surprised and/or annoyed when it starts affecting the URL bar, too. See for example comment #16 right here in this bug.

How should we deal with this so that the patch does not get backed out again?


> I'm on the fence about how the middle click no longer opens the page on a
> new tab.  In one sense this makes sense since the functionality is reversed.
> What was done on Tab Mix Plus? - we should probably follow what was done in
> the addon.

In Tab Mix Plus, there was a group of checkboxes titled “Open tabs from:” with individual checkboxes:

 [?] Bookmarks
 [?] History
 [x] Address bar
 [x] Search bar
 [?] Synced Tabs
 [?] Groups of bookmarks/history
     Don’t override tabs when opening a group of bookmarks/history

which controlled what happens on a normal click or Enter, and then a single separate checkbox:

 [x] Middle-click or Control-click opens items in current tab

which did what it said, for all cases selected above. In my variation of the patch, I attempted to implement the behavior as if this latter checkbox was checked (because it makes sense to me to not waste a UI gesture).

However, I tested it now on Firefox Nightly 56.0a1 and TMP 0.5.0.4 [latest available from AMO], with [x] Address bar and [x] Middle-click or Control-click opens items in current tab, and it seems it did not affect the address bar drop-down after all. From TMP perspective, it could be considered a minor bug/omission.

In the scope of the address bar, I am okay with either behavior of the mouse, because I do not click on address bar dropdown items, I select them with arrow keys and Enter.

As soon as a UX decision is made, I will update the patch to match it and put it up for code review.
Thanks Yuri for the extra clarification. It really helps to understand how Tab Mix Plus worked (since I didn't use it myself).

From a UX stand point it makes sense to have all search points behave in the same way.
But IMO this bug seems to err more on the side of personal customization and giving TMP users a way to recreate their experience.
We also need understand that not all users will want the experience in both the search and URL bar. 

With that in mind creating a separate pref "browser.urlbar.openintab" allows for this and let's the user customize this in the way that works for them. This allows users get to the experience they want with less chance of bug filing from unexpected changes. This should keep this from being backed out since each individual pref will act as expected.

A Few updates needed:
1. Don’t duplicate page on enter in search bar (keep as refresh)
2. Keep Middle click the way it is (open in a new tab)

I'm aware there's some concern about opening a new pref.  
Marco, would you be okay with this?


(In reply to Yuri Khan from comment #60)
> (In reply to Eric Pang [:epang] UX from comment #58)
> 
> > Also, being behind a pref keeps it from being risky to land
> 
> One possible concern is that this same pref is used by the search box. A
> user who has toggled it to get new tabs from the search box might be
> surprised and/or annoyed when it starts affecting the URL bar, too. See for
> example comment #16 right here in this bug.
> 
> How should we deal with this so that the patch does not get backed out again?
> 
> 
> > I'm on the fence about how the middle click no longer opens the page on a
> > new tab.  In one sense this makes sense since the functionality is reversed.
> > What was done on Tab Mix Plus? - we should probably follow what was done in
> > the addon.
> 
> In Tab Mix Plus, there was a group of checkboxes titled “Open tabs from:”
> with individual checkboxes:
> 
>  [?] Bookmarks
>  [?] History
>  [x] Address bar
>  [x] Search bar
>  [?] Synced Tabs
>  [?] Groups of bookmarks/history
>      Don’t override tabs when opening a group of bookmarks/history
> 
> which controlled what happens on a normal click or Enter, and then a single
> separate checkbox:
> 
>  [x] Middle-click or Control-click opens items in current tab
> 
> which did what it said, for all cases selected above. In my variation of the
> patch, I attempted to implement the behavior as if this latter checkbox was
> checked (because it makes sense to me to not waste a UI gesture).
> 
> However, I tested it now on Firefox Nightly 56.0a1 and TMP 0.5.0.4 [latest
> available from AMO], with [x] Address bar and [x] Middle-click or
> Control-click opens items in current tab, and it seems it did not affect the
> address bar drop-down after all. From TMP perspective, it could be
> considered a minor bug/omission.
> 
> In the scope of the address bar, I am okay with either behavior of the
> mouse, because I do not click on address bar dropdown items, I select them
> with arrow keys and Enter.
> 
> As soon as a UX decision is made, I will update the patch to match it and
> put it up for code review.
Flags: needinfo?(mak77)
(In reply to Eric Pang [:epang] UX from comment #61)
> I'm aware there's some concern about opening a new pref.  
> Marco, would you be okay with this?

TL;DR: This would end up being the n-th undiscoverable pref, while proper mouse/keyboard support for opening in background tabs would be discoverable and not much additional work for the user. Eventually we could support browser.search.openintab ONLY for search suggestions in the unified urlbar.

My concerns are:
1. We are about to introduce a new undiscoverable pref without an idea about its usage numbers (number of TMP users is not relevant, because everyone used different sub features of it)
2. The use case for search suggestions is a little bit clearer, you can open a search suggestion result page in a new tab, you are making a search, you don't know exactly what you are looking for. The use-case for urls is not so clear.
3. When merging the address bar and the search bar (that is what we are trying to do), I can see users confused about browser.searchbar.openintab not working anymore, that's why I was suggesting to support it only on search suggestions in the unified urlbar, rather than requiring users to discover that there's another new pref.
4. It's already possible to open results in a new tab using middle-click or ALT+ENTER), the problems there are bug 1389739 (altGR should work) and bug 1123442 (we should provide a way to open those tab in background). I still fail to see a clear use-case for which that additional burden (pressing ALT) is so problematic.
Flags: needinfo?(mak77)
Flags: needinfo?(jmoradi)
To sum up further, my opinion is that we should fix the 2 bugs making ALT+ENTER and middle click problematic, and support browser.search.openintab only for search suggestions in the unified urlbar.
(In reply to Marco Bonardo [::mak] from comment #62)

> 1. We are about to introduce a new undiscoverable pref without an idea about
> its usage numbers (number of TMP users is not relevant, because everyone
> used different sub features of it)

This is by far the most requested feature in the forums missing since TMP got unsupported, and pretty much a common denominator for TMP users along with multi row tabs.

> 2. The use case for search suggestions is a little bit clearer, you can open
> a search suggestion result page in a new tab, you are making a search, you
> don't know exactly what you are looking for. The use-case for urls is not so
> clear.

Wheter search or new url, it's way easier to close a redundant tab than to take steps to end up with both the original tab and the new one. And then there's losing the scroll position and form data in the original tab.

> 3. When merging the address bar and the search bar (that is what we are
> trying to do), I can see users confused about browser.searchbar.openintab
> not working anymore, that's why I was suggesting to support it only on
> search suggestions in the unified urlbar, rather than requiring users to
> discover that there's another new pref.

Your power users are not really excited about FF becoming a Chrome clone. I sincerly hope you NEVER get rid of the search bar. And, btw, whoever is interested in this will have no difficulty whatsoever in discovering the new pref, which, btw, should also be exposed for webextensions a la TMP. 

> 4. It's already possible to open results in a new tab using middle-click or
> ALT+ENTER), the problems there are bug 1389739 (altGR should work) and bug
> 1123442 (we should provide a way to open those tab in background). I still
> fail to see a clear use-case for which that additional burden (pressing ALT)
> is so problematic.

A clear use case: one browses with mouse only with keyboard tucked away and middle-click is way less ergonomic.
Absolutelly agree with comment 64

The answer for "I still fail to see a clear use-case for which that additional burden ..." is very simple we were using it for many years and now we need to start thinking about one more key press to do action we are doing tousand of times a day. It is just the biggest pain in the ass of my daily use of FF57 & dev.

You are so excited to get new users abandoning IE or other browsers but you are for long time ignoring your loyal users, is it good way for the future?
Please refrain from advocacy comments, we're discussing a technical issue here.
You didn't reply to my points, "I'm used to do X instead of X+1" is not a good argument, we can get used to new things. The problem is understanding why the new things are not good and how we can improve them.

I didn't yet see a counter-example of why my suggestion in comment 63 would not cover most of your needs.

(In reply to Hlsg from comment #64)
> This is by far the most requested feature in the forums missing since TMP
> got unsupported

How many unique users to put this into perspective? 10, 100, 1000?

> Wheter search or new url, it's way easier to close a redundant tab than to
> take steps to end up with both the original tab and the new one. And then
> there's losing the scroll position and form data in the original tab.

Which steps, you means pressing middle button instead of left button?
Doesn't Back restore the scroll position?

> middle-click is way less ergonomic.

How? CTRL+click is another alternative. The good thing is that most of the handling is shared across all the UI, very discoverable.
> Frankly I fail to see the logic in the default behavior. If I type some text to a search bar and simply hit ENTER it's 99% I'm doing something new, so to open a new tab for it seems logical, the same goes for a typed URL or an opened link from bookmarks.

Frankly I fail to see *your* logic. The address bar is connected with the *current tab* as also the design implies. So - for me - it's clear and without any doubt that I want to open a URL in the *current tab. If I want to open a URL in a new tab I open a new tab.
While I have plenty of advocacy comments, let me address the core substantive comments posed (with admittedly a tad of advocacy):
1. There are 450,000 TMP users. Yes, they use different features, but probably 80% use this functionality, if the requests on the TMP forums are any indication.  That's 350,000 users. And they really, really care about this, even if you do not. When Chrome cut off TMP, I stopped using Chrome. When FF cut it off, I started using Waterfox. This is the one and only bug I have ever commented on, and that's even with my FF freezing on my Surface Tab since v.42 and no fix in sight. I would rather live with that than the Alt-Enter behavior. It is only because of the kludgy workaround that I'm using FF57 at all. Yes, there are alternative ways, but a switch is jarring; as someone else asked, how would you react if we suddenly switched functionality and gave you no option. I suspect you would consider it a poor UX. Sure, you can get used to something new, but if I have to do that, why not just get used to a new browser?

2,3. If you are trying to merge search bar and urltab, then what will you be telling all the people who rely on the current searchbar pref who want to enter searches in the URL? It makes sense that if you want the searchbar to merge into URLbar, that one of the key prefs of searchbar should carry over - whatever you type in opens in a new tab. If you are worried about confusion from this simple pref, and can't imagine the confusion you'll see if only search suggestions (which are turned off for me except history, by the way) open in a new tab.

4. Use case: I type on my surface tablet, using the onscreen keyboard. Clicking alt-enter to open in new tab is extra work that I don't need. Second use case: if I forget, I lose my place if I was in a form or other page that doesn't put me back where I was. I used to accidentally close documents a few times a year because Ctrl-W changed from "insert symbol" in Word Perfect to "close document" in Word - despite not having used WordPerfect in years. I fixed it by remapping Ctrl-W because Microsoft allowed me to decide what was best for me, rather than a developer deciding what was best for me.

TL;DR: if it is hidden prefs you are worried about, then expose it! Prefs are only hidden because of this new view that somehow prefs pages have to be so simple that people have no options. If this were exposed (and searchbar was exposed) then you would give users a way to use the product in a way that best suits them.

I agree with one comment above - these are not technical issues: the fix is built. These are UX design issues that developers have to sort through. Unlike the other commentators, I won't deride that - you can build how you best want to build. But I will tell you this: there is a fine line between too much and too little choice for the users, and Firefox has always been best because of its wealth of choice. WebExtensions radically tilted the tables the other way, and statements that users should just get used having no choice, to me, threaten the very soul of Firefox. This browser should work for me, I shouldn't have to work for the browser.
(In reply to Marco Bonardo [::mak] from comment #62)

> 4. It's already possible to open results in a new tab using middle-click or
> ALT+ENTER) […] I still
> fail to see a clear use-case for which that additional burden (pressing ALT)
> is so problematic.

The use case is part of an overarching user story, that is, for me and many former users of TMP:

    Tabs are great and memory is plentiful. Every time that is not a clear-cut
    navigation between two closely related pages, let’s open a new tab.

This gives a tangible benefit:

+ Every page you have not explicitly released stays open. If/when you need it again, it’s a single tab switch away.

As opposed to:

− When you need a page you previously visited, you have to (1) find the tab where it was, (2) find it in that tab’s history, and potentially (3) wait for a new network fetch if that site has an aggressive cache expiration policy. If the tab was closed, then (2') find the page in the global history and (3') wait for a new network fetch.


The burden is not in holding Alt. The burden is in having to remember to press Alt, always, or risk destroying context.

Imagine working in a code editor where opening a file takes anywhere from 500 to 15000 milliseconds. Imagine also that every time you click to open another file, it will close the previous file you were working on and forget your position in it, unless you remember to hold down Alt. That is how the current Firefox feels to us.


(In reply to Sören Hentzschel from comment #69)

> Frankly I fail to see *your* logic. The address bar is connected with the
> *current tab* as also the design implies.

That is a relatively recent UI change. It dates back to Firefox 3.7 or maybe 4.0. Before that, the URL bar was above/outside the tabs. Naturally, with the “open-everything-in-tabs” workflow, the URL bar gets moved back there, to preserve the logical control hierarchy. TMP provided that, too; for now we achieve it with userChrome.css.

> If I want to open a URL in a new tab I open a new tab.

I briefly tried doing that. You’ll be amused: I started getting the doorway syndrome. You know, when you pass through a doorway and forget what you were going to do? That’s the feeling I get when confronted with an empty new tab.


(In reply to Marco Bonardo [::mak] from comment #63)
> To sum up further, my opinion is that we should fix the 2 bugs making
> ALT+ENTER and middle click problematic, and support browser.search.openintab
> only for search suggestions in the unified urlbar.

I want to make it clear: My need, as scoped to this bug, is to open all URL bar results in new tabs, without having to hold down Alt or reach for the mouse. I have a patch that solves my need, at the cost of having to compile my own builds of Firefox. I would like to turn this patch into a form that has reasonable survival chances in the upstream Firefox codebase, but only as long as it still solves my need.
(In reply to Marco Bonardo [::mak] from comment #66)

> (In reply to Hlsg from comment #64)
> > This is by far the most requested feature in the forums missing since TMP
> > got unsupported
> 
> How many unique users to put this into perspective? 10, 100, 1000?
> 

Not much to add to what Yuri said, as he is 100% on point, except that the number of users wanting this is without a doubt considerably over 100k. This extension(which is actually an ugly hack to almost achieve this) alone has almost 5k users and most of the TMP users on the forums have no idea about it: https://addons.mozilla.org/en-US/firefox/addon/new-tab-from-location-bar/
Thanks for constructive comments.
For those asking why is a problem to add a simple pref, you must understand that a new pref has ALWAYS costs: a future code maintenance cost, a QE cost and a support cost (because a. users forget they set a pref and then think things are broken and b. The browser is tested in default conditions, a mixture of prefs can cause new unexpected bugs that we'll have to fix).
That means before taking ANY pref, we must do an analysis of the cost/benefit ratio.

Looking through the TMP forums I see a lot of other requests among which "have a whitelist of pages that should NOT open in a new tab"... that somehow seems to point out the fact some TMP users are unhappy with the default "open ANY url in a new tab" behavior. This is another reason why we should understand exactly WHAT would satisfy the most.
One could come and say "if everything opens in a new tab, then ALT+ENTER or middle click should open in the current one".
As things stand, we'd wontfix all of these requests. The risk is having something that on the paper should have satisfied users, but since the original implementation was already incomplete, our porting will only be good for a tiny minority.
I want to repeat we'll never be able to satisfy everyone, period.

> 2,3. If you are trying to merge search bar and urltab, then what will you be
> telling all the people who rely on the current searchbar pref who want to
> enter searches in the URL? It makes sense that if you want the searchbar to
> merge into URLbar, that one of the key prefs of searchbar should carry over

That's exactly what I'm saying, browser.search.openintab could carry over to the urlbar search entries. No need for the user to search what's the new magic hidden pref.

> - whatever you type in opens in a new tab. If you are worried about
> confusion from this simple pref, and can't imagine the confusion you'll see
> if only search suggestions (which are turned off for me except history, by
> the way) open in a new tab.

Those users set browser.search.openintab, that's what they asked for (Note the pref says "search", not "searchbar"), and it should only act if the searchbar has been removed. 

> 4. Use case: I type on my surface tablet, using the onscreen keyboard.

This is a valid use-case, thank you for pointing it out.
For tablet/touch users indeed it's not really handy to use mouse nor a key combo, they could indeed benefit from the new pref.
On the other side, we are restricting to a minority of a minority now :(

(In reply to Yuri Khan from comment #73)
> The burden is not in holding Alt. The burden is in having to remember to
> press Alt, always, or risk destroying context.

This is a possibly valid concern that would not be covered by browser.search.openintab

> > (In reply to Hlsg from comment #64)
> This extension(which is actually an ugly hack to almost achieve this) alone has
> almost 5k users and most of the TMP users on the forums have no idea about
> it: https://addons.mozilla.org/en-US/firefox/addon/new-tab-from-location-bar/

Is this working as a temporary solution? From the previous comments looks like many don't know about this add-on.
I wonder if Piro filed a bug to get a WebExt API to do that job in a less hacky way, he surely know where he needs to hook, and then he could easily implement filtering, modifiers and anything else.

I'll leave the final decision to Epang, he has all the details to make such decision (he already made one, but asked me for further concerns that I now expressed).

option 1: add the new browser.urlbar.openintab pref. This will open everything in a new tab. We'll have to test various edge cases like switch to tab to ensure they don't break. It won't be tested by default, so it could break with new stuff we add to the urlbar in the future. We'll never add whitelist or filters, it will be an "all or nothing". It won't reuse empty tabs, it won't "link" tabs. In this regard Piro's add-on seems to do a better job, our implementation will always look incomplete.

option 2: reuse browser.search.openintab, if the searchbar is removed (unified locationbar) search suggestions from the location bar will open in a new tab, otherwise things will work as usual. This would be completely in line with the current pref. It wouldn't cover the "use firefox as a multi-tab editor" case.

options 3: wontfix and be sorry. We should though work with Piro to understand which kind of WebExt API he'd need to avoid hackish ways to implement this.
Ah, found him.
Piro, could you please tell us if you filed any bug to get better WebExt APIs to support your New Tab from Location Bar add-on?
Do you have a possible idea of how it should look like?
Would adding a pref to always open results in a new tab help your add-on or would it still need to hook the same way to provide the other features?
Flags: needinfo?(yuki)
(In reply to Marco Bonardo [::mak] from comment #75)
> I wonder if Piro filed a bug to get a WebExt API to do that job in a less
> hacky way, he surely know where he needs to hook, and then he could easily
> implement filtering, modifiers and anything else.

I did https://bugzilla.mozilla.org/show_bug.cgi?id=1393349 which has been closed as duplicate of this one.
Ops! Honestly it was a bit generic and as such the dupe can stand.  Having an add-on developer insight and a proposal would go a longer way in a WebExt bug report.
(In reply to Marco Bonardo [::mak] from comment #75)

> One could come and say "if everything opens in a new tab, then ALT+ENTER or
> middle click should open in the current one".

<waves hand> That’s me!

> option 1: add the new browser.urlbar.openintab pref. This will open
> everything in a new tab. […]
> It won't reuse empty tabs, it won't "link" tabs.

Why not? In its current state, it does and it does.

> We should though work with Piro to understand which kind of WebExt API he'd need to avoid hackish ways to implement this.

I think the extension would need to know the kind of thing being navigated to (URL, search query, open tab), the UI gesture (key, modifiers, mouse button), and be able to either perform the actual navigation, or return details telling the browser how to do it (same tab, other existing tab (tabId), new tab left of current, right of current, at the end of related tabs group, at the left end, at the right end, new tab in another window (windowId, position), new window). There would need to be some safeguard against an extension taking too long to reply, and possibly a new permission.

The same general structure could also solve the related issue with bookmarks and history. Applicability to more than one issue is a general indication of non-hackishness, right?
(In reply to Marco Bonardo [::mak] from comment #75)
> Thanks for constructive comments.

A few hopefully constructive comments.

> > 4. Use case: I type on my surface tablet, using the onscreen keyboard.
> 
> This is a valid use-case, thank you for pointing it out.
> For tablet/touch users indeed it's not really handy to use mouse nor a key
> combo, they could indeed benefit from the new pref.
> On the other side, we are restricting to a minority of a minority now :(

So the current Windows 10 precision touchpad driver took away three finger tap middle click as a configuration option. So you can add "most laptops" to make your minority bigger. And then you add in "people using **** mice from the manufacturer where to do a middle click you have to push down on the wheel without spinning it". And as for "alt-click", I hope you understand that things that require two hands to work in parallel are more challenging for people for a variety of issues (obvious examples: one-handed people, people who have suffered strokes, Parkinsons, etc) than actions only requiring one hand.

> > it: https://addons.mozilla.org/en-US/firefox/addon/new-tab-from-location-bar/
> 
> Is this working as a temporary solution? 

It's about 80%. It refreshes the page you're leaving, which is usually ok, but it doesn't work when you're on a website that pops up a "confirm that you're leaving" dialogue box (example, www.brigebase.com/client).

> options 3: wontfix and be sorry. We should though work with Piro to
> understand which kind of WebExt API he'd need to avoid hackish ways to
> implement this.

So, there is an option 4 here, and it's sort of the elephant in the room: Work with TMP to extend the WebExt API to provide the functionality to fully implement TMP in WebExt. It was a widely popular extension, to the point where you can see cracks forming in your userbase as people migrate to waterfox and other variants that still support it or (more frighteningly) refuse to update their browser so they won't lose that functionality.
I understand the reasoning behind not wanting yet another undiscoverable pref, but there's already one for bookmarks, and already one for searches. For consistency you should complete the set, or remove them all and provide the API support necessary for addon authors to provide that functionality.

I think a possible (maybe even simple?) API fix for this would be to provide the transitionType to browser.webNavigation.onBeforeNavigate. Then you could stop the navigation in progress (leaving the contents of the current tab unchanged) then open a new tab for the typed URL.
>This would end up being the n-th undiscoverable pref, while proper mouse/keyboard support for opening in background tabs would >be discoverable and not much additional work for the user. 

I agree that the pref will be difficult to discover - but believe the users that are looking for it will find it.
Also, we can find outlets to spread word of the the pref (sumo, reddit, twitter, etc.)

>2. The use case for search suggestions is a little bit clearer, you can open a search suggestion result page in a new tab, you >are making a search, you don't know exactly what you are looking for. The use-case for urls is not so clear.

Having only the search suggestions open in a new tab feels like an odd experience to me.  My assumption is that most users will expect all items in the url search dropdown to behave in the same way.

>4. It's already possible to open results in a new tab using middle-click or ALT+ENTER), the problems there are bug 1389739 >(altGR should work) and bug 1123442 (we should provide a way to open those tab in background). I still fail to see a clear >use-case for which that additional burden (pressing ALT) is so problematic.

At first I also thought “what’s the big deal?” (seeing as I’m used to the default behavior)… But putting myself into the place of a user that’s been using the browser in a specific way for a while (long enough to create muscle memory) I can see why this would become frustrating.  

>For those asking why is a problem to add a simple pref, you must understand that a new pref has ALWAYS costs: a future code >maintenance cost, a QE cost and a support cost (because a. users forget they set a pref and then think things are broken and b. >The browser is tested in default conditions, a mixture of prefs can cause new unexpected bugs that we'll have to fix).
>That means before taking ANY pref, we must do an analysis of the cost/benefit ratio.

This is a very valid point. IMO the cost is low for the amount of interest (as commented by Yuri) there is for this functionality. Though I see this more as a temporary fix.

In the future I would love to see this in our tab preferences so that users can customize their own experience in the browser.

Something like this (and not limited to):

When you click/hit enter on a search item it opens…
…in the current tab
…in a new tab next to the current tab
…in a new tab to the right of tabs
…in a new window

Thanks everyone for their interest in this bug, for now I suggest we move forward with Option 1 from Marco. 
> option 1: add the new browser.urlbar.openintab pref. 

Here's the expected behavior

- All URL bar search list items (excluding currently opened tabs) open in new tab to the right of existing tabs  
     - On click
     - On enter
- URL bar search list items open in current tab
     - On middle click
     - On control/command click
- Currently open tabs - Switches to tab
- Enter in url bar of current tab refreshes (don’t duplicate)
> So, there is an option 4 here, and it's sort of the elephant in the room:
> Work with TMP to extend the WebExt API to provide the functionality to fully
> implement TMP in WebExt. It was a widely popular extension, to the point
> where you can see cracks forming in your userbase as people migrate to
> waterfox and other variants that still support it or (more frighteningly)
> refuse to update their browser so they won't lose that functionality.

Has anyone reached out to the developes of TMP?  I can look into this if no one has - it's worth a discussion at least.
(In reply to Eric Pang [:epang] UX from comment #83)

> Thanks everyone for their interest in this bug, for now I suggest we move
> forward with Option 1 from Marco. 
> > option 1: add the new browser.urlbar.openintab pref. 
> 
> Here's the expected behavior

I’d like the whole matrix of possibilities specified. Let me know if I got this right:

|          | Enter or left click | Alt+Enter or middle click |
|----------|---------------------|---------------------------|
| URLs     | new tab*†           | current tab               |
| searches | new tab†            | current tab               |
| tabs     | switch to tab       | unavailable‡              |

*) except if the full URL (including #hash) exactly matches the URL in the current tab, in which case load it in current tab

†) position of new tab is governed by (currently non-existent) user preferences for new tabs, so at the end of the tab bar for the moment

‡) holding down Alt causes the dropdown to treat open tabs as URLs so it will be loaded in current tab
(In reply to Yuri Khan from comment #85)
> (In reply to Eric Pang [:epang] UX from comment #83)
> 
> > Thanks everyone for their interest in this bug, for now I suggest we move
> > forward with Option 1 from Marco. 
> > > option 1: add the new browser.urlbar.openintab pref. 
> > 
> > Here's the expected behavior
> 
> I’d like the whole matrix of possibilities specified. Let me know if I got
> this right:
> 
> |          | Enter or left click | Alt+Enter or middle click |
> |----------|---------------------|---------------------------|
> | URLs     | new tab*†           | current tab               |
> | searches | new tab†            | current tab               |
> | tabs     | switch to tab       | unavailable‡              |
> 
> *) except if the full URL (including #hash) exactly matches the URL in the
> current tab, in which case load it in current tab
> 
> †) position of new tab is governed by (currently non-existent) user
> preferences for new tabs, so at the end of the tab bar for the moment
> 
> ‡) holding down Alt causes the dropdown to treat open tabs as URLs so it
> will be loaded in current tab

Thanks for the breakdown, this looks good to me.
(In reply to Yuri Khan from comment #85)
> (In reply to Eric Pang [:epang] UX from comment #83)
> 
> > Thanks everyone for their interest in this bug, for now I suggest we move
> > forward with Option 1 from Marco. 
> > > option 1: add the new browser.urlbar.openintab pref. 
> > 
> > Here's the expected behavior
> 
> I’d like the whole matrix of possibilities specified. Let me know if I got
> this right:
> 
> |          | Enter or left click | Alt+Enter or middle click |
> |----------|---------------------|---------------------------|
> | URLs     | new tab*†           | current tab               |
> | searches | new tab†            | current tab               |
> | tabs     | switch to tab       | unavailable‡              |
> 
> *) except if the full URL (including #hash) exactly matches the URL in the
> current tab, in which case load it in current tab
> 
> †) position of new tab is governed by (currently non-existent) user
> preferences for new tabs, so at the end of the tab bar for the moment
> 
> ‡) holding down Alt causes the dropdown to treat open tabs as URLs so it
> will be loaded in current tab

Again Yuri, this is 100% on point.

(In reply to Eric Pang [:epang] UX from comment #84)
> > So, there is an option 4 here, and it's sort of the elephant in the room:
> > Work with TMP to extend the WebExt API to provide the functionality to fully
> > implement TMP in WebExt. It was a widely popular extension, to the point
> > where you can see cracks forming in your userbase as people migrate to
> > waterfox and other variants that still support it or (more frighteningly)
> > refuse to update their browser so they won't lose that functionality.
> 
> Has anyone reached out to the developes of TMP?  I can look into this if no
> one has - it's worth a discussion at least.

That would be a good idea indeed. The developer is on bugzilla: tabmix.onemen@gmail.com
(In reply to Marco Bonardo [::mak] from comment #77)
> Ah, found him.
> Piro, could you please tell us if you filed any bug to get better WebExt
> APIs to support your New Tab from Location Bar add-on?
> Do you have a possible idea of how it should look like?
> Would adding a pref to always open results in a new tab help your add-on or
> would it still need to hook the same way to provide the other features?

I didn't report any bug around this topic except the bug 1271945.

Through the experience of updating my addon "New Tab from Location Bar" [1], I've realized that exposing of "transitionType" [2] to "webRequest.onBeforeRequest" [3] will be an enough effective change for this problem.

[1]https://addons.mozilla.org/firefox/addon/new-tab-from-location-bar/
[2]https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation/transitionType
[3]https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest

I've not read all of comments on this bug but this topic seems too complex: everyone say different thing about what is the expected behavior. In my opinion, a new preference "browser.urlbar.openintab" similar to "browser.search.openintab" should introduce very straight result: "always open results in new tab, except non-loading action like switching to existing tab". And if you need more intelligent decisions for various cases, they should be done by addons like above based on "transitionType" exposed to "webRequest.onBeforeRequest" (and/or other hints). If you need to implement extra result for cases like Alt-Enter, then something boolean information like "accelerated" or "modified" will be needed at "webRequest.onBeforeRequest".
Flags: needinfo?(yuki)
(In reply to Yuri Khan from comment #85)

> *) except if the full URL (including #hash) exactly matches the URL in the
> current tab, in which case load it in current tab
> 

A use case against this behavior is if you want to keep a cached version of a page and also load a "live" version (eg, to compare changes). Currently I hit *enter* on the url bar and get a new tab; with the proposed behavior I would instead get a refresh? IMHO, this goes against "everything you type in the address bar opens a new tab".
(In reply to sriram from comment #89)

> A use case against this behavior is if you want to keep a cached version of
> a page and also load a "live" version (eg, to compare changes). Currently I
> hit *enter* on the url bar and get a new tab; with the proposed behavior I
> would instead get a refresh? IMHO, this goes against "everything you type in
> the address bar opens a new tab".

Good call.

@Eric, maybe we should reconsider the “refresh on same URL” rule?

Points against it:

* Tab Mix Plus never had it
* complicates code
* reduces consistency
* reasonable use case from an actual user

Points in favor:

* familiarity for long-time users of the traditional behavior?
(In reply to Yuri Khan from comment #90)
> (In reply to sriram from comment #89)
> 
> > A use case against this behavior is if you want to keep a cached version of
> > a page and also load a "live" version (eg, to compare changes). Currently I
> > hit *enter* on the url bar and get a new tab; with the proposed behavior I
> > would instead get a refresh? IMHO, this goes against "everything you type in
> > the address bar opens a new tab".
> 
> Good call.
> 
> @Eric, maybe we should reconsider the “refresh on same URL” rule?
> 
> Points against it:
> 
> * Tab Mix Plus never had it
> * complicates code
> * reduces consistency
> * reasonable use case from an actual user
> 
> Points in favor:
> 
> * familiarity for long-time users of the traditional behavior?

I dindn't even process this. @sriram is absolutely right, consistency beeing key here.
Status: REOPENED → NEW
Priority: P3 → P5
Whiteboard: [specs in comment 83, tests required for landing]
(In reply to Yuri Khan from comment #85)
> (In reply to Eric Pang [:epang] UX from comment #83)
> 
> > Thanks everyone for their interest in this bug, for now I suggest we move
> > forward with Option 1 from Marco. 
> > > option 1: add the new browser.urlbar.openintab pref. 
> > 
> > Here's the expected behavior
> 
> I’d like the whole matrix of possibilities specified. Let me know if I got
> this right:
> 
> |          | Enter or left click | Alt+Enter or middle click |
> |----------|---------------------|---------------------------|
> | URLs     | new tab*†           | current tab               |
> | searches | new tab†            | current tab               |
> | tabs     | switch to tab       | unavailable‡              |
> 
> *) except if the full URL (including #hash) exactly matches the URL in the
> current tab, in which case load it in current tab
> 
> †) position of new tab is governed by (currently non-existent) user
> preferences for new tabs, so at the end of the tab bar for the moment
> 
> ‡) holding down Alt causes the dropdown to treat open tabs as URLs so it
> will be loaded in current tab

Yuri, another consideration would be to reuse a current new/blank tab.
(In reply to Hlsg from comment #91)
> (In reply to Yuri Khan from comment #90)
> > (In reply to sriram from comment #89)
> > 
> > > A use case against this behavior is if you want to keep a cached version of
> > > a page and also load a "live" version (eg, to compare changes). Currently I
> > > hit *enter* on the url bar and get a new tab; with the proposed behavior I
> > > would instead get a refresh? IMHO, this goes against "everything you type in
> > > the address bar opens a new tab".
> > 
> > Good call.
> > 
> > @Eric, maybe we should reconsider the “refresh on same URL” rule?

@Yuri If this is the expected behavior from TMP I don't have a problem with it.
Can you clarify if a new tab is opened or if a duplicate of the tab is opened on enter?
Thanks for checking in with me!

> That would be a good idea indeed. The developer is on bugzilla: tabmix.onemen@gmail.com
@hlsg Thanks for the email.  I've reached out to our community program to let them know about this add on (and for instructions on how to proceed).
(In reply to Eric Pang [:epang] UX from comment #93)

> Can you clarify if a new tab is opened or if a duplicate of the tab is
> opened on enter?

I am having a little trouble seeing the distinction. Both opening the same URL in a new tab and duplicating a tab cause the same observable result:

* I get a new tab with that page in it;
* I get a cache access (observed on the Network tab of the Developer Toolbox);
* I may get a network request;
* and any nondeterminism that is present on the target page fires.

One subtle difference is that the Duplicate Tab menu item o tests required for landingbeys the “[x] Open duplicated tabs next to original” preference while opening the same URL by pressing Enter in the address bar follows the “[ ] Open other tabs next to current one” and “[ ] Only if related to current tab” preferences. In that sense, yes, in TMP, pressing Enter in the address bar opens a new unrelated tab.

Am I missing something else?


(In reply to Hlsg from comment #92)

> Yuri, another consideration would be to reuse a current new/blank tab.

Yes, the try build I made for Eric does that and he didn’t explicitly object.

So, updated spec:

|          |               Enter or left click             | Alt+Enter or |
|          | [current tab not empty] | [current tab empty] | middle click |
|----------|-------------------------|---------------------|--------------|
| URLs     | new unrelated tab       | current tab         | current tab  |
| searches | new unrelated tab       | current tab         | current tab  |
| tabs     | switch to tab           | current tab         | current tab  |


(In reply to Marco Bonardo from comment #91.5)

> tests required for landing

Okay, I will see what I can do.
> Yuri, another consideration would be to reuse a current new/blank tab.

Sorry, this was a misunderstanding on my part.  When I read the above comment I thought it meant opening a completely fresh new tab.  Not opening the current tab in a new tab. 

All good, thanks!
Attachment #8944240 - Flags: review?(adw)
Comment on attachment 8944239 [details] [diff] [review]
Bug 1394304 - Use pref to control whether to open url bar results in a new tab.

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

Thanks Yuri, this looks good basically, but there are a couple of cases I'd like Eric to clarify.

(1) Without this patch, if the current tab is empty and you meta-click an item, the item opens in a new tab.  With the patch *and openintab set to its default value of false*, the item opens in the current, empty tab.  So we're changing behavior for people who don't opt into this new pref.  Is that OK?  On the other hand, this change makes meta-clicking and pressing alt-Enter behave the same.

(This happens because the isTabEmpty() check is now outside the `if (isMouseEvent)` block.)

(2) whereToOpenLink() returns "tabshifted" in a couple of cases [1], and it means the same as "tab" except that the new tab should open in the background.  Should we treat that the same as "tab" in this patch or not?  i.e., if the normal `where` is "tabshifted" and openintab = true, should we treat that as if `where` = "tab" and open the page in the current tab?

[1] https://dxr.mozilla.org/mozilla-central/rev/c4ebc8c28a33b785dfbfa533810517cc707d1ad0/browser/base/content/utilityOverlay.js#155

::: browser/base/content/urlbarBindings.xml
@@ +581,1 @@
>                let altEnter = !isMouseEvent &&

Could you please remove the unnecessary !isMouseEvent term while you're here?
Comment on attachment 8944240 [details] [diff] [review]
Bug 1394304 - Add tests for browser.urlbar.openintab.

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

Hmm, I started writing that this test needs to be more thorough:  It should check alt-Enter on an empty tab, Enter and alt-Enter on switch-to-tab items, and clicks for all combinations of meta/switch-to-tab/empty tab.  And if you're testing all that, then you probably need to test with the one-off buttons, too.  And why limit the action type to switchtab?  You need to test all the other action types, too.

But making sure tabs are actually opened or not on the right pages is secondary to what this patch is doing, which is just determining the right `where`.  What do you think about factoring out the `where` decision into its own method, and then just testing that method?  You could pass mock events to it.

::: browser/base/content/test/urlbar/browser_urlbarEnter_openInTab.js
@@ +25,5 @@
> +  Preferences.set("browser.urlbar.openintab", old_openintab);
> +  await PlacesUtils.history.clear();
> +});
> +
> +add_task(async function() {

Could you please name these test functions?  It makes it a little easier to spot where failures happen in test logs.
Eric, please see comment 98.
Flags: needinfo?(epang)
Assignee: nobody → yurivkhan
Status: NEW → ASSIGNED
Blocks: 1432645
Thanks for flagging me Drew!

> (1) Without this patch, if the current tab is empty and you meta-click an
> item, the item opens in a new tab.  With the patch *and openintab set to its
> default value of false*, the item opens in the current, empty tab.  So we're
> changing behavior for people who don't opt into this new pref.  Is that OK? 
> On the other hand, this change makes meta-clicking and pressing alt-Enter
> behave the same.

When the pref is set to false the default behavior should NOT change.

> (This happens because the isTabEmpty() check is now outside the `if
> (isMouseEvent)` block.)
> 
> (2) whereToOpenLink() returns "tabshifted" in a couple of cases [1], and it
> means the same as "tab" except that the new tab should open in the
> background.  Should we treat that the same as "tab" in this patch or not? 
> i.e., if the normal `where` is "tabshifted" and openintab = true, should we
> treat that as if `where` = "tab" and open the page in the current tab?

IMO, the current implementation works since it gives the user a way to open a tab in the background.  I don't believe there will be a big impact either way since I don't expect most users to know about Shift+Meta+Click.
But I prefer to keep the functionality then to lose it all together when the pref is on.


I also had a chance to reach out to the web extensions team.  They believe this pref will be exposed via a WebExtensions API.
Flags: needinfo?(epang)
Comment on attachment 8944239 [details] [diff] [review]
Bug 1394304 - Use pref to control whether to open url bar results in a new tab.

Thanks Eric.  I'll clear these review requests for now.  Yuri, please flag me again when you have new patches.
Attachment #8944239 - Flags: review?(adw)
Attachment #8944240 - Flags: review?(adw)
Attachment #8946137 - Flags: review?(adw)
Attachment #8944239 - Attachment is obsolete: true
Attachment #8946138 - Flags: review?(adw)
Attachment #8944240 - Attachment is obsolete: true
Attachment #8946139 - Flags: review?(adw)
Attachment #8946136 - Flags: review?(adw) → review+
Comment on attachment 8946137 [details] [diff] [review]
Bug 1394304: Part 0b - Refactor _whereToOpen.

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

::: browser/base/content/urlbarBindings.xml
@@ +522,4 @@
>          <parameter name="event"/>
>          <body><![CDATA[
>            let isMouseEvent = event instanceof MouseEvent;
> +          let altEnter = event && !isMouseEvent && event.altKey;

Please move this inside the `else` block and remove the !isMouseEvent term.
Attachment #8946137 - Flags: review?(adw) → review+
Attachment #8946138 - Flags: review?(adw) → review+
Comment on attachment 8946139 [details] [diff] [review]
Bug 1394304: Part 2 - Add tests for _whereToOpen.

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

Thanks, Yuri.
Attachment #8946139 - Flags: review?(adw) → review+
Moved “let altEnter” back into the !isMouseEvent branch.
Attachment #8946137 - Attachment is obsolete: true
Attachment #8947506 - Flags: review?(adw)
No code changes, just a rebase.
Attachment #8946138 - Attachment is obsolete: true
Attachment #8947508 - Flags: review?(adw)
Attachment #8947506 - Attachment description: 1394304-part0b.patch → Bug 1394304: Part 0b - Refactor _whereToOpen.
Attachment #8947506 - Flags: review?(adw) → review+
Attachment #8947508 - Flags: review?(adw) → review+
I have rebased my patches onto yesterday’s tip of mozilla-central and done a Try build with all tests enabled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=386d0066addd16c06580b0affeb463ebca47b33a

It found a missing semicolon in the tests, which I have amended locally. Additionally, I’m hitting a bunch of test failures, none of which seem to be related.

What do I need to do next?
Attachment #8946139 - Attachment is obsolete: true
Attachment #8948240 - Flags: review?(adw)
Comment on attachment 8948240 [details] [diff] [review]
Bug 1394304: Part 2 - Add tests for _whereToOpen.

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

Thanks Yuri.  You're right, it looks like the errors on your try push are unrelated.  (Except for the eslint failure, which this patch fixes, right?)  I'll mark this bug as checkin-needed, and then one of the sheriffs will land it, so there's nothing else you need to do at this point.
Attachment #8948240 - Flags: review?(adw) → review+
Attachment #8901671 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/256247373b45
Part 0a - Extract method _whereToOpen in urlbarBindings. r=adw
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8ae0661153
Part 0b - Refactor _whereToOpen. r=adw
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3394c9aec3
Part 1 - Use pref to control whether to open url bar results in a new tab. r=adw
https://hg.mozilla.org/integration/mozilla-inbound/rev/420221f11352
Part 2 - Add tests for _whereToOpen. r=adw
Keywords: checkin-needed
Thank you Yuri for doing this. I'm testing it in Nightly and you seem to have done averything right here.

Now the last piece of the puzzle is outlined in this old bug https://bugzilla.mozilla.org/show_bug.cgi?id=513180. As a TMP user, I've never noticed this before 57.
I wonder, how is https://bugzilla.mozilla.org/show_bug.cgi?id=1420969 solved without addresing this? Mike?

(In reply to Eric Pang [:epang] UX from comment #93)
> @hlsg Thanks for the email.  I've reached out to our community program to
> let them know about this add on (and for instructions on how to proceed).

Has/will anything come of this? Also, please see above, Eric.
Flags: needinfo?(mconca)
Flags: needinfo?(epang)
Eric, now that we have a precedent that, for a certain subset of users, middle click does not have to always open new tabs, maybe we could revisit bug 1397372 and bug 1416545 and reopen one of them?
(In reply to Hlsg from comment #115)
> Thank you Yuri for doing this. I'm testing it in Nightly and you seem to
> have done averything right here.
> 
> Now the last piece of the puzzle is outlined in this old bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=513180. As a TMP user, I've
> never noticed this before 57.
> I wonder, how is https://bugzilla.mozilla.org/show_bug.cgi?id=1420969 solved
> without addresing this? Mike?
> 
> (In reply to Eric Pang [:epang] UX from comment #93)
> > @hlsg Thanks for the email.  I've reached out to our community program to
> > let them know about this add on (and for instructions on how to proceed).
> 
> Has/will anything come of this? Also, please see above, Eric.

Mike Conca got back to me about it and said it's unlikely that it will migrate.
He believe this pref will be exposed via a WebExtensions API.
Flags: needinfo?(epang)
Not sure how he comes to this conclusion. There already is an experimental Web Extension port. For the moment it only has limited functionality but oneman is working on it.

https://addons.mozilla.org/en-US/firefox/addon/tab-mix-plus-webextension/

http://www.tabmixplus.org/forum/viewforum.php?f=8

http://tabmixplus.org/support/troubleshooting/data/legacy-tabmix.html
(In reply to Eric Pang [:epang] UX from comment #117)
> (In reply to Hlsg from comment #115)
> > Thank you Yuri for doing this. I'm testing it in Nightly and you seem to
> > have done averything right here.
> > 
> > Now the last piece of the puzzle is outlined in this old bug
> > https://bugzilla.mozilla.org/show_bug.cgi?id=513180. As a TMP user, I've
> > never noticed this before 57.
> > I wonder, how is https://bugzilla.mozilla.org/show_bug.cgi?id=1420969 solved
> > without addresing this? Mike?
> > 
> > (In reply to Eric Pang [:epang] UX from comment #93)
> > > @hlsg Thanks for the email.  I've reached out to our community program to
> > > let them know about this add on (and for instructions on how to proceed).
> > 
> > Has/will anything come of this? Also, please see above, Eric.
> 
> Mike Conca got back to me about it and said it's unlikely that it will
> migrate.
> He believe this pref will be exposed via a WebExtensions API.

As per comment118, there is already an experimental version on AMO. But how about Those old bugs referring to the search address arrow breaking consistency?
The WebExtensions API can expose the browser preference that determines where search results open, but the extensions subsystem does not see or control the front end of the search.  The fix for bug 513180 is completely independent of the WebExtensions API.
Flags: needinfo?(mconca)
Depends on: 1455326
Blocks: 1442843
Hi

How is thos resolved in FF 60? I've ff 60 and still don't see any option for an "open in new tab adressbar entry" or i don't do something right.

Greetings
(In reply to lupus288 from comment #121)

> How is thos resolved in FF 60? I've ff 60 and still don't see any option for
> an "open in new tab adressbar entry" or i don't do something right.

You go to about:config and flip the browser.urlbar.openintab preference to true. After that, pressing Enter in the address bar will open a new tab unless the current tab is empty. Alt+Enter will replace the contents of the current tab.
Thank You Yuri ! Works like a charm :) That's what i was missing from TMP. Hopefully they fix multiple tab rows too.

Greetings
No longer blocks: 1226546
You need to log in before you can comment on or make changes to this bug.