Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use original URI when switching UA on a page that was redirected #1542

Merged
merged 1 commit into from Aug 19, 2019

Conversation

bluemarvin
Copy link
Contributor

Partially fixes #1473

@bluemarvin
Copy link
Contributor Author

To test:

  1. Go to twitch.tv (note: gets redirected to m.twitch.tv)
  2. switch to desktop mode

URL should be www.twitch.tv (twitch.tv redirects to www on desktop)

Note for what will not work:

  1. Go to twitch.tv (note: gets redirected to m.twitch.tv)
  2. click on one of the videos
  3. Switch to desktop mode

The domain will remain m.twich.tv even in desktop mode.

This patch only works when switching UA on a domain that was redirected. Once you click a link, FxR no longer knows it has been redirected.

To address always dropping the m. prefix FxR would need to handle that on a per domain basis.

@keianhzo
Copy link
Collaborator

keianhzo commented Aug 9, 2019

@bluemarvin I've seen this when switch back and forth from VR to Desktop:

  1. Go to twitch.tv (note: gets redirected to m.twitch.tv)
  2. Click on one of the videos
  3. Switch to desktop mode
  4. Click on the username of the highlighted video
  5. Switch back to VR mode

Result: You go back to twitch.tv
Expected: You should reload the current site

@bluemarvin
Copy link
Contributor Author

That’s what fennec does too and it’s wrong. I’ll see if I can figure out what isn’t working.

@bluemarvin
Copy link
Contributor Author

This turning out to be more difficult that I first thought. The problem I am having is that in the twich.tv case specifically, it is a single page site. So we don't actually get onLoadRequest() for most link clicks on the site. so it is impossible to know when to not use the original URI that triggered the redirection. This also explains Fennec's similar behavior. What's needed is for onLocationChange() which does fire to include a parameter that indicates if the location change was caused by human interaction of clicking on a link or if it is just the page playing with the history.

@bluemarvin bluemarvin self-assigned this Aug 14, 2019
@bluemarvin
Copy link
Contributor Author

It was suggested I look at the history delegate to see if it is possible to determine what is causing the location change.

@bluemarvin bluemarvin force-pushed the desktop-ua-fix branch 2 times, most recently from 830fc6c to 78f96e2 Compare August 14, 2019 23:10
@bluemarvin
Copy link
Contributor Author

After trying a lot of different things with GeckoView, until GV can tell us if a visited URI was human initiated or script initiated there isn't a way to track redirects. This is a brute force method that just drops the m. prefix if the URI contains it before switching to desktop mode.

@keianhzo
Copy link
Collaborator

@bluemarvin LGTM just the merge conflicts and maybe we could also add support for mobile.? Twitter uses that instead of m. and probably some others.

if (overrideUri != null) {
mCurrentSession.loadUri(overrideUri, GeckoSession.LOAD_FLAGS_BYPASS_CACHE);
} else if (state.mUri != null){
mCurrentSession.loadUri(state.mUri, GeckoSession.LOAD_FLAGS_BYPASS_CACHE);;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: double ;at the end of this line.

@bluemarvin
Copy link
Contributor Author

After trying a lot of different things with GeckoView, until GV can tell us if a visited URI was human initiated or script initiated there isn't a way to track redirects. This is a brute force method that just drops the m. prefix if the URI contains it before switching to desktop mode.

It looks like twitter uses cookies. Even if I drop the mobile. prefix, it just comes back even with the desktop UA.

@keianhzo keianhzo merged commit b41a82c into master Aug 19, 2019
@bluemarvin bluemarvin deleted the desktop-ua-fix branch August 26, 2019 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The mobile version of a webpage is displayed even though the desktop user agent is selected
2 participants