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

REOPENED
Assigned to

Status

()

REOPENED
8 years ago
3 years ago

People

(Reporter: luke.iliffe, Assigned: dao)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
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
Last Resolved: 8 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
(Reporter)

Comment 2

8 years ago
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

Comment 3

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

Comment 4

8 years ago
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+

Updated

8 years ago
Assignee: nobody → margaret.leibovic

Comment 11

8 years ago
Created attachment 497365 [details] [diff] [review]
patch
Attachment #497365 - Flags: review?(dao)
(Assignee)

Comment 12

8 years ago
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-

Comment 13

8 years ago
Created attachment 497399 [details] [diff] [review]
patch v2
Attachment #497365 - Attachment is obsolete: true
Attachment #497399 - Flags: review?(dao)

Comment 14

8 years 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();
(Assignee)

Updated

8 years ago
Depends on: 610203

Comment 15

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

Comment 16

8 years ago
(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.

Updated

8 years ago
Whiteboard: [needs review dao]

Comment 17

8 years ago
(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
(Assignee)

Comment 18

8 years ago
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-

Updated

8 years ago
Whiteboard: [needs review dao][softblocker] → [softblocker][waiting on 610203]

Comment 19

8 years ago
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+
(Assignee)

Updated

8 years ago
Duplicate of this bug: 647411

Updated

8 years ago
Duplicate of this bug: 658449
(Assignee)

Updated

7 years ago
Duplicate of this bug: 674821
Dragging an external file should not overwrite App Tabs. Does this bug cover that?

Comment 26

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

Comment 27

7 years ago
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)

Updated

7 years ago
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

Updated

7 years ago
Whiteboard: [softblocker][waiting on 610203][final?] → [may be fixed by 674161]
No longer depends on: 674161

Updated

7 years ago
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.

Comment 31

7 years ago
(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)

Updated

7 years ago
Assignee: nobody → dao
(Assignee)

Comment 32

7 years ago
Created attachment 607925 [details] [diff] [review]
patch
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 }
(Assignee)

Comment 34

7 years ago
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+
(Assignee)

Comment 36

7 years ago
Created attachment 618032 [details] [diff] [review]
patch v2

(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)
(Assignee)

Comment 37

7 years ago
Created attachment 618033 [details] [diff] [review]
patch v2
Attachment #618032 - Attachment is obsolete: true
Attachment #618033 - Flags: review?(gavin.sharp)
Attachment #618032 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
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 39

6 years ago
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 40

6 years ago
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.

Comment 42

6 years ago
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)
You need to log in before you can comment on or make changes to this bug.