1417133 - When browser.tabs.loadBookmarksInTabs is true, reuse current tab if it is blank

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P5)

Description

[marco]

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.

Comment 1

[Yuri Khan]

Attachment: Reuse blank tab when opening a bookmark in a new tab

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 2

[Marco Bonardo [:mak]]

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 3

[Mark Banner (:standard8)]

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 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

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.

Comment 5

[marco]

How about closing the empty tab instead od reusing it?

Comment 6

[Yuri Khan]

(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?

Comment 7

[Hlsg]

(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.

Comment 8

[Yuri Khan]

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

Comment 9

[Hlsg]

(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.

Comment 10

[Yuri Khan]

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

Comment 11

[Eric Pang [:epang] UX]

(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?

Comment 12

[Yuri Khan]

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

Comment 13

[Eric Pang [:epang] UX]

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.

Comment 14

[Eric Pang [:epang] UX]

(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

Comment 15

[Amy Lee [:amylee] UX]

(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?

Comment 16

[Yuri Khan]

@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.

Comment 17

[Hlsg]

(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.

Comment 18

[Yuri Khan]

(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.

Comment 19

[Hlsg]

(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!

Comment 20

[Eric Pang [:epang] UX]

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?

Comment 21

[Yuri Khan]

(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?

Comment 22

[Eric Pang [:epang] UX]

(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 23

[Yuri Khan]

Attachment: When loadBookmarksInTabs is true and current tab is blank, reuse it.

Comment 24

[Mark Banner (:standard8)]

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(...)`

Comment 25

[Yuri Khan]

(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 26

[Yuri Khan]

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?

Comment 27

[Mark Banner (:standard8)]

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 28

[Yuri Khan]

Attachment: When loadBookmarksInTabs is true and current tab is blank, reuse it.

Comment 29

[Yuri Khan]

Attachment: Add tests.

Comment 30

[Mark Banner (:standard8)]

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.

Comment 31

[Yuri Khan]

Attachment: 1417133.patch

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

Comment 32

[Mark Banner (:standard8)]

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.

Comment 33

[Hlsg]

(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.

Comment 34

[Marco Bonardo [:mak]]

can this land?

Comment 35

[Mark Banner (:standard8)]

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50c17275d91a
When loadBookmarksInTabs is true and current tab is blank, reuse it. r=Standard8

Comment 37

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/50c17275d91a

Comment 39

[Mohammad Maruf Rahman]

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

Comment 40

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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

Comment 41

[j123123us]

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.

Comment 42

[Mark Banner (:standard8)]

(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.

Comment 43

[wroot]

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