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

VERIFIED FIXED in Firefox 62

Status

()

defect
P5
normal
VERIFIED FIXED
2 years ago
8 months ago

People

(Reporter: marco, Assigned: yurivkhan)

Tracking

(Blocks 2 bugs)

57 Branch
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 verified)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

2 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5
Assignee

Comment 1

2 years ago
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.
Attachment #8936031 - Flags: review?(mak77)
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.
Attachment #8936031 - Flags: review?(standard8)
Attachment #8936031 - Flags: review?(mak77)
Attachment #8936031 - Flags: feedback?(philipp)
Assignee: nobody → yurivkhan
Status: NEW → ASSIGNED
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.
Attachment #8936031 - Flags: review?(standard8)
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.
Attachment #8936031 - Flags: feedback?(philipp) → feedback-
Reporter

Comment 5

Last year
How about closing the empty tab instead od reusing it?
Assignee

Comment 6

Last year
(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

Last year
(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.
Assignee

Comment 8

Last year
The discussion seems to have stalled here. Philipp, could you please address comment 6, second half?
Flags: needinfo?(philipp)

Comment 9

Last year
(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.
Assignee

Comment 10

Last year
@Eric, you helped with bug 1394304. The change proposed here would be consistent with the behavior introduced by that change. What do you say?
Flags: needinfo?(epang)
Summary: Blank tabs should be reused when browser.tabs.loadBookmarksInTabs is true → When browser.tabs.loadBookmarksInTabs is true, reuse current tab if it is blank
(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?
Flags: needinfo?(epang) → needinfo?(yurikhan)
Assignee

Comment 12

Last year
Yes, that’s the gist of the proposal and the tentative implementation in attachment 8936031 [details] [diff] [review].
Flags: needinfo?(epang)
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.
Flags: needinfo?(epang) → needinfo?(amlee)
(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?
Flags: needinfo?(amlee)
Assignee

Comment 16

Last year
@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.
Flags: needinfo?(amlee)

Comment 17

Last year
(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.
Assignee

Comment 18

Last year
(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

Last year
(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?
Assignee

Comment 21

Last year
(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!
Flags: needinfo?(philipp)
Flags: needinfo?(amlee)

Updated

Last year
Blocks: 1442843
Assignee

Comment 23

Last year
Attachment #8936031 - Attachment is obsolete: true
Attachment #8969999 - Flags: review?(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(...)`
Attachment #8969999 - Flags: review?(standard8) → review-
Assignee

Comment 25

Last year
(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.
Assignee

Comment 26

Last year
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.
Assignee

Comment 28

Last year
Attachment #8969999 - Attachment is obsolete: true
Attachment #8971867 - Flags: review?(standard8)
Assignee

Comment 29

Last year
Posted patch Add tests. (obsolete) — Splinter Review
Attachment #8971868 - Flags: review?(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.
Attachment #8971868 - Flags: review?(standard8) → feedback+
Attachment #8971867 - Flags: review?(standard8) → feedback+
Assignee

Comment 31

Last year
Posted patch 1417133.patchSplinter Review
Renamed “tab” variables, added the missing semicolon, squashed the patches.
Attachment #8971867 - Attachment is obsolete: true
Attachment #8971868 - Attachment is obsolete: true
Attachment #8972514 - Flags: review?(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.
Attachment #8972514 - Flags: review?(standard8) → review+

Comment 33

Last year
(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?
Flags: needinfo?(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
Flags: needinfo?(yurikhan)
Flags: needinfo?(standard8)
Keywords: checkin-needed

Comment 36

Last year
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
Keywords: checkin-needed

Comment 37

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/50c17275d91a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Duplicate of this bug: 1474086
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [good first verify] [bugday-20180718]
Setting Firefox 62 as verified per comment 39. Thank you Mohammad Maruf Rahman for your testing efforts!
Status: RESOLVED → VERIFIED

Comment 41

8 months ago
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.

Updated

8 months ago
Blocks: 1498093
You need to log in before you can comment on or make changes to this bug.