An app tab's contents are overwritten if an address is typed in the url bar and enter pressed. The same goes for a search term in the search window. To preserve the app tabs contents a new tab should be opened instead.

The search use case should work now (it does for me). As for the urlbar nav case... that's being handled in bug 575561

Reopening this as bug 575561 is now fixed yet it seems, contrary to comment 1, the urlbar case was not part of the scope of that bug. Entering an external url in the urlbar on an apptab still causes the contents to be overwritten. Ignore the search bar component in comment 0 as this does work, this bug is exclusively about preventing apptabs being overwritten by entering URLs in the awesomebar.

If bug 607266 is implemented, you won't be able to enter a URL in the location bar, which would make this bug invalid.

Requesting blocking BetaN because chromeless apptabs have missed the feature freeze and this remains the last piece of primary UI from which it is easy to overwrite apptab contents. Completing this for the 4.0 release would make the apptab behaviour consistent. Other similar bugs such a Bug 579872, Bug 575561 and Bug 594131 had blocking+. I am used to searching the awesome bar a lot and I often find myself searching after reading email in an apptab. Rather than the results opening in a new tab it just overwrites my email. I know about alt+enter but I am not used to using it normally, and normally it is not required.

Would like UX direction here. There's no dataloss, and there is a workaround (go back), so not going to hold the release for this. Blocking-.

Yes, this should be prioritized and fixed, unless we can get chromeless app tabs into one of the betas, which doesn't seem likely at this point (but I'll let zpao comment on that).

And in case it wasn't clear from the bug, any URL entered in an app tab should open in a new, normal tab.

(In reply to comment #6) > Yes, this should be prioritized and fixed, unless we can get chromeless app > tabs into one of the betas, which doesn't seem likely at this point (but I'll > let zpao comment on that). Nope. Not happening.

So then we have to fix this, or else app tabs aren't app tabs anymore, they're just narrower normal tabs. :) This has been fixed for the search field, the URL field should behave the same way. Re-nominating.

UX says this change is fundamental to the nature of the feature, so blocking+.

Comment on attachment 497365 [details] [diff] [review] patch >--- a/browser/base/content/urlbarBindings.xml >+++ b/browser/base/content/urlbarBindings.xml >@@ -261,26 +261,29 @@ > // but don't let that interfere with the loading of the url. > Cu.reportError(ex); > } > > if (aTriggeringEvent instanceof MouseEvent) { > // We have a mouse event (from the go button), so use the standard > // UI link behaviors > let where = whereToOpenLink(aTriggeringEvent, false, false); >+ if (gBrowser.selectedTab.pinned) >+ where = "tab"; This should only be done for where == "current" (not for "window", "tabshifted" and "save"). >- if (aTriggeringEvent && aTriggeringEvent.altKey) { >+ if ((aTriggeringEvent && aTriggeringEvent.altKey) || >+ gBrowser.selectedTab.pinned) { > this.handleRevert(); > let prevTab = gBrowser.selectedTab; > if (isTabEmpty(prevTab)) > gBrowser.removeTab(prevTab); > content.focus(); > gBrowser.loadOneTab(url, { > postData: postData, > inBackground: false, Please update for bug 618942, which I landed a few minutes ago.

Comment on attachment 497399 [details] [diff] [review] patch v2 >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml >--- a/browser/base/content/urlbarBindings.xml >+++ b/browser/base/content/urlbarBindings.xml > let where = whereToOpenLink(aTriggeringEvent, false, false); >+ if (gBrowser.selectedTab.pinned && where == "current") >+ where = "tab"; It seems unnecessary. It would be done in openUILinkIn. And javascript url (bookmarklet) needs to be considered. > if (where != "current") { This check could be removed. Whether "current" or not, the following will be done. > this.handleRevert(); > content.focus(); > } > openUILinkIn(url, where, > { allowThirdPartyFixup: true, postData: postData }); > return; > } > >- if (aTriggeringEvent && >- aTriggeringEvent.altKey && >- !isTabEmpty(gBrowser.selectedTab)) { >+ if (gBrowser.selectedTab.pinned || >+ (aTriggeringEvent && >+ aTriggeringEvent.altKey && >+ !isTabEmpty(gBrowser.selectedTab))) { I would suggest replacing the following gBrowser.loadOneTab and loadURI with openUILinkIn(url, 'tab'/'current'). openUILinkIn will do the necessary checks. > this.handleRevert(); > content.focus(); > gBrowser.loadOneTab(url, { > postData: postData, > inBackground: false, > allowThirdPartyFixup: true}); > aTriggeringEvent.preventDefault(); > aTriggeringEvent.stopPropagation();

It would be better to make the logic here consistent with searchbar.handleSearchCommand.

(In reply to comment #14) > > if (where != "current") { > This check could be removed. Whether "current" or not, the following will be > done. Really? Why do you say that? I'm not sure if that's true. > I would suggest replacing the following gBrowser.loadOneTab and loadURI with > openUILinkIn(url, 'tab'/'current'). openUILinkIn will do the necessary checks. I feel like a change like that is out of the scope of this bug. (In reply to comment #15) > It would be better to make the logic here consistent with > searchbar.handleSearchCommand. Why? I don't see anything about app tabs in that method, so I don't see how that relates to this bug.

(In reply to comment #16) > > > if (where != "current") { > > This check could be removed. Whether "current" or not, the following will be > > done. > > Really? Why do you say that? I'm not sure if that's true. After the url is loaded, the urlbar is reverted and the focus is at the content even if you don't call gURLBar.handleRevert() and content.focus(). > > It would be better to make the logic here consistent with > > searchbar.handleSearchCommand. > > Why? I don't see anything about app tabs in that method, so I don't see how > that relates to this bug. That's why it's better. openUILinkIn will handle the app tab issue. Just call openUILinkIn(url, 'current').

Comment on attachment 497399 [details] [diff] [review] patch v2 ithinc is right, openUILink would handle this automatically. Also, I think this should wait for bug 610203, which severely changes this code and, depending on how the final patch will look, may even fix this bug.

Is there a good reason that this blocks betaN? If not, it should be moved over to final+.

bug 610203 is blocking final, not betaN, so either this patch should be blocking final too, or bug 610203 should be a betaN blocker.

** PRODUCT DRIVERS PLEASE NOTE ** This bug is one of 7 automatically changed from blocking2.0:betaN+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons: - it was marked as a soft blocking issue for a beta - the whiteboard indicated that it might be appropriate for a .x release

Dragging an external file should not overwrite App Tabs. Does this bug cover that?

(In reply to sdrocking from comment #25) > Dragging an external file should not overwrite App Tabs. Does this bug cover > that? Drag and drop should be covered by bug 579873.

Updating summary to distinguish bugs about LOCKING URL from bugs about REDIRECTING URL to new tabs.

This bug is only targeted for external URLs, why do URLs of the same domain open in new tabs on UX? (Happens with Paste-and-Go of next Nightly thread URL on mozillaZine)

So if this isn't just about locking copy paste in the location bar for app tabs (like popup windows tend to do), then there must be some way of inferring that app tabs are not registered tabs in the tab index available to update by external cases, essentially splitting the location bar into two different code paths: One that allows external updates and one that doesn't.

(In reply to Dennis "Dale" Y. [:cuz84d] from comment #30) > So if this isn't just about locking copy paste in the location bar for app > tabs (like popup windows tend to do), then there must be some way of > inferring that app tabs are not registered tabs in the tab index available > to update by external cases, essentially splitting the location bar into two > different code paths: One that allows external updates and one that doesn't. There are a whole bunch of different code paths for loading URLs, so it's not quite as simple as two different code paths. I'm un-assigning myself because my focus has changed, I haven't worked on this in a very long time.

Comment on attachment 607925 [details] [diff] [review] patch Review of attachment 607925 [details] [diff] [review]: ----------------------------------------------------------------- Just my 2 cents ::: browser/base/content/utilityOverlay.js @@ +235,5 @@ > function openLinkIn(url, where, params) { > if (!where || !url) > + return null; > + > + var rv = { where: where }; The use of rv feels kind of forced, especially since half the time we're setting with code like > rv.where = (where = "tab") @@ +250,5 @@ > var aIsUTF8 = params.isUTF8; > > if (where == "save") { > saveURL(url, null, null, true, null, aReferrerURI); > + return rv; Like right here, return { where: "save" }; @@ +292,5 @@ > sa.AppendElement(allowThirdPartyFixupSupports); > > Services.ww.openWindow(w || window, getBrowserURL(), > null, "chrome,dialog=no,all", sa); > + return rv; we know what where is, and even added code to explicitly set it when we could just return { where: "window" } @@ +362,5 @@ > > if (!loadInBackground && isBlankPageURL(url)) > w.focusAndSelectUrlBar(); > + > + return rv; return { where: where }

I wrote it this way in case we want to return more data in the future. Otherwuse we could just return 'where' directly.

Comment on attachment 607925 [details] [diff] [review] patch Can you write a test for this? >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml >+ let where; >+ else >+ where = "current"; Just initialize |where| to "current"? >+ let browser = gBrowser.selectedBrowser; >+ let rv = openUILinkIn(url, where, params); >+ if (rv.where != "current") { >+ if (browser == gBrowser.selectedBrowser) > this.handleRevert(); >+ else >+ browser.userTypedValue = null; You should add a comment explaning this logic. In particular, why you can't just unconditionally call handleRevert() (it operates on the current browser, and actual updating of of the URL bar is only necessary if the current tab is the one that triggered the load in a new window/tab). Is it possible for the Go button to be present or URL bar to be editable with if !toolbar.visible? If so, using openUILinkIn for "current" would result in an odd behavior. >diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js >- let uriObj = Services.io.newURI(url, null, null); >+ let uriObj; >+ if (aAllowThirdPartyFixup) { >+ let uriFixup = Cc["@mozilla.org/docshell/urifixup;1"].getService(Ci.nsIURIFixup); >+ uriObj = uriFixup.createFixupURI(url, uriFixup.FIXUP_FLAG_USE_UTF8); >+ } else { >+ uriObj = Services.io.newURI(url, null, null); >+ } Why this change?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #35) > Is it possible for the Go button to be present or URL bar to be editable > with if !toolbar.visible? nope > >diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js > > >- let uriObj = Services.io.newURI(url, null, null); > >+ let uriObj; > >+ if (aAllowThirdPartyFixup) { > >+ let uriFixup = Cc["@mozilla.org/docshell/urifixup;1"].getService(Ci.nsIURIFixup); > >+ uriObj = uriFixup.createFixupURI(url, uriFixup.FIXUP_FLAG_USE_UTF8); > >+ } else { > >+ uriObj = Services.io.newURI(url, null, null); > >+ } > > Why this change? Because newURI doesn't work for strings requiring fixup.

Comment on attachment 618033 [details] [diff] [review] patch v2 Review of attachment 618033 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me but I'm not at all familiar with all the urlbar stuff. I'd like to have Gavin give the final r+ for this (or anyone else that feels confident to do so). ::: browser/base/content/test/browser_urlbarPinnedTab.js @@ +6,5 @@ > + let pinnedTab = gBrowser.selectedTab; > + > + gBrowser.pinTab(pinnedTab); > + registerCleanupFunction(function () { > + gBrowser.pinTab(pinnedTab); Did you mean to unpin the tab here? @@ +12,5 @@ > + > + let originalURLBarValue = gURLBar.value; > + gBrowser.userTypedValue = "example.org"; > + URLBarSetURI(); > + gURLBar.handleCommand(); Is it maybe better to do > gURLBar.value = "example.org"; > gURLBar.focus(); > EventUtils.synthesizeKey("VK_RETURN", {}); here instead of accessing too much internals? ::: browser/base/content/utilityOverlay.js @@ +265,5 @@ > > + if (!w) > + rv.where = (where = "window"); > + > + if (where == "window") { I find that's quite hard to understand. Can't we leave it like before and just always set rv.where? > if (!w || where == "window") { > rv.where = "window"; > ... @@ +358,1 @@ > if (window == fm.activeWindow) How about: > if (window == Services.focus.activeWindow)

Comment on attachment 618033 [details] [diff] [review] patch v2 Review of attachment 618033 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/urlbarBindings.xml @@ +366,1 @@ > this.handleRevert(); Can handleRevert() be called directly in openLinkIn instead of return a result?

Comment on attachment 618033 [details] [diff] [review] patch v2 Review of attachment 618033 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/utilityOverlay.js @@ +359,5 @@ > w.content.focus(); > else > w.gBrowser.selectedBrowser.focus(); > > if (!loadInBackground && isBlankPageURL(url)) A mistake introduced otherwhere. isBlankPageURL should be w.isBlankPageURL

Why? There is a isBlankPageURL defined in utilityOverlay, and its return value is not window-specific.

Then that's no problem. I altered isBlankPageURL function in my extension, which made it window dependent.

Comment on attachment 618033 [details] [diff] [review] patch v2 (bitrotten, clearing old review request)