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

AMP Player: Cannot manipulate history API as it causes issues #31010

Closed
rnkimani opened this issue Nov 5, 2020 · 14 comments · Fixed by #32153
Closed

AMP Player: Cannot manipulate history API as it causes issues #31010

rnkimani opened this issue Nov 5, 2020 · 14 comments · Fixed by #32153

Comments

@rnkimani
Copy link

rnkimani commented Nov 5, 2020

What's the issue?

  • I have been trying to implement custom functionality using the history API but it has issues.
  • This is because the history contains multiple states and causes flaky functionality.
  • This is because the player already uses history API to go from one story to another

Which AMP version is affected?

  • Latest

cc @Enriqe

@Enriqe
Copy link
Contributor

Enriqe commented Nov 5, 2020

Could you please provide more details on what you are trying to achieve? Listing the repro steps and the expected / actual behavior would be very helpful.

@rnkimani
Copy link
Author

@Enriqe if you test out here and click the bubble on the site, then navigate to a few stories forward, if you hit the back button in the browser, it doesn't close the url, the page has still active state.

@rnkimani
Copy link
Author

Screen recording

@rnkimani
Copy link
Author

rnkimani commented Dec 7, 2020

@Enriqe ^

@Enriqe
Copy link
Contributor

Enriqe commented Dec 7, 2020

Actually I don't think it's the player but the amp-story.js (the story document itself) that is actually using the history API. I'm actually unsure what the intended behavior should be here. Could you share what are you writing to the history object when navigating stories & what your expected behavior is?

My thinking is that we should write to the history state when the player is opened and then on navigation from story to story. Then, when clicking backwards on the browser window we should pop those states until the player is closed. WDYT @gmajoulet, any thoughts here?

/cc @ampproject/wg-stories

@newmuis
Copy link
Contributor

newmuis commented Dec 7, 2020

Wait, the story inside the player affects the top-level history stack?

@gmajoulet
Copy link
Contributor

The story inside the player would always write in its own window history, never in the top level window. There is a lot of AMP code that makes sure of this.
The AMP Story player itself does not do anything with the browser history as well.

Using this demo we have, I don't see anything being written in the browser history: https://www.gstatic.com/amphtml/stamp/qa/player.html

Could you please try to repro this outside of your VueJS context, or check if your application code could be responsible for this?

@gmajoulet
Copy link
Contributor

I did find this code in your application, do you think that might cause it?

u.addEventListener("navigation", ({detail: t})=>{
    lt.updateDetails(t);
    const e = d[t.index];
    if (ct.setData(d[t.index]),
    e && h.show(vt(e)),
    a.useHistory) {
        const t = "?" + e.id;
        window.history.replaceState("", "", t)
    }
}
)

@rnkimani
Copy link
Author

rnkimani commented Dec 8, 2020

@gmajoulet That code isn't affecting it since I have it behind a feature flag and it's turned off.
In this case a.useHistory is off.

@rnkimani
Copy link
Author

rnkimani commented Dec 8, 2020

I'm also programmatically controlling the player and I see in the example you gave it doesn't. Maybe that could be the issue too.

@gmajoulet
Copy link
Contributor

Sorry for the delay. I have been able to repro this outside of your codebase.

Repro steps:

  • Create a player with 5 stories
  • BAD: When navigating to the 3rd Story, an entry gets added to the browser History stack
  • EXPECTED: no change to the browser History stack

cc @ampproject/wg-stories

@Enriqe
Copy link
Contributor

Enriqe commented Jan 19, 2021

It seems that updating the src of an iframe will write an entry to the parent window's history object.

An alternative is to use the replace() method on the location object of the iframe, but that only works for same-domain (friendly) iframes.

For cross-origin iframes we will need to remove the iframe, update the src, and then re-append to the DOM.
https://stackoverflow.com/a/8681618/2103724

@rnkimani
Copy link
Author

Yeah, and it seems it couldn't be overridden with any replace function call above it. It would still retain history of the ones on the iframe.

@gmajoulet
Copy link
Contributor

To update this thread, @Enriqe is working on simplifying some Player code to make the overall library faster and more reliable. These updates will have the nice side effect of fixing this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants