1228547 - Don't reset the UA when reloading tabs

Status: VERIFIED FIXED

(Firefox for iOS :: General, defect)

Description

[Catalin Suciu [:csuciu]]

Build: 1303

1. Visit ziare.com
2. 'Request desktop site'
3. Reload the page, or tap on a link to open an article in the same tab

Result: The mobile version of the site is offered

Comment 1

[Johannes Marbach]

Attachment: Pull request

Comment 2

[Aaron Train [:aaronmt]]

Probably needs a UX thought +CC :tecgirl

Comment 3

[Aaron Train [:aaronmt]]

For clarification, Safari does not reset the UA on reload and continues to hold onto the desktop UA on new links clicked (except for opening links in a new tab). I suppose we should do that.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This does not work correctly.

In webView(webView: WKWebView, didFinishNavigation navigation: WKNavigation!) we are doing the following:

        // Remember whether or not a desktop site was requested and reset potentially spoofed user agent
        if #available(iOS 9, *) {
            tab.desktopSite = webView.customUserAgent?.isEmpty == false
            webView.customUserAgent = nil
        }

This will always reset the custom user agent for the next request. I don't think that is the behaviour we want.

I think we want the same thing as Safari does, which is this:

* When you RDS, the current page is reloaded with a Desktop user agent
* When you open a new link in the same tab (that now has RDS enabled) for the *same* site, we need to stay in RDS.
* When you open a new link the same tab (that now has RDS enabled) for a *different* site, we need to reset RDS

So you need to keep track of the page URL and decide to disable RDS probably before each request starts in webView(webView: WKWebView, decidePolicyForNavigationAction navigationAction: WKNavigationAction, decisionHandler: (WKNavigationActionPolicy) -> Void)

Here is a good test case:

1) Open a new tab
2) Load Github.com - See that it loads the mobile site
3) Enable RDS - See that it reloads and github.com is now the desktop version
4) Tap on a link in the navigation bar like 'Explore' - See that it loads the Desktop version
5) In the same tab, open http://github.com/Alamofire/Alamofire from the location bar - See that it loads the Desktop version
6) On the Almofire Github page, scroll down to the Communiation section and tap the link to Stack Overflow - See that RDS is reset and that SO opens in Mobile mode

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Aaron, Safari does reset RDS if you go to a different web site on a tab where RDS is enabled. See test case above.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8693071 [details] [review]
Pull request

See my comment earlier. I think we need better Safari parity before this can land. I attached a good test case.

Comment 7

[Aaron Train [:aaronmt]]

(In reply to Stefan Arentz [:st3fan] from comment #5)
> Aaron, Safari does reset RDS if you go to a different web site on a tab
> where RDS is enabled. See test case above.

I meant links clicked through the same domain, however you are correct on the above as I just tested on my iPhone on Safari.

Comment 8

[Johannes Marbach]

Thanks for the feedback on the pull request, Stefan. I think that always resetting the UA for the next request is exactly what we agreed on in Bug 1109675 (specifically in the comments on GitHub). But you are right in that it should be refined. I can look into updating the pull request with the suggested changes.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8693071 [details] [review]
Pull request

(In reply to hennes from comment #8)
> Thanks for the feedback on the pull request, Stefan. I think that always
> resetting the UA for the next request is exactly what we agreed on in Bug
> 1109675 (specifically in the comments on GitHub).

Yep, I was unfamiliar with Safari's behavior myself, so I apologize for not giving more direction in bug 1109675. But no worries -- I think your code that's already landed was a good first iteration; we want to land/test features rapidly so we can find precisely the kinds of follow-up bugs we have here.

Comment 10

[Johannes Marbach]

Yes, that makes perfect sense. :)

I updated the pull request and the behavior should comply with Stefan's test case now. Brian, Stefan, I added both of you as reviewers because I wasn't sure whom to pick. I guess one reviewer will be enough but I'll let you guys work it out.

Comment 11

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

hennes, this works really well. fantastic.

Comment 12

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Brian if you are happy with the code then we should land it and still ship it as part of 1.3!

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 8693071 [details] [review]
Pull request

Looks great, thanks! Left a few minor comments in the PR.

Comment 14

[Johannes Marbach]

Thanks for the feedback. I updated the PR with the small changes.

Comment 15

[Brian Nicholson (:bnicholson)]

Thanks! Landed:

master: 531f59c
v1.x: d7255be