Shortcut pinned tabs never become unpinned when navigating to a very different page

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mfinkle, Assigned: Margaret, Mentored)

Tracking

unspecified
Firefox 42
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, fennec+)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

Bug 937253 added support for pinning a tab to a specific origin URL, used to switch back to the tab if the homescreen shortcut is used again. This keeps Firefox from opening new tabs from homescreen shortcut launches.

If a tab becomes pinned like this, we should probably unpin it if the user navigates to a very different page, or maybe types a new URL to any page.

Thoughts on the trigger?
1. Any onLocationChange where the URL domain is not the same as the origin URL?
2. Any manually typed onLocationChange?
Margaret has done similar work on Desktop with Pinned tabs.
Flags: needinfo?(margaret.leibovic)
Whiteboard: [lang=js]
(In reply to Mark Finkle (:mfinkle) from comment #1)
> Margaret has done similar work on Desktop with Pinned tabs.

The thing I did for desktop is just for link clicks, so it doesn't take into account changing the location from the location bar. We added logic in docshell for this because we wanted to catch this link click so that we could open a new tab before the location change happened (see bug 575561).

However, on Fennec, if we're cool with the location changing in the same tab, I would say we should just add something in our onLocationChange handler to check if the host changed, and if it did, unpin the tab.
Flags: needinfo?(margaret.leibovic)
Posted patch unpin WIP (obsolete) — Splinter Review
This patch will "unpin" the Tab if the new domain is not the same as the pinned domain. Needs some cleanup and more testing, maybe some optimization too.
Assignee: nobody → mark.finkle
We should fix this. I recently noticed that my homescreen bookmark will appear to stop working, because it will open a tab that I have navigated elsewhere.

Without an understanding of how this is implemented, that just makes Fennec seem busted.
tracking-fennec: --- → ?
Assignee: mark.finkle → margaret.leibovic
tracking-fennec: ? → 37+
Assignee: margaret.leibovic → mark.finkle
Posted patch unpin v0.1 (obsolete) — Splinter Review
This patch adds the required cleanup to SessionStore to support removing a TabValue. It also resets the TabValue for the "appOrigin" if we navigate to a new location without the same domain as the "app" ,effectively unpinning the Tab. As noted in the comments, using the BACK button will not make the Tab pinned again.
Attachment #8579822 - Attachment is obsolete: true
Attachment #8588152 - Flags: review?(margaret.leibovic)
Comment on attachment 8588152 [details] [diff] [review]
unpin v0.1

Review of attachment 8588152 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +4512,5 @@
> +      let originDomain = this._getDomainFromURL(originURL);
> +      if (originDomain != domain) {
> +        // Note: going 'back' will not make this tab pinned again
> +        ss.deleteTabValue(this, "appOrigin");
> +      }

I know it's an edge case, but what if a user navigates to a page that will fail to return a domain, such as an about: page. In that case, we wouldn't hit this logic. You might as well move this out of the if statement, since originalDomain != domain if the domain is null.

@@ +4559,5 @@
>  
> +  // For a given URL: http://www.example.com
> +  // return the domain: "www.example.com" or null if we can't get the domain
> +  _getDomainFromURL: function(url) {
> +    let parts = url.match(/^((?:[a-z]+:\/\/)?(?:[^\/]+@)?)(.+?)(?::\d+)?(?:\/|$)/);

Any reason not to create a new nsIURI and get the domain from that instead? It looks like this regex logic was just added for domain highlighting [1], and I suspect that's why we needed something more specific than just looking for the host. Although maybe we don't even need it for that?

I see that we do the same thing in aboutPasswords.js... if we change our mind here, we should change that as well.

[1] http://hg.mozilla.org/mozilla-central/rev/fa9887910dba
Comment on attachment 8588152 [details] [diff] [review]
unpin v0.1

Review of attachment 8588152 [details] [diff] [review]:
-----------------------------------------------------------------

f+ for now until we discuss this regex issue.
Attachment #8588152 - Flags: review?(margaret.leibovic) → feedback+
Maybe mcomella would be interested in pushing this across the finish line...
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mark.finkle)
tracking-fennec: 37+ → ?
Assignee: mark.finkle → s.kaspari
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mark.finkle)
tracking-fennec: ? → +
Assignee: s.kaspari → margaret.leibovic
(In reply to :Margaret Leibovic from comment #6)

> @@ +4559,5 @@
> >  
> > +  // For a given URL: http://www.example.com
> > +  // return the domain: "www.example.com" or null if we can't get the domain
> > +  _getDomainFromURL: function(url) {
> > +    let parts = url.match(/^((?:[a-z]+:\/\/)?(?:[^\/]+@)?)(.+?)(?::\d+)?(?:\/|$)/);
> 
> Any reason not to create a new nsIURI and get the domain from that instead?
> It looks like this regex logic was just added for domain highlighting [1],
> and I suspect that's why we needed something more specific than just looking
> for the host. Although maybe we don't even need it for that?
> 
> I see that we do the same thing in aboutPasswords.js... if we change our
> mind here, we should change that as well.
> 
> [1] http://hg.mozilla.org/mozilla-central/rev/fa9887910dba

Looking into this a bit more, I don't think we should use this regex for this. It's really designed for highlighting, not for functional host comparisons. For example, "http://bar.com" will return the same host as "file://bar.com", when the latter doesn't actually have a host. This is a silly example, but I think it would be more robust to just use nsIURI.host.
Bug 1132591 - Unpin shortcut tabs when navigating to a new host. r=mcomella
Attachment #8634835 - Flags: review?(michael.l.comella)
Attachment #8588152 - Attachment is obsolete: true
(In reply to :Margaret Leibovic from comment #9)
> For example, "http://bar.com" will return the same host as
> "file://bar.com", when the latter doesn't actually have a host.

To my understanding, file URIs skip over the host field - i.e. in your example, it should be `file:///bar.com`, where the third / skips the host and starts the path: `<scheme>://<host>/<path>`

But I agree - using URI parsing APIs sounds more robust.
Comment on attachment 8634835 [details]
MozReview Request: Bug 1132591 - Unpin shortcut tabs when navigating to a new host. r=mcomella

https://reviewboard.mozilla.org/r/13453/#review12335

Looks good though it'd be good to address the sub-domain issue.

::: mobile/android/components/SessionStore.js:1211
(Diff revision 1)
> -      throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
> +    }

Why do we no longer need to return this error code?

::: mobile/android/components/SessionStore.js:1210
(Diff revision 1)
> -    else
> +      this.saveStateDelayed();

This makes sense in my head: we change session state so we need to save the new session state. Is there anything more to it?

::: mobile/android/chrome/content/browser.js:4655
(Diff revision 1)
> +      if (originHost != aLocationURI.host) {

This will be true for sub-domains, clearing the pinned state. Is that intended?

I would prefer if this did not affect changed subdomains, just the main domain.
Attachment #8634835 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #12)
> Comment on attachment 8634835 [details]
> MozReview Request: Bug 1132591 - Unpin shortcut tabs when navigating to a
> new host. r=mcomella

I added the session store changes, so:

> ::: mobile/android/components/SessionStore.js:1211
> (Diff revision 1)
> > -      throw (Components.returnCode = Cr.NS_ERROR_INVALID_ARG);
> > +    }
> 
> Why do we no longer need to return this error code?

We don't want to always check for existence before calling this code to avoid an exception, and desktop doesn't throw either.

> ::: mobile/android/components/SessionStore.js:1210
> (Diff revision 1)
> > -    else
> > +      this.saveStateDelayed();
> 
> This makes sense in my head: we change session state so we need to save the
> new session state. Is there anything more to it?

Nope. That's it. We never called this method before, so it was buggy.

> 
> ::: mobile/android/chrome/content/browser.js:4655
> (Diff revision 1)
> > +      if (originHost != aLocationURI.host) {
> 
> This will be true for sub-domains, clearing the pinned state. Is that
> intended?
> 
> I would prefer if this did not affect changed subdomains, just the main

Keep in mind that m.twitter.com != twitter.com

I'd rather "unpin" if anything about the host is different.
> domain.
(In reply to Mark Finkle (:mfinkle) from comment #13)
> > I would prefer if this did not affect changed subdomains, just the main
> 
> Keep in mind that m.twitter.com != twitter.com

But calendar.google.com ~= google.com/calendar

But to be fair, this automatically redirects so I'm not sure how one might have created a calendar.google.com homescreen shortcut. I'm pretty sure I've seen pages where each link opens a new subdomain with related content though I can't find any at the moment.

I'll defer to your judgment.
(In reply to Michael Comella (:mcomella) from comment #14)
> I'm pretty sure I've seen pages where each link opens a new subdomain with
> related content though I can't find any at the moment.

Oh - Bugzilla!

reviewboard.mozilla.org from bugzilla.mozilla.org. Similarly, viewing attachments opens them in a new sub-domain. But then again, navigating to mozilla.org would be a completely different experience.

The majority case probably doesn't use subdomains for related content so perhaps you are right.
url:        https://hg.mozilla.org/integration/fx-team/rev/09d779eb4be2c328c5d2b9c42e4b25a0d8f954ad
changeset:  09d779eb4be2c328c5d2b9c42e4b25a0d8f954ad
user:       Margaret Leibovic <margaret.leibovic@gmail.com>
date:       Thu Jul 16 11:01:03 2015 -0700
description:
Bug 1132591 - Unpin shortcut tabs when navigating to a new host. r=mcomella
https://hg.mozilla.org/mozilla-central/rev/09d779eb4be2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.