267169 - "Home" button should open home page in new tab if middle-clicked

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

The "Home" button should act like a bookmark in the personal toolbar folder.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

s

Comment 2

[alanjstr]

Firefox already has this.  Heck, in Firefox you can do this for home, back, and
forward.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

I decided to leave the functionality alone if you have multiple tabs for your
home page.  It seems reasonable as-is.

Comment 4

[OstGote!]

(In reply to comment #2)
> Firefox already has this.  
Was bug 232172.

This bug should block the tracking bug 70501.

Comment 5

[[e-mail]]

Comment on attachment 164193 [details] [diff] [review]
Patch

>-                     oncommand="BrowserHome(); event.preventBubble()"
>+                     oncommand="BrowserHome(event); event.preventBubble()"
>+                     onclick="if (event.button == 1) {BrowserHome(event); event.preventBubble()}"
I suppose this preventBubble() will be needed when we finally support middle
clicks on bookmarks in the personal toolbar, but you missed a ; after ()
though. Maybe it would make more sense to put the event.preventBubble() in
BrowserHome()? Or you could replace the oncommand with command="Browser:Home".
Speaking of middle-clicking the personal toolbar, we probably should support
the middle-click to open home page in new window pref for groups for
consistency. You could then get rid of the oncommand and the condition in the
onclick, so that the onclick handler is also used for left clicks. Finally
you've got a redeclaration of var tab.

Comment 6

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 164193 [details] [diff] [review])
>>-                     oncommand="BrowserHome(); event.preventBubble()"
>>+                     oncommand="BrowserHome(event); event.preventBubble()"
>>+                     onclick="if (event.button == 1) {BrowserHome(event);
event.preventBubble()}"
Sorry for mislaying the bookmarks middle click code :-[
You don't need event.preventBubble(); at all because the bookmarks handlers were
moved from the Personal Toolbar to the bookmarks-ptf hbox.

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

For multi-tab home pages:
if browser.tabs.opentabfor.middleclick is false, left clicking the home button
adds tabs, middle clicking brings up a new window.
if browser.tabs.opentabfor.middleclick is true, and you have it set to add
instead of replace tabs, both left and middle clicking add tabs.
if browser.tabs.opentabfor.middleclick is true, and you have it set to replace
instead of add tabs, both left and middle clicking replace tabs.

Comment 9

[[e-mail]]

Comment on attachment 164312 [details] [diff] [review]
Patch

>+      if (!pref.getBoolPref("browser.tabs.loadInBackground"))
if (!BookmarksUtils.shouldLoadTabInBackground(aEvent)) surely?

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Comment 11

[[e-mail]]

Comment on attachment 164407 [details] [diff] [review]
Patch

>+    else {
>+      tab = gBrowser.loadGroup(URIs);
>+      
>+      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
>+        gBrowser.selectedTab = tab;
>+    }
I don't see the point of this blank line but jag might like it there.

Comment 12

[jag (Peter Annema)]

Comment on attachment 164407 [details] [diff] [review]
Patch

>Index: xpfe/browser/resources/content/navigator.js
>===================================================================

>+function BrowserHome(aEvent)
> {
>+  var tab;
>   var homePage = getHomePage();
>+  var target = BookmarksUtils.getBrowserTargetFromEvent(aEvent);

Please move |var tab| and |var target| inside the |if (homePage.length == 1)|.

>   if (homePage.length == 1) {
>-    loadURI(homePage[0]);
>+    switch (target) {
>+    case "current":
>+      loadURI(homePage[0]);
>+      break;
>+    case "tab":
>+      tab = gBrowser.addTab(homePage[0]);
>+      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
>+        gBrowser.selectedTab = tab;
>+      break;
>+    case "window":
>+      openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", homePage[0]);
>+    }
>+

Nit: no need for this blank line.

It seems like the above target check and the resulting actions is done in
several places. Perhaps it should be factored out into a helper function
available to all those places?

>   } else {
>     var URIs = [];
>     for (var i in homePage)
>+      if (target  == "window")
>+        URIs.push(homePage[i]);
>+      else
>+        URIs.push({URI: homePage[i]});

Nit: Got two spaces there before the ==

Since the body of that for-loop is more than one line, prefer to embed it in {
... }; even though not strictly necessary, it makes it more clear where the
body ends and prevents silly bugs like someone adding a second statement to the
body and not noticing it won't really be part of the for-loop.

Since target won't change it would've been better to convey that in the code,
e.g.:

  var URIs = [];
  if (target == "window") {
    for (var i in homePage)
      URIs.push(homePage[i]);
  } else {
    for (var i in homePage)
      URIs.push({URI: homePage[i]});
  }

Well, I guess that's arguable, since now you're doing the for loop twice.
However, there's no need for the first loop, really. Just do |URIs = homePage;|
Or avoid it completely and just use it directly in |openDialog(...)|, and merge
this code with the second check on target, to get:

  } else {
    if (target == "window") {
      openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no",
homePage.join("\n"));
    } else {
      var URIs = [];
      for (var i in homePage)
	URIs.push({URI: homePage[i]});

      tab = gBrowser.loadGroup(URIs);

      if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
	gBrowser.selectedTab = tab;
    }
  }

Comment 13

[jag (Peter Annema)]

> It seems like the above target check and the resulting actions is done in
> several places. Perhaps it should be factored out into a helper function
> available to all those places?

Hmmm, I guess it's done in different enough ways that it wouldn't be worth it,
so nevermind that bit.

Comment 14

[[e-mail]]

(In reply to comment #12)
>(From update of attachment 164407 [details] [diff] [review])
>>+  var tab;
>>   var homePage = getHomePage();
>>+  var target = BookmarksUtils.getBrowserTargetFromEvent(aEvent);
>Please move |var tab| and |var target| inside the |if (homePage.length == 1)|.
That can't be right, can it? They are used in the other case too.

>Or avoid it completely and just use it directly in |openDialog(...)|, and
>merge this code with the second check on target, to get:
> 
>   } else {
>     if (target == "window") {
>       openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no",
> homePage.join("\n"));
>     } else {
>       var URIs = [];
>       for (var i in homePage)
>       URIs.push({URI: homePage[i]});
> 
>       tab = gBrowser.loadGroup(URIs);
> 
>       if (!BookmarksUtils.shouldLoadTabInBackground(aEvent))
>         gBrowser.selectedTab = tab;
>     }
>   }
Good catch! Odd that Bugzilla seemed to tabify the indents. Or was that you?

Comment 15

[jag (Peter Annema)]

(In reply to comment #14)
> >Please move |var tab| and |var target| inside the |if (homePage.length == 1)|.
> That can't be right, can it? They are used in the other case too.

You're right, that wasn't right. They're just fine where they are :-)

> Good catch! Odd that Bugzilla seemed to tabify the indents. Or was that you?

Thanks. I just used spaces, I guess bugzilla tabified some (but not all?) of it.

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Comment 17

[jag (Peter Annema)]

Comment on attachment 164981 [details] [diff] [review]
Patch

sr=jag

Comment 18

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

checked in by timeless

Comment 19

[David E. Ross]

See bug #271426.  

If the addition of a calling parameter to BrowserHome(); remains as implemented,
installation documentation for Mozilla 1.8 must detail that change so that both
extension developers and extension users can make appropriate modifications.