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

Back button is broken in iOS since 1.5.0 #574

Closed
powerbuoy opened this issue Jan 25, 2018 · 49 comments
Closed

Back button is broken in iOS since 1.5.0 #574

powerbuoy opened this issue Jan 25, 2018 · 49 comments
Assignees

Comments

@powerbuoy
Copy link

Since the latest update that introduced the very useful isNavigating* flags the back button is broken in iOS and seems to always take the user back to the home page.

To reproduce simply set up an Aurelia CLI app with at least 3 routes, then navigate from home, to route 2 to route 3 then hit the back button and you'll end up on home - not route 2.

@davismj
Copy link
Member

davismj commented Jan 26, 2018

Thanks for bringing this to my attention. I'll take a look when I can get my hands on an iOS device.

Any idea what might be going on?

@powerbuoy
Copy link
Author

Unfortunately no :/ But after further testing it does seem like the back button always brings you back to the home page for some reason.

window.history.length seems correct (there's more than one entry in there at least) but window.history.go(-1) always takes you to the home page...

Same issue in the iOS simulator if you have access to an OSX machine.

@powerbuoy
Copy link
Author

If there's anything I can help you test just let me know and I'll do my best!

@davismj
Copy link
Member

davismj commented Jan 26, 2018

I've got an old iPod touchy for testing, but I gotta get a charger for it lol. Appreciate the leads.

@powerbuoy
Copy link
Author

powerbuoy commented Jan 26, 2018 via email

@powerbuoy
Copy link
Author

Can confirm it's definitely a problem with aurelia-router@1.5.0 - rolling back to 1.4.0 solves the issue.

@jwx
Copy link
Member

jwx commented Jan 26, 2018

Could it have something to do with routes not being added to browser history?

I don't think anything I've added has caused this (but yeah, if I had a nickel for every time...) but I have to admit I haven't tested my PRs on iOS on account of not having it available. 😊

@jwx
Copy link
Member

jwx commented Jan 26, 2018

(That blush looked way too happy. It's supposed to be a sad and embarassed blush, guys.)

@jerep
Copy link

jerep commented Jan 31, 2018

I have the exact same issue with Safari on Mac and downgrading to aurelia-router@1.4.0 solved it.

@3cp
Copy link
Member

3cp commented Feb 5, 2018

I also encountered this problem on macOS Safari. You don't need an iOS device to debug.

@jdedwards3
Copy link

jdedwards3 commented Feb 5, 2018

Possibly a related issue? I ran into both at the same time.

#528

Can anyone determine if downgrading the router to v1.4 fixes issue 528 as well?

@powerbuoy
Copy link
Author

@jimwards17 #528 was reported before 1.5.0 released afaik.

@powerbuoy
Copy link
Author

So... not to be that guy, but is anyone working on this? It's a pretty huge bug that's currently live and out there in the latest release of aurelia-router...

@davismj davismj self-assigned this Feb 7, 2018
@davismj
Copy link
Member

davismj commented Feb 7, 2018

All right so I got my touchy charged up and create a test repo: http://davismj.me/test-router/

I navigate Welcome > Github Users > Child Router. I click the back button. I'm at Github Users. This is the expected behavior.

I'll need a repro to move forward, @powerbuoy .

@powerbuoy
Copy link
Author

powerbuoy commented Feb 7, 2018

@davismj Cheers :) My guess is it doesn't affect hashChange apps because your example works in Safari for me. Here's a newly created Aurelia CLI app that exhibits the bug though.

route-test.zip

And here's a video: https://youtu.be/p-BHoAm6qNo and a live demo: http://route-test.lagerkvist.eu/

@3cp
Copy link
Member

3cp commented Feb 7, 2018

@davismj

  1. test on a Mac Safari should be easier for you.
  2. I confirmed @powerbuoy's finding, you need to turn on pushState in router.config.options.pushState = true;.
  3. in addition, you need <base href="/"> in index.html, and set config.options.root = "/";, in order to reproduce the issue. Anyway, they are mandatory steps to turn on pushState, based on aurelia-router official document.

@davismj
Copy link
Member

davismj commented Feb 7, 2018

Got it, so it is a push state issue. I'll take another look.

@huochunpeng no access to a mac sadly

I personally never use pushState in apps. Is it that popular of an option?

@3cp
Copy link
Member

3cp commented Feb 7, 2018

@davismj, to be honest, you might be the only one left using hash instead of push. :-)

@davismj
Copy link
Member

davismj commented Feb 7, 2018

@huochunpeng are there any errors logged to the console in safari? that would be a huge help to me

@3cp
Copy link
Member

3cp commented Feb 7, 2018

@davismj I will check tomorrow once I got to office.

@powerbuoy
Copy link
Author

There are no errors in the Safari console. window.history looks like this:

screen shot 2018-02-07 at 15 05 44

if that helps at all.

@mattgaspar
Copy link

mattgaspar commented Feb 7, 2018

This bug does occur when using hash change.
I noticed that back actually removes the hash from the url. Normally a link to a route of '' would normal change the browser address to /#/, but when pressing back the url changes to /

I heard mention on github of a feature that would prevent the back navigation from browsing to a previous site (different host). Did that get implemented in 1.5?

@davismj - I was going to take a look at the router code but I couldn't figure out why there are type definitions in the .js files. VS Code shows syntax errors all over! Is there an extension to make this work? Is there a document on the IDE setup for contributing to Aurelia? I couldn't find anything.

@3cp
Copy link
Member

3cp commented Feb 7, 2018

@davismj zero error in Safari console for me too. Even with developmentLogging turned on, no extra debug/log/warn/error.

@davismj
Copy link
Member

davismj commented Feb 8, 2018

Appreciate the feedback guys. I'll do what I can.

@mattgaspar Thanks for the insight, both on the bug and on contributing. I use sublime text and so I've never seen errors and never thought twice about it. I'll take another look and maybe look at adding documentation about contributing. In the mean time, check out the flow types plugin for vs code.

The typings exist for the generation for .dts files. They are technically TypeScript typings but when we build the JavaScript we use Babel and the flow plugin.

@3cp
Copy link
Member

3cp commented Feb 9, 2018

@davismj I confirm the issue come from

this.history.setState('NavigationTracker', navtracker);

Once you comment out that line, on more issue on Safari. It looks like Safari is not happy if you touch history state object.

@3cp
Copy link
Member

3cp commented Feb 9, 2018

Bit more information, the additional field NavigationTracker that setState added to current history state object, it doesn't survive through history.back().

Which means the value your get from history state after history.back() is always undefined, leave you some logic holes when comparing with currentNavigationTracker.

@3cp
Copy link
Member

3cp commented Feb 9, 2018

Based on what I read from https://www.w3.org/TR/html50/browsers.html#history

7. Set history.state to a structured clone of cloned data.

Both history.pushState and history.replaceState need to CLONE the input state object.

Although the standard does not explicitly reject you from touching the cloned object, but to me it's common sense that the standard tries to protect the state object as immutable.

In this sense, Safari looks like the only browser strictly follow the standard.

@davismi it sounds likes Aurelia team need to revert getState/setState in aurelia-history 1.1.0, and find another way to implement the new router flags isNavigatingForward/Back/....

@jwx I should read your implementation before I say something stupid out of imagination. (The first half was not wrong but irrelevant).

@davismj
Copy link
Member

davismj commented Feb 9, 2018

@huochunpeng Thanks for this, great info and saved me a lot of headache since I don't have a Safari machine.

I think getState and setState are still likely viable strategies, but my guess is that there is a problem in the implementation. If you were not able to modify the state variables at all, then the state variables are totally useless. If you are able to modify them, then it seems we're just doing it in a way that safari doesn't like and we just need to fix that.

My guess is that the problem is here. I think maybe Safari doesn't like not having a title in the replaceState method.
https://github.com/aurelia/history-browser/blob/master/src/index.js#L192

I'm reaching out to you on Gitter now. For additional info.

@jwx
Copy link
Member

jwx commented Feb 9, 2018

I'm the one that added the use of the state object and as far as I interpreted https://www.w3.org/TR/html50/browsers.html#state-object it's intended to support the kind of usage implemented. There might be something wrong with the implementation, though (especially since https://caniuse.com/#search=replaceState says it shold be okay to use), and @davismj might have found the culprit. I might be able to take a look at this during the weekend, but I don't have iOS around so if someone could then help out with testing that'd be great.

@davismj
Copy link
Member

davismj commented Feb 9, 2018

@huochunpeng and I are working on it. I'll keep everyone posted here.

@jwx
Copy link
Member

jwx commented Feb 9, 2018

@davismj
Copy link
Member

davismj commented Feb 9, 2018

Unlikely, the third parameter is optional.

@jwx
Copy link
Member

jwx commented Feb 9, 2018

I think we're already in the realm of Unlikely since such a fairly simple thing is not working. But technically, the third parameter isn't left out; it's set to null. And that might make a difference in Safari/iOS. Even though it's a bit ... unlikely.

davismj added a commit to aurelia/history-browser that referenced this issue Feb 9, 2018
There is currently a bug in Safari that doesn't properly handle the optional third parameter to the replaceState(state, title, url) function when a <base /> tag is present on the page. This fix explicitly sets the url property to the location.href value.

Thanks to @powerbuoy @huochunpeng and @jwx for helping diagnose this issue!

Closes #34 aurelia/router#574
@davismj
Copy link
Member

davismj commented Feb 9, 2018

@huochunpeng #574 (comment)

Seems like Safari isn't strictly following the standard after all. Thanks again for debugging and finding the fix.

@3cp
Copy link
Member

3cp commented Feb 9, 2018

:-)

@powerbuoy
Copy link
Author

Awesome work guys!

@powerbuoy
Copy link
Author

Any ETA on a 1.5.1 release with this fix? :) The biggest use-case for me with isNavigatingBack is being able to restore the scroll position when the user goes back. We have a list of search results and it's not great UX to end up at the top of the list every time the user goes back to it.

@EisenbergEffect
Copy link
Contributor

@davismj Let me know when you're ready for me to do a new release.

@pndewit
Copy link
Contributor

pndewit commented Feb 20, 2018

I can confirm that the PR fixes my issues, with navigating back on Safari in macOS, as well. Looking forward to this being merged and released!

@davismj
Copy link
Member

davismj commented Feb 20, 2018

@EisenbergEffect can be pulled in, see PR comments

@powerbuoy
Copy link
Author

Any chance we'll see a release this week?

@EisenbergEffect
Copy link
Contributor

Yes. @davismj Can you go ahead and merge this? After that, just give me the all clear for a release and it will go out this weekend.

@pndewit
Copy link
Contributor

pndewit commented Feb 23, 2018

@EisenbergEffect, @davismj already merged the PR :)

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Feb 24, 2018

LOL That's what I get for being in a rush and not reading everything carefully! We'll get it out this weekend.

@davismj davismj closed this as completed Feb 24, 2018
@davismj
Copy link
Member

davismj commented Feb 24, 2018

https://github.com/aurelia/history-browser/releases/tag/1.1.1

@jdedwards3
Copy link

jdedwards3 commented May 4, 2018

this is still not working as expected on safari mobile

@powerbuoy
Copy link
Author

@jimwards17 care to elaborate?

@jdedwards3
Copy link

jdedwards3 commented May 4, 2018

aurelia/history-browser#40

I thought this was a router issue but it looks like @davismj updated the hsitory browser package. I'm not really sure where it makes the most sense to post this isssue, but i'm running latest for all packages and im still seeing this issue.

Also on any device including desktop the confirm prompt is presented but if you select cancel and then click the back button again and then select ok in the confirm prompt you are brought two history locations back. This might be two separate issues but the browser history not being maintained is pretty big...

@davismj
Copy link
Member

davismj commented May 12, 2018

@jimwards17 that is a known issue: #528

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

No branches or pull requests

9 participants