User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20171112125346 Steps to reproduce: - Go to about:config - Set browser.tabs.closeWindowWithLastTab to false - Set browser.tabs.loadBookmarksInTabs to true - Close all the tabs - Now you have a blank page and you are ready to open some links - Click on a bookmark Actual results: - A new tab is opened with the requested link - The empty original tab is left there open and useless Expected results: - The new tab should replace the empty original tab There are some related bugs, #344381 and #320989, but I think this is a different case. If I manually createad a new empty tab and that was not reused, then fine, I would understand why it's not reused. The problem is that the last empty tab is forced by browser.tabs.closeWindowWithLastTab and this setting doesn't interact well with browser.tabs.loadBookmarksInTabs.

This patch checks if a new tab would be opened and, if the current tab is blank, reuses it instead of opening a new one. This is not dependent on the state of loadBookmarksInTabs.

Comment on attachment 8936031 [details] [diff] [review] Reuse blank tab when opening a bookmark in a new tab Review of attachment 8936031 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding the review to Mark since I'll be away for a while. Off-hand it looks ok but I didn't have enough time to think of edge cases. THOUGH, I wonder if this should only apply to loadBookmarksInTabs or, as your patch implements, to any open in tab case. This is something that requires UX feedback, because there is also the possibility some users may not appreciate this behavior, or that we just want to keep consistency with the rest of the browser where opening in a tab usually just does that. We may care or not, it's not a call I can make without UX. Pinging Philipp, eventually feel free to forward. If I should meet him in person I'll post the result of our discussion here.

Comment on attachment 8936031 [details] [diff] [review] Reuse blank tab when opening a bookmark in a new tab Review of attachment 8936031 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling review until we hear back from UX. I'll ping them directly to see if we can get an answer on this soon.

Comment on attachment 8936031 [details] [diff] [review] Reuse blank tab when opening a bookmark in a new tab Hm, this could lead to a bunch of complications down the road, since it potentially breaks the mental model of where new tabs appear. Especially when many tabs are opened with some of them being potentially blank, this could lead to new tabs appearing in essentially random places. Since this feature would be mostly about the convenience of not having to close empty tabs yourself, I don't think it's worth that tradeoff.

How about closing the empty tab instead od reusing it?

(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #4) > Hm, this could lead to a bunch of complications down the road, since it > potentially breaks the mental model of where new tabs appear. (Well, that model is already broken in my opinion; see bug 1296043, bug 1387372, and bug 1344749.) > Especially when many tabs are opened with some of them being potentially blank, this > could lead to new tabs appearing in essentially random places. The idea is not to reuse any random empty tab. It is to reuse the tab the user is currently looking at, on the condition that it is empty. Also, the idea, for me at least, is that the one major use case for this reuse is when the window has exactly one tab and that tab is empty (because the window has just been created, or because all of the window’s tabs have been closed). What’d you say to reusing the empty tab only in these conditions?

(In reply to Yuri Khan from comment #6) > (In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) > please use needinfo from comment #4) > > > Hm, this could lead to a bunch of complications down the road, since it > > potentially breaks the mental model of where new tabs appear. > > (Well, that model is already broken in my opinion; see bug 1296043, bug > 1387372, and bug 1344749.) > > > Especially when many tabs are opened with some of them being potentially blank, this > > could lead to new tabs appearing in essentially random places. > > The idea is not to reuse any random empty tab. It is to reuse the tab the > user is currently looking at, on the condition that it is empty. > > Also, the idea, for me at least, is that the one major use case for this > reuse is when the window has exactly one tab and that tab is empty (because > the window has just been created, or because all of the window’s tabs have > been closed). What’d you say to reusing the empty tab only in these > conditions? This makes perfect sense to me. Of course it shouldn't touch any blank tabs that don't have focus.

The discussion seems to have stalled here. Philipp, could you please address comment 6, second half?

(In reply to Yuri Khan from comment #8) > The discussion seems to have stalled here. Philipp, could you please address > comment 6, second half? Yuri, maybe you should needinfo someone else from UX.

@Eric, you helped with bug 1394304. The change proposed here would be consistent with the behavior introduced by that change. What do you say?

(In reply to Yuri Khan from comment #10) > @Eric, you helped with bug 1394304. The change proposed here would be > consistent with the behavior introduced by that change. What do you say? >The idea is not to reuse any random empty tab. It is to reuse the tab the user is currently looking at, on the condition that it >is empty. >Also, the idea, for me at least, is that the one major use case for this reuse is when the window has exactly one tab and that >tab is empty (because the window has just been created, or because all of the window’s tabs have been closed). What’d you say to >reusing the empty tab only in these conditions? @Yuri, sorry for the delayed response on this. Just want to clarify that I'm understanding the experience proposed correctly. When opening a bookmark in a currently selected... ...content filled tab (not new tab) > the bookmark opens in a new tab ...new tab > the bookmark opens on the current tab Is my understanding right?

Yes, that’s the gist of the proposal and the tentative implementation in attachment 8936031 [details] [diff] [review].

Thanks for confirming so quickly Yuri! Going to redirect this to Amy Lee since she works closely with new tab and bookmarks. She can determine if this is desired behavior.

(In reply to Yuri Khan from comment #12) > Yes, that’s the gist of the proposal and the tentative implementation in > attachment 8936031 [details] [diff] [review]. Yuri, can you help clarify if this is dependent on the following prefs being set? Or are we talking about default behaviour? - Set browser.tabs.closeWindowWithLastTab to false - Set browser.tabs.loadBookmarksInTabs to true

(In reply to Eric Pang [:epang] UX from comment #13) > Thanks for confirming so quickly Yuri! Going to redirect this to Amy Lee > since she works closely with new tab and bookmarks. She can determine if > this is desired behavior. I'm on Nightly right now and this is the behavior of opening a bookmark: 1. Opening a bookmark when I am on new tab will open the bookmark in the same tab 2. Opening a bookmark when I am on a website will also open the bookmark in the same tab This feels correct to me. Is there something that I am missing in the discussion?

@Amy: The context of this feature request is when browser.tabs.loadBookmarksInTabs is set to true. With that setting, bookmarks always open in new tabs. In case there already is a new tab, we want to reuse it. @Eric: The attached patch is not dependent on any of these. Any time a bookmark would open in a new tab, if the current tab is empty, then it is reused. I don’t personally think conditioning on closeWindowWithLastTab is a good idea. For example, opening a new window also results in a new/empty tab, and opening a bookmark there will end up with an unneeded tab. Conditioning on loadBookmarksInTabs, on the other hand, is okay with me. Will do if needed.

(In reply to Yuri Khan from comment #16) > @Amy: > The context of this feature request is when browser.tabs.loadBookmarksInTabs > is set to true. With that setting, bookmarks always open in new tabs. In > case there already is a new tab, we want to reuse it. > > > @Eric: > The attached patch is not dependent on any of these. Any time a bookmark > would open in a new tab, if the current tab is empty, then it is reused. > > I don’t personally think conditioning on closeWindowWithLastTab is a good > idea. For example, opening a new window also results in a new/empty tab, and > opening a bookmark there will end up with an unneeded tab. > > Conditioning on loadBookmarksInTabs, on the other hand, is okay with me. > Will do if needed. This shouldn't be conditioned on loadBookmarksInTabs as this affects external links as well as bookmarks, and for external links, the behavior is independent of loadBookmarksInTabs. Btw., external links should be covered as well, as they are just as annoying.

(In reply to Hlsg from comment #17) > This shouldn't be conditioned on loadBookmarksInTabs as this affects > external links as well as bookmarks, and for external links, the behavior is > independent of loadBookmarksInTabs. Btw., external links should be covered > as well, as they are just as annoying. Oh please file another bug for external links. That’s a different place in code and it opens a whole other can of worms about which window external links should open in.

(In reply to Yuri Khan from comment #18) > (In reply to Hlsg from comment #17) > > This shouldn't be conditioned on loadBookmarksInTabs as this affects > > external links as well as bookmarks, and for external links, the behavior is > > independent of loadBookmarksInTabs. Btw., external links should be covered > > as well, as they are just as annoying. > > Oh please file another bug for external links. That’s a different place in > code and it opens a whole other can of worms about which window external > links should open in. Oh, I see! Well, you're obviously a lot more familiar with the workings of this, so you could formulate said bug a lot better than me if you're willing to take the time. Please link to it if you do. Thanks!

Yuri, Amy and I discussed the functionality and we agree that it makes sense for bookmarks to open on a selected new tab when behind the pref loadBookmarksInTabs. To remain consistent with the pref browser.urlbar.openintab and the above functionality we should also update browser.search.openintab to do the same in a new tab. Thoughts?

(In reply to Eric Pang [:epang] UX from comment #20) > Yuri, > > Amy and I discussed the functionality and we agree that it makes sense for > bookmarks to open on a selected new tab when behind the pref > loadBookmarksInTabs. Thanks. I will update the patch. > To remain consistent with the pref browser.urlbar.openintab and the above > functionality we should also update browser.search.openintab to do the same > in a new tab. Thoughts? Makes sense. Will you file a ticket or should I?

(In reply to Yuri Khan from comment #21) > (In reply to Eric Pang [:epang] UX from comment #20) > > Yuri, > > > > Amy and I discussed the functionality and we agree that it makes sense for > > bookmarks to open on a selected new tab when behind the pref > > loadBookmarksInTabs. > > Thanks. I will update the patch. > > > To remain consistent with the pref browser.urlbar.openintab and the above > > functionality we should also update browser.search.openintab to do the same > > in a new tab. Thoughts? > > Makes sense. Will you file a ticket or should I? I filed Bug 1455326 to cover this. Thanks!

Comment on attachment 8969999 [details] [diff] [review] When loadBookmarksInTabs is true and current tab is blank, reuse it. Review of attachment 8969999 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch. I think there's a few things that can be tidied up with it. We will also need some tests for it before we can accept it. This is probably a good candidate to start with, as it already toggles the loadBookmarksInTabs pref: https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js It would be nice if we could also extend the sidebar tests to cover the preference as well, so we have coverage in more of our locations: https://searchfox.org/mozilla-central/source/browser/components/places/tests/browser/browser_sidebar_open_bookmarks.js ::: browser/components/places/PlacesUIUtils.jsm @@ +705,5 @@ > function PUIU_openNodeWithEvent(aNode, aEvent) { > let window = aEvent.target.ownerGlobal; > + // Prefer the caller window if it's a browser window, otherwise use > + // the top browser window. > + var browserWindow = null; Generally you shouldn't assign a variable to null, and then assign it to something else straight away. Just assign it to the value you want. Please also use let rather than var. @@ +708,5 @@ > + // the top browser window. > + var browserWindow = null; > + browserWindow = > + window && window.document.documentElement.getAttribute("windowtype") == "navigator:browser" ? > + window : BrowserWindowTracker.getTopWindow(); I think this would be made clearer if you did something like: let window = aEvent.target.ownerGlobal; let browserWindow = window; if (!browserWindow || window.document.documentElement.getAttribute("windowtype") != "navigator:browser") { BrowserWindowTracker.getTopWindow(); } @@ +715,5 @@ > + if (this.loadBookmarksInTabs && PlacesUtils.nodeIsBookmark(aNode)) { > + if (where == "current" && !aNode.uri.startsWith("javascript:")) { > + where = "tab"; > + } > + if (where == "tab" && browserWindow.isTabEmpty(browserWindow.gBrowser.selectedTab)) { Why not just add the isTabEmpty() condition to the original if statement? i.e. `&& !browserWindow.isTabEmpty(...)`

(In reply to Mark Banner (:standard8) from comment #24) > Thank you for the patch. I think there's a few things that can be tidied up > with it. We will also need some tests for it before we can accept it. That’s fair. I will see what I can do about tests.

Comment on attachment 8969999 [details] [diff] [review] When loadBookmarksInTabs is true and current tab is blank, reuse it. Review of attachment 8969999 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/PlacesUIUtils.jsm @@ +705,5 @@ > function PUIU_openNodeWithEvent(aNode, aEvent) { > let window = aEvent.target.ownerGlobal; > + // Prefer the caller window if it's a browser window, otherwise use > + // the top browser window. > + var browserWindow = null; This block was copy-pasted from _openTabSet, about 100 lines above. Should I fix it there as well? Maybe extract it as a helper function? ```c++ _getBrowserWindow: function PIUI__getBrowserWindow(aWindow) { if (!window || window.document.documentElement.getAttribute("windowtype") != "navigator:browser") { return BrowserWindowTracker.getTopWindow(); } return window; } // there: let browserWindow = this._getBrowserWindow(aWindow); // here: let browserWindow = this._getBrowserWindow(aEvent.target.ownerGlobal); ``` @@ +715,5 @@ > + if (this.loadBookmarksInTabs && PlacesUtils.nodeIsBookmark(aNode)) { > + if (where == "current" && !aNode.uri.startsWith("javascript:")) { > + where = "tab"; > + } > + if (where == "tab" && browserWindow.isTabEmpty(browserWindow.gBrowser.selectedTab)) { Consider the following four cases. 1: Left click on bookmark, non-empty tab. `whereToOpenLink` returns "current", first `if` changes it to "tab", second `if` falls through. 2: Left click on bookmark, empty tab. `whereToOpenLink` returns "current", first `if` changes it to "tab", second `if` changes it back to "current". 3: Middle click on bookmark, non-empty tab. `whereToOpenLink` returns "tab", both `if`s fall through. 4: Middle click on bookmark, empty tab. `whereToOpenLink` returns "tab", first `if` falls through, second `if` changes it to "current". If I do as you suggest, case 4 will open a new tab, even though the current tab is empty. Do we want that?

Hi Yuri, sorry for the slow response. (In reply to Yuri Khan from comment #26) > Comment on attachment 8969999 [details] [diff] [review] > ::: browser/components/places/PlacesUIUtils.jsm > @@ +705,5 @@ > > function PUIU_openNodeWithEvent(aNode, aEvent) { > > let window = aEvent.target.ownerGlobal; > > + // Prefer the caller window if it's a browser window, otherwise use > > + // the top browser window. > > + var browserWindow = null; > > This block was copy-pasted from _openTabSet, about 100 lines above. Should I > fix it there as well? Maybe extract it as a helper function? Ah yes, I think we should definitely extract that as a helper function, not in the PlacesUIUtils object, but at the bottom of the file. We should definitely drop the intermediate assignment to null, as that's just useless. Given that we'll be passing in a window, I think this could be as simple as `return window.document.... ? ... : ...`, and we wouldn't need to break it up like I suggested before (as it isn't quite as complex as it was looking). > @@ +715,5 @@ > Consider the following four cases. Ah, thank you that's what I was missing. Probably best to keep the code as it is then :-) It would be really nice if you could include test cases for these, so we can easily check for regressions. Let me know if you need help on those.

Comment on attachment 8971868 [details] [diff] [review] Add tests. Review of attachment 8971868 [details] [diff] [review]: ----------------------------------------------------------------- These look great, thank you for writing them. There's a couple of minor things to address below. When you upload the updated version, please can you do it as just one patch, as that's how we'll be landing them, and just request review from me a final time. ::: browser/components/places/tests/browser/browser_sidebar_open_bookmarks.js @@ +86,5 @@ > + tree.selectItems([gBms[1].guid]); > + let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser, gBms[1].url); > + tree.focus(); > + EventUtils.sendKey("return"); > + let tab = await newTabPromise; Please can you name this `tab` something different to above (the withNewTab callback)? e.g. `newTab`, this avoids confusion as to which tab is being acted upon. @@ +87,5 @@ > + let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser, gBms[1].url); > + tree.focus(); > + EventUtils.sendKey("return"); > + let tab = await newTabPromise; > + Assert.ok(true, "The bookmark was opened in a new tab.") There's a missing semicolon on this line. If you run ./mach eslint browser/components/places it'll tell you about these types of issues.

Renamed “tab” variables, added the missing semicolon, squashed the patches.

Comment on attachment 8972514 [details] [diff] [review] 1417133.patch Review of attachment 8972514 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the update, it looks good. I've just pushed this to the try server to confirm that the tests are fine (I ran them locally so they should be). Once that completes we can add the checkin-needed keyword to get this landed.

(In reply to Yuri Khan from comment #18) > (In reply to Hlsg from comment #17) > > This shouldn't be conditioned on loadBookmarksInTabs as this affects > > external links as well as bookmarks, and for external links, the behavior is > > independent of loadBookmarksInTabs. Btw., external links should be covered > > as well, as they are just as annoying. > > Oh please file another bug for external links. That’s a different place in > code and it opens a whole other can of worms about which window external > links should open in. Yuri, I've opened bug1458534 for external links.

can this land?

Thanks for pointing this out, somehow I'd missed updating this. Adding checkin-needed, here's the try push (all ok): https://treeherder.mozilla.org/#/jobs?repo=try&revision=54e8ff12b7cbb1e4173c653e865e39d0cbe74cbd

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/50c17275d91a When loadBookmarksInTabs is true and current tab is blank, reuse it. r=Standard8

I have reproduced this bug with Nightly 59.0a1 (2017-11-14) on Windows 10, 64 Bit! This bug's fix is verified with latest Beta! Build ID 20180713213322 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

Setting Firefox 62 as verified per comment 39. Thank you Mohammad Maruf Rahman for your testing efforts!

This only works for opening one bookmark link in blank tab, not works for opening multiple links such as "Open all in Tabs" in bookmark folder which contains 2 or more links, checked on version 62.0.3.

(In reply to j123123us from comment #41) > This only works for opening one bookmark link in blank tab, not works for > opening multiple links such as "Open all in Tabs" in bookmark folder which > contains 2 or more links, checked on version 62.0.3. Please file a new bug for that with steps to repeat. Commenting on an old bug is likely to get lost.

Filed as https://bugzilla.mozilla.org/show_bug.cgi?id=1498093