Ian Pottinger Reporter
	Description • 14 years ago

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.

	Biju
	Comment 1 • 14 years ago

But please make sure "Bookmarklets" run on same page with out any problem.

	Sindre Dammann Assignee
	Comment 2 • 14 years ago

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.

	Ian Pottinger Reporter
	Comment 3 • 14 years ago

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.

	Peter Henkel [:Terepin]
	Comment 4 • 14 years ago

(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.

	Biju
	Comment 5 • 14 years ago

(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

	Peter Henkel [:Terepin]
	Comment 6 • 14 years ago

Nothing. Chromeless means no chrome at all.

	Tan Wei Lin Jason [:Ryuji]
	Comment 7 • 14 years ago

No chrome mean you won't see any toolbar except for the Tab bar.

	Peter Henkel [:Terepin]
	Comment 8 • 14 years ago

Exactly.

	Tiago Sá
	Comment 9 • 14 years ago

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

	Biju
	Comment 10 • 14 years ago

(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

	Ian Pottinger Reporter
	Comment 11 • 14 years ago

(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.

	Tan Wei Lin Jason [:Ryuji]
	Comment 12 • 14 years ago

I read in Blair's blog that Proposed Chromeless AppTab are experimental and by no means final proposal for Firefox 4.

	Biju
	Comment 13 • 14 years ago

(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.

	Ian Pottinger Reporter
	Comment 14 • 14 years ago

(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

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 15 • 14 years ago

(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.

	Sindre Dammann Assignee
	Comment 16 • 14 years ago

(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.

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 17 • 14 years ago

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.

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 18 • 14 years ago

(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.

	Dão Gottwald [:dao]
	Comment 19 • 14 years ago

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?

	Dão Gottwald [:dao]
	Comment 21 • 14 years ago

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.

	Dão Gottwald [:dao]
	Comment 23 • 14 years ago

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!

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 25 • 14 years ago

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).

	Dão Gottwald [:dao]
	Comment 26 • 14 years ago

(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.

	Sindre Dammann Assignee
	Comment 27 • 14 years ago

(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?

	Dão Gottwald [:dao]
	Comment 30 • 14 years ago

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.

	Dão Gottwald [:dao]
	Comment 31 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/0152fbafc520

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 32 • 14 years ago

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.

	Marcia Knous [:marcia]
	Comment 33 • 14 years ago

https://litmus.mozilla.org/show_test.cgi?id=12780 added to Litmus.

	Marc Schumann [:Wurblzap]
	Comment 35 • 13 years ago

Thank you for having taken care of this :)