1455326 - When browser.search.openintab is true, reuse current tab if it is blank

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P3)

Description

[Eric Pang [:epang] UX]

To remain consistent with the prefs browser.urlbar.openintab and browser.tabs.loadBookmarksInTabs we should also update browser.search.openintab to reuse current new tab (if it's the active tab).

Comment 1

[Mike Kaply [:mkaply]]

How do you define blank?

Comment 2

[Yuri Khan]

In browser/base/content/browser.js, there is a function for that.

https://searchfox.org/mozilla-central/search?q=isTabEmpty&path=

Comment 3

[Hlsg]

See bug1458534 for external links.

Comment 4

[hdd60311]

Any news about this?

Comment 5

[Mike Kaply [:mkaply]]

Attachment: Bug 1455326 - Don't open search in new tab if current tab is blank.

Review commit: https://reviewboard.mozilla.org/r/255152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255152/

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8990150 [details]
Bug 1455326 - Don't open search in new tab if current tab is blank.

https://reviewboard.mozilla.org/r/255152/#review262052

Thanks Mike!

::: browser/components/search/content/search.xml:286
(Diff revision 1)
>                where += "-background";
>            } else {
>              var newTabPref = Services.prefs.getBoolPref("browser.search.openintab");
>              if (((aEvent instanceof KeyboardEvent) && aEvent.altKey) ^ newTabPref)
>                where = "tab";
> +            if (where == "tab" && isTabEmpty(gBrowser.selectedTab)) {

This should get the job done, but you could write it a little more simply IMO:

  if (((aEvent instanceof KeyboardEvent && aEvent.altKey) ^ newTabPref) &&
      !isTabEmpty(gBrowser.selectedTab)) {
    where = "tab";
  }

Since `where` is "current" by default.  Writing it this way also emphasizes that this is connected to the newTabPref and not e.g. the mouse check right below this.

But up to you, r+ either way

::: browser/components/search/content/search.xml:1791
(Diff revision 1)
>              }
>            } else {
>              var newTabPref = Services.prefs.getBoolPref("browser.search.openintab");
>              if (((aEvent instanceof KeyboardEvent) && aEvent.altKey) ^ newTabPref)
>                where = "tab";
> +            if (where == "tab" && isTabEmpty(gBrowser.selectedTab)) {

Here too of course

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 8990150 [details]
Bug 1455326 - Don't open search in new tab if current tab is blank.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255152/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa2c71e82371
Don't open search in new tab if current tab is blank. r=adw

Comment 9

[Hlsg]

Thanks for this, Mike!
Just leaving this here, as it's the last piece missing from this "puzzle" with this and bug1417133 being fixed: bug1458534.

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/aa2c71e82371