Bookmarks should always opens a new tab when the current tab is an app tab

VERIFIED FIXED in Firefox 4.0b7

Status

()

defect
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: ipottinger, Assigned: sindrebugzilla)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

Trunk
Firefox 4.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [mozmill-test-needed][in-litmus-bug-week])

Attachments

(1 attachment, 5 obsolete attachments)

The application hosted within an App Tab can be lost by clicking a bookmark while an app tab is the current tab. 

This will remain a concern even if by default the bookmark menu, toolbar and sidebar are inaccessible/disabled when an app tab is active. The personal-bookmark widget can be moved via customization to the menu or tabs toolbars, making it permanently accessible despite the proposed changes to the UI for app tabs.

One solution would be to ensure clicking a bookmark always opens a new tab when the current tab is an app tab.
Blocks: 579874
Blocks: pinnedtabs
But please make sure "Bookmarklets" run on same page with out any problem.
Posted patch Patch + test (obsolete) — Splinter Review
Changed whereToOpenLink() so it returns "tab" instead of "current" if the current tab is pinned. Also added another parameter to the function which if set to true causes the function to skip the aforementioned step.

This patch prevents links from the following sources from opening in the current tab when the current tab is pinned: 
*Bookmarks (except javascript bookmarks)
*History (except Back button, etc.)
*Home button

I assume this is the desired behavior for all of those sources, and that it therefore is acceptable for this patch to handle all of them even though it goes beyond the scope of the bug report?

Should the location bar/search bar also redirect to a new tab when the current tab is pinned? If so perhaps it would make sense to have this patch handle that as well?

The test seems less than optimal, but I couldn't figure out how to properly make the script open a bookmark. Any advice would be appreciated.
Wow Sindre, glad to see someone pick up the ball and run!

(In reply to comment #2)
> Should the location bar/search bar also redirect to a new tab when the current
> tab is pinned? If so perhaps it would make sense to have this patch handle that
> as well?

I'm not sure if in the finalized UI the location bar/search bar will be present when an AppTab is active.  I think I remember seeing some mockups where the whole location toolbar was removed. Best to ask the UX team on what their current thinking is before investing your time.
(In reply to comment #2)
> Should the location bar/search bar also redirect to a new tab when the current
> tab is pinned? If so perhaps it would make sense to have this patch handle that
> as well?

AppTabs will be chromeless.
Attachment #463277 - Flags: review?(gavin.sharp)
(In reply to comment #4)
> (In reply to comment #2)
> > Should the location bar/search bar also redirect to a new tab 
> AppTabs will be chromeless.

What if I put Search Bar (as well as URL bar) on Bookmark Toolbar
Nothing. Chromeless means no chrome at all.
No chrome mean you won't see any toolbar except for the Tab bar.
Exactly.
Has it been decided already? Because what this means is that users with bookmarklets for their apptabs won't be able to use them, users with the new tab (and others!) button on the navigation bar won't be able to open new tabs, users who have app tabbed an about:blank tab won't be able to navigate to a site they want as an app tab.

And if it's the same for the Home Tab (as originally planned), your going to get "my Firefox doesn't have an interface! Help!"

It's such a bad idea, I can't begin to describe it. Oh, way, I actually can:

http://groups.google.com/group/mozilla.dev.usability/browse_thread/thread/0dd173f268a16f34
(In reply to comment #7)
> No chrome mean you won't see any toolbar except for the Tab bar.

If that is the case why do we want to fix this bug, As per your explanation of chromeless there is no bookmarks user can see.
I feel in chromeless along with Tab bar we should allow user to create custom toolbar which will be visible on chromeless
(In reply to comment #10)
> If that is the case why do we want to fix this bug, As per your explanation of
> chromeless there is no bookmarks user can see.

In the default state there might not be any bookmarks user can see, however as per comment 0, if the user customizes the browser by moving the personal-bookmark widget or the bookmark button to the tab or menu toolbars then they will remain accessible even in the proposed "chromeless" AppTabs unless they are made to automatically disable themselves or disappear.
I read in Blair's blog that Proposed Chromeless AppTab are experimental and by no means final proposal for Firefox 4.
(In reply to comment #11)
> per comment 0, if the user customizes the browser by moving the
> personal-bookmark widget or the bookmark button to the tab or menu toolbars
> then they will remain accessible even in the proposed "chromeless" AppTabs
> unless they are made to automatically disable themselves or disappear.

So user can customize URL and Search Bar, or Drag and Drop a URL.

Then we need to handle all the above situations. 
If not in this bug, then in another one.
(In reply to comment #13)
> Then we need to handle all the above situations. 
> If not in this bug, then in another one.

Biji, at the top of this page you will find the "Blocks:" field which list bugs that depend on this one being fixed first.  In particular, bug 579874's "Depends on:" field lists other bugs related to preventing App Tabs from being "overwritten".  Please feel free to file any additional bugs related to this issue and mark them as blocking both bug 579874 and bug 551849
(In reply to comment #2)
> Should the location bar/search bar also redirect to a new tab when the current
> tab is pinned?

Probably. At least that's the feeling I have from the UX team.

> If so perhaps it would make sense to have this patch handle that
> as well?

Probably best to do that in a new bug. This bug is about bookmarks specifically.

To all debating the chromelessness of App Tabs... There is a plan to make them chromeless, but honestly, if I had to drop any requirements from App Tabs, that's one of the first to go.
(In reply to comment #15)
> (In reply to comment #2)
> > If so perhaps it would make sense to have this patch handle that
> > as well?
> 
> Probably best to do that in a new bug. This bug is about bookmarks
> specifically.

In my current patch I replaced some code in urlbarBindings.xml and search.xml to force the behavior when clicking the search button or go button (but not when using enter to submit as that piece of code doesn't call whereToOpenLink) to be the same as it was pre-patch. To me it seems strange for this patch to contain this code change if a future patch is just going to reverse it.

Also, should I request review from someone else, or just wait? I never can figure out who the right person to request review from is.
Comment on attachment 463277 [details] [diff] [review]
Patch + test

>+++ b/browser/base/content/test/browser_bug579872.js
>+function test() {
>+  let oldTab = gBrowser.selectedTab;
>+  gBrowser.pinTab(oldTab);
>+
>+  let fakeEvent = [];
>+  fakeEvent.button = 0;

let fakeEvent = { button: 0 }

>+  isnot( whereToOpenLink( fakeEvent, false, false, false ), "current", "Shouldn't open in current tab." );
>+  
>+  gBrowser.unpinTab(oldTab);
>+}

You're right that this isn't the greatest test. If you don't actually test that a new tab is being opened, you should at least cover your bases and simulate each use case you have (ie. the same parameters to whereToOpenLink).


>-function whereToOpenLink( e, ignoreButton, ignoreAlt )
>+function whereToOpenLink( e, ignoreButton, ignoreAlt, noPinnedForce )

The name of this argument is a bit confusing. Might be better to call it ignorePinned.

>+++ b/browser/components/places/src/PlacesUIUtils.jsm
>   openNodeWithEvent: function PUIU_openNodeWithEvent(aNode, aEvent) {
>-    this.openNodeIn(aNode, this._getCurrentActiveWin().whereToOpenLink(aEvent));
>+    this.openNodeIn(aNode, this._getCurrentActiveWin().whereToOpenLink(aEvent, false, false, /^javascript/i.test(aNode.uri)));

This line is too long. I would |let where = ...| and then use that in openNodeIn.

Switching review to Dao since he's done much of the app tabs work.
Attachment #463277 - Flags: review?(gavin.sharp) → review?(dao)
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #2)
> > > If so perhaps it would make sense to have this patch handle that
> > > as well?
> > 
> > Probably best to do that in a new bug. This bug is about bookmarks
> > specifically.
> 
> In my current patch I replaced some code in urlbarBindings.xml and search.xml
> to force the behavior when clicking the search button or go button (but not
> when using enter to submit as that piece of code doesn't call whereToOpenLink)
> to be the same as it was pre-patch. To me it seems strange for this patch to
> contain this code change if a future patch is just going to reverse it.

Ahh yes you did. I thought it was clicking elsewhere when I looked at it (didn't look too closely). That part probably needs to go. I think we'll want to do a more detailed specification of what happens when navigating via the urlbar or links (see bug 575561). Maybe you're right though. I'm sure Dao will say something about it and perhaps we should get some UX in here.
Comment on attachment 463277 [details] [diff] [review]
Patch + test

>-function whereToOpenLink( e, ignoreButton, ignoreAlt )
>+function whereToOpenLink( e, ignoreButton, ignoreAlt, noPinnedForce )
> {
>   // This method must treat a null event like a left click without modifier keys (i.e.
>   // e = { shiftKey:false, ctrlKey:false, metaKey:false, altKey:false, button:0 })
>   // for compatibility purposes.
>   if (!e)
>     return "current";
> 
>   var shift = e.shiftKey;
>@@ -145,16 +147,19 @@ function whereToOpenLink( e, ignoreButto
>     return shift ? "tabshifted" : "tab";
> 
>   if (alt)
>     return "save";
> 
>   if (shift || (middle && !middleUsesTabs))
>     return "window";
> 
>+  if (gBrowser.selectedTab.pinned && !noPinnedForce)
>+    return "tab";

gBrowser doesn't always exist where this file is used. See <http://mxr.mozilla.org/mozilla-central/search?string=utilityOverlay.js>. You need to use getTopWin().

That said, instead of modifying the return value of whereToOpenLink, can we have openUILinkIn not use the current tab if it's pinned and the domain of the passed URI doesn't match?
Attachment #463277 - Flags: review?(dao) → review-
Posted patch Patch v2 + Test v2 (obsolete) — Splinter Review
Attachment #463277 - Attachment is obsolete: true
Attachment #467254 - Flags: review?(dao)
Comment on attachment 467254 [details] [diff] [review]
Patch v2 + Test v2

>+function test() {
>+  let newTab = gBrowser.addTab("http://www.mozilla.org");
>+  waitForExplicitFinish();
>+  newTab.addEventListener("load", mainPart, true);

You're waiting for the favicon to load here, which you don't really want. newTab.linkedBrowser.addEventListener will actually wait for the content to load.

>+  function mainPart() {
>+    gBrowser.pinTab(newTab);
>+    
>+    openUILinkIn("javascript:var x=0;", "current");
>+    is(gBrowser.tabs.length, 2, "Should open in current tab");
>+    
>+    openUILinkIn("http://www.mozilla.org/1", "current");
>+    is(gBrowser.tabs.length, 2, "Should open in current tab");
>+    
>+    gBrowser.selectedTab = newTab;
>+    openUILinkIn("http://www.google.com/", "current");
>+    is(gBrowser.tabs.length, 3, "Should open in new tab");
>+    
>+    newTab.removeEventListener("load", mainPart, true);
>+    gBrowser.removeTab(newTab);
>+    gBrowser.removeTab(gBrowser.tabs[1]); // Google tab
>+    finish();

Tests shouldn't query online resources -- use example.org and example.com instead of mozilla.org and google.com.

>--- a/browser/base/content/utilityOverlay.js
>+++ b/browser/base/content/utilityOverlay.js

>+  if (where == "current" && w.gBrowser.selectedTab.pinned) {
>+    let ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService);
>+    let uriObj = ioService.newURI(url, null, null);
>+    if (!uriObj.schemeIs("javascript") &&
>+        w.gBrowser.currentURI.host != uriObj.host) {
>+      where = "tab";
>+    }
>+  }

nit: use Services.io instead of getService

newURI can throw as well as uri.host, I think, so I think you'll need to wrap most of this in try/catch.

Looks good overall, but I'd like to see a new patch that addresses these things.
Attachment #467254 - Flags: review?(dao) → review-
Assignee: nobody → sindrebugzilla
Status: NEW → ASSIGNED
Posted patch Patch v3 + Test v3 (obsolete) — Splinter Review
Attachment #467254 - Attachment is obsolete: true
Attachment #467384 - Flags: review?(dao)
Comment on attachment 467384 [details] [diff] [review]
Patch v3 + Test v3

>+  if (where == "current" && w.gBrowser.selectedTab.pinned) {
>+    try {
>+      let uriObj = Services.io.newURI(url, null, null);
>+      if (!uriObj.schemeIs("javascript") &&
>+          w.gBrowser.currentURI.host != uriObj.host) {
>+        where = "tab";
>+      }
>+    } catch (err) {
>+    }

set where = "tab" when catching the exception

thanks!
Attachment #467384 - Flags: review?(dao) → review+
Attachment #467384 - Attachment is obsolete: true
Attachment #467886 - Flags: review?(dao)
Attachment #467886 - Flags: review?(dao)
Attachment #467886 - Flags: review+
Attachment #467886 - Flags: approval2.0?
Comment on attachment 467886 [details] [diff] [review]
Patch v4 + Test v3

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>+  if (where == "current" && w.gBrowser.selectedTab.pinned) {
>+    try {
>+      let uriObj = Services.io.newURI(url, null, null);
>+      if (!uriObj.schemeIs("javascript") &&
>+          w.gBrowser.currentURI.host != uriObj.host) {

Shouldn't this compare prePath rather than host? port/scheme differences can matter (likely only in corner cases, granted).
(In reply to comment #25)
> >+  if (where == "current" && w.gBrowser.selectedTab.pinned) {
> >+    try {
> >+      let uriObj = Services.io.newURI(url, null, null);
> >+      if (!uriObj.schemeIs("javascript") &&
> >+          w.gBrowser.currentURI.host != uriObj.host) {
> 
> Shouldn't this compare prePath rather than host? port/scheme differences can
> matter (likely only in corner cases, granted).

In the light of bug 575561, I don't think we want to differentiate between https and http for instance.
(In reply to comment #26)
> (In reply to comment #25)
> > >+  if (where == "current" && w.gBrowser.selectedTab.pinned) {
> > >+    try {
> > >+      let uriObj = Services.io.newURI(url, null, null);
> > >+      if (!uriObj.schemeIs("javascript") &&
> > >+          w.gBrowser.currentURI.host != uriObj.host) {
> > 
> > Shouldn't this compare prePath rather than host? port/scheme differences can
> > matter (likely only in corner cases, granted).
> 
> In the light of bug 575561, I don't think we want to differentiate between
> https and http for instance.

Should I use hostPort instead of host then?
Posted patch Patch v5 + Test v4 (obsolete) — Splinter Review
Attachment #467886 - Attachment is obsolete: true
Attachment #468280 - Flags: review?(dao)
Attachment #467886 - Flags: approval2.0?
Attachment #468280 - Attachment is obsolete: true
Attachment #468280 - Flags: review?(dao)
Posted patch Patch v6 + Test v4 (obsolete) — Splinter Review
Attachment #468282 - Flags: review?(dao)
Comment on attachment 468282 [details] [diff] [review]
Patch v6 + Test v4

While https<->http is probably the only transition we care about in practice, I'm not sure why we would disallow other scheme transitions...
I don't have an opinion on the port.

I think we should just take v4 for now.
Attachment #468282 - Flags: review?(dao)
Attachment #467886 - Attachment is obsolete: false
Attachment #467886 - Flags: approval2.0?
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Attachment #467886 - Flags: approval2.0?
Attachment #468282 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/0152fbafc520
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Component: Tabbed Browser → General
Flags: in-testsuite+
Keywords: checkin-needed
QA Contact: tabbed.browser → general
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b6
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100901 Firefox/4.0b6pre

The automated test doesn't interact with the real elements. So an additional Mozmill test wouldn't be that bad.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Whiteboard: [mozmill-test-needed]
Blocks: 594131
https://litmus.mozilla.org/show_test.cgi?id=12780 added to Litmus.
Flags: in-litmus? → in-litmus+
Whiteboard: [mozmill-test-needed] → [mozmill-test-needed][in-litmus-bug-week]
Duplicate of this bug: 597815
Blocks: 590972
Depends on: 600883
Depends on: 633260
Thank you for having taken care of this :)
Depends on: 908968
You need to log in before you can comment on or make changes to this bug.