Open Bug 598587 Opened 10 years ago Updated 1 year ago

Prevent pinned tabs from being overwritten by external urls entered from URL bar (but open the URL in new normal tab)

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

()

REOPENED
Tracking Status
blocking2.0 --- .x+

People

(Reporter: luke.iliffe, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
Blocks: 579874
The search use case should work now (it does for me).

As for the urlbar nav case... that's being handled in bug 575561
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Summary: Prevent App Tabs from being overwritten by external urls opened from URL or search bar - open in new tab instead. → Prevent App Tabs from being overwritten by external urls opened from URL - open in new tab instead.
Duplicate of 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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Prevent App Tabs from being overwritten by external urls opened from URL - open in new tab instead. → Prevent App Tabs from being overwritten by external urls entered from URL bar
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.
blocking2.0: --- → ?
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-.
blocking2.0: ? → -
Keywords: uiwanted
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.
blocking2.0: - → ?
Keywords: uiwanted
UX says this change is fundamental to the nature of the feature, so blocking+.
blocking2.0: ? → betaN+
Assignee: nobody → margaret.leibovic
Attached patch patch (obsolete) — Splinter Review
Attachment #497365 - Flags: review?(dao)
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.
Attachment #497365 - Flags: review?(dao) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #497365 - Attachment is obsolete: true
Attachment #497399 - Flags: review?(dao)
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();
Depends on: 610203
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.
Whiteboard: [needs review dao]
(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').
Whiteboard: [needs review dao] → [needs review dao][softblocker]
Hardware: x86 → All
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.
Attachment #497399 - Flags: review?(dao) → review-
Whiteboard: [needs review dao][softblocker] → [softblocker][waiting on 610203]
Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.
Whiteboard: [softblocker][waiting on 610203] → [softblocker][waiting on 610203][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
blocking2.0: betaN+ → .x+
Duplicate of this bug: 647411
Duplicate of this bug: 658449
Duplicate of this bug: 674821
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.
Summary: Prevent App Tabs from being overwritten by external urls entered from URL bar → Prevent App Tabs from being overwritten by external urls entered from URL bar (but open the URL in new normal tab)
Duplicate of this bug: 688499
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)
Depends on: 674161
No longer depends on: 610203
Whiteboard: [softblocker][waiting on 610203][final?] → [may be fixed by 674161]
No longer depends on: 674161
Whiteboard: [may be fixed by 674161]
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.
Assignee: margaret.leibovic → nobody
Assignee: nobody → dao
Attached patch patch (obsolete) — Splinter Review
Attachment #497399 - Attachment is obsolete: true
Attachment #607925 - Flags: review?(gavin.sharp)
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?
Attachment #607925 - Flags: review?(gavin.sharp) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #607925 - Attachment is obsolete: true
Attachment #618032 - Flags: review?(gavin.sharp)
Attached patch patch v2Splinter Review
Attachment #618032 - Attachment is obsolete: true
Attachment #618033 - Flags: review?(gavin.sharp)
Attachment #618032 - Flags: review?(gavin.sharp)
Attachment #618033 - Flags: review?(ttaubert)
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)
Attachment #618033 - Flags: review?(ttaubert) → feedback+
Summary: Prevent App Tabs from being overwritten by external urls entered from URL bar (but open the URL in new normal tab) → Prevent pinned tabs from being overwritten by external urls entered from URL bar (but open the URL in new normal tab)
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)
Attachment #618033 - Flags: review?(gavin.sharp)
Duplicate of this bug: 1528767
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.