Closed
Bug 267169
Opened 20 years ago
Closed 20 years ago
"Home" button should open home page in new tab if middle-clicked
Categories
(SeaMonkey :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: csthomas, Assigned: csthomas)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
5.33 KB,
patch
|
neil
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
The "Home" button should act like a bookmark in the personal toolbar folder.
Assignee | ||
Comment 1•20 years ago
|
||
s
Assignee | ||
Updated•20 years ago
|
Attachment #164188 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #164188 -
Flags: review?(timeless)
Firefox already has this. Heck, in Firefox you can do this for home, back, and forward.
Assignee | ||
Updated•20 years ago
|
Attachment #164188 -
Attachment is obsolete: true
Attachment #164188 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #164188 -
Flags: review?(timeless)
Assignee | ||
Comment 3•20 years ago
|
||
I decided to leave the functionality alone if you have multiple tabs for your home page. It seems reasonable as-is.
Assignee: general → cst
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #164193 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #164193 -
Flags: review?(timeless)
Assignee | ||
Updated•20 years ago
|
Attachment #164193 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #164193 -
Flags: superreview?(jag)
Attachment #164193 -
Flags: review?(timeless)
Attachment #164193 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•20 years ago
|
Whiteboard: active
(In reply to comment #2) > Firefox already has this. Was bug 232172. This bug should block the tracking bug 70501.
Comment 5•20 years ago
|
||
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.
Attachment #164193 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Updated•20 years ago
|
Blocks: link-modifiers
Comment 6•20 years ago
|
||
(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.
Assignee | ||
Comment 7•20 years ago
|
||
Attachment #164193 -
Attachment is obsolete: true
Assignee | ||
Comment 8•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #164312 -
Flags: superreview?(jag)
Attachment #164312 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•20 years ago
|
Attachment #164193 -
Flags: superreview?(jag)
Comment 9•20 years ago
|
||
Comment on attachment 164312 [details] [diff] [review] Patch >+ if (!pref.getBoolPref("browser.tabs.loadInBackground")) if (!BookmarksUtils.shouldLoadTabInBackground(aEvent)) surely?
Assignee | ||
Updated•20 years ago
|
Attachment #164312 -
Attachment is obsolete: true
Attachment #164312 -
Flags: superreview?(jag)
Attachment #164312 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 10•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #164407 -
Flags: superreview?(jag)
Attachment #164407 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 11•20 years ago
|
||
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.
Attachment #164407 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Comment 12•20 years ago
|
||
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•20 years ago
|
||
> 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•20 years ago
|
||
(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•20 years ago
|
||
(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.
Assignee | ||
Updated•20 years ago
|
Attachment #164407 -
Attachment is obsolete: true
Attachment #164407 -
Flags: superreview?(jag)
Assignee | ||
Comment 16•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #164981 -
Flags: superreview?(jag)
Attachment #164981 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 17•20 years ago
|
||
Comment on attachment 164981 [details] [diff] [review] Patch sr=jag
Attachment #164981 -
Flags: superreview?(jag) → superreview+
Updated•20 years ago
|
Attachment #164981 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Comment 18•20 years ago
|
||
checked in by timeless
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: active
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 19•20 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•