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

Behavior of navigate(currentURL, { replace }) #111

Closed
domenic opened this issue May 12, 2021 · 38 comments · Fixed by #219
Closed

Behavior of navigate(currentURL, { replace }) #111

domenic opened this issue May 12, 2021 · 38 comments · Fixed by #219
Labels
change A proposed change to the API, not foundational, but deeper than the surface feedback wanted

Comments

@domenic
Copy link
Collaborator

domenic commented May 12, 2021

@jakearchibald, @annevk, @smaug---- and I discussed the behavior of navigation.navigate(appHistory.current.url) today.

The current spec says that this always does a replace navigation, similar to location.href = location.href. This is a bit weird because it means that the replace parameter is ignored, e.g.

// All three of these are equivalent: they all do a replace.
navigation.navigate(navigation.currentEntry.url);
navigation.navigate(navigation.currentEntry.url, { replace: true });
navigation.navigate(navigation.currentEntry.url, { replace: false });

We discussed a few options for what to do:

  1. Do a same-document navigation, either push or replace according to the replace option.

    • Precedent: pushState(null, "") / replaceState(null, "")
    • But, this is not great because in general we want navigate() to be about new-document navigations; to make them same-document, you intercept with the navigate event.
  2. Do a new-document replace navigation, ignoring the replace option

    • Precedent: location.href = location.href, or location.assign(location.href).
    • This is the spec's current behavior.
  3. Do a new-document navigation, either push or replace according to the replace option.

    • A new-document push navigation to the same URL as the current one is only possible today by using redirects, and only in some browsers.
  4. (3), but have the default be a somewhat-magic "replace for same-URL, push for different-URL". I.e.

    navigation.navigate(navigation.currentEntry.url);                      // does a replace
    navigation.navigate(navigation.currentEntry.url, { replace: true });   // does a replace
    navigation.navigate(navigation.currentEntry.url, { replace: false });  // does a push
    
    navigation.navigate(someOtherURL);                                // does a push
    navigation.navigate(someOtherURL, { replace: true });             // does a replace
    navigation.navigate(someOtherURL, { replace: false });            // does a push
  5. Return a rejected promise whenever you pass replace: false and try to navigate to the current URL.

  6. Return a rejected promise whenever you pass anything besides replace: true and try to navigate to the current URL. I.e. you need to be super-explicit.

We were leaning toward (4), although the lack of good precedent is a bit tricky.

@jakearchibald
Copy link

  • But, this is not great because in general we want navigate() to be about new-document navigations

Is that true for appHistory.navigate('#foo')?

We were leaning toward (4), although the lack of good precedent is a bit tricky.

There's more precedent when it comes to enums, with one being "auto". I scribbled down some ideas in #113, including:

replace is currently a boolean, but it feels like it needs to be three states. So, how about entryPlacement:

  • "add" - Creates a new history entry and adds it to the timeline, clearing forward history.
  • "replace" - Creates a new history entry and replaces the current item in the timeline.
  • "auto" - What browsers do now, where it depends on the current URL and the target URL.
@domenic
Copy link
Collaborator Author

domenic commented May 13, 2021

Is that true for appHistory.navigate('#foo')?

No. The more precise statement is, we want to encourage people doing SPA navs to go through the navigate handler. Navigating to #foo is not such an SPA nav.

There's more precedent when it comes to enums

I meant precedent in regards to letting people perform same-URL push navigations. I'm not sure we want to make that functionality easy.

@domenic domenic added change A proposed change to the API, not foundational, but deeper than the surface feedback wanted labels May 27, 2021
@domenic domenic added this to the Might block v1 milestone Nov 4, 2021
@domenic
Copy link
Collaborator Author

domenic commented Jan 6, 2022

Somewhat related: whatwg/html#7386. What does appHistory.navigate("#foo") do if you're already on #foo?

@Yay295
Copy link

Yay295 commented Jan 7, 2022

Scroll to #foo I'd think. That's what a link would do.

@annevk
Copy link

annevk commented Jan 7, 2022

If that is correct and Chrome only implements the early return for Location's hash setter, I agree that ideally we would not copy that here. Typically we do not have such early returns in platform APIs (e.g., thinking of various node tree mutations).

@annevk
Copy link

annevk commented Mar 10, 2022

What's the advantage of 4 over 2? "A bit weird" doesn't strike me as enough of a negative to introduce novel behavior.

@domenic
Copy link
Collaborator Author

domenic commented Mar 10, 2022

Yeah, 2 is probably the path of least resistance. Maybe with a console warning, if not a rejected promise, if you pass replace: false explicitly. It feels a bit uncomfortable that the developer explicitly requested non-replace and you give them replace anyway, basically.

@annevk
Copy link

annevk commented Mar 10, 2022

I guess I could see 5 being okay. Perhaps some input from web developers would help sway this one way or another.

@annevk
Copy link

annevk commented Mar 17, 2022

From #72 (comment):

Currently the HTML spec has such a gadget: it makes the replace-or-push decision based on the iframe's current URL. But we should fix that separately. For example, we could adopt Chromium's solution, which is to always push when the navigation initiator is cross-origin.

This approach doesn't work with upfront decisions, right? (I.e., the idea that you decide before navigating whether you end up replacing or pushing.) Because the initial URL could be same-origin, but then redirect to the URL you want to attack.

@domenic
Copy link
Collaborator Author

domenic commented Mar 17, 2022

The decision is made based on the initiator document's origin vs. the target iframe's current document's origin. That works, I think.

Cases:

  • Initiator document is same-origin to target iframe's current document and target iframe's destination document: will be push or replace, but everything is same-origin so it's fine to expose the information
  • Initiator document is same-origin to target iframe's current document but cross-origin to target iframe's destination document: the URLs are not the same so will be push anyway
  • Initiator document is cross-origin to target iframe's current document but same-origin to target iframe's destination document: the URLs are not the same so will be push anyway
  • Initiator document is cross-origin to target iframe's current document and cross-origin to target iframe's destination document: will always be push, per the Chromium change
@domenic
Copy link
Collaborator Author

domenic commented Mar 17, 2022

@annevk, @smaug----, @natechapin and I discussed this offline. Our main concerns were:

  • @domenic and @smaug--- did not want there to be a case where you explicitly requested replace behavior, and silently got push.
  • @annevk preferred avoiding replace: undefined (or omitted) and replace: false meaning different things.
  • We think it'd be fine to expose push-with-the-current URL, but also would prefer not to for now since it's currently only doable with redirect tricks.

These constraints combine into something based on @jakearchibald's ideas upthread in #111 (comment) . Specifically, we change the replace boolean to a history enum, with values "auto" (default), "replace", and "push". (Credit to @natechapin for the history naming.) Then:

  • navigate(nonCurrentURL) does push
  • navigate(currentURL) does replace
  • navigate(currentURL, { history: "relace" }), navigate(nonCurrentURL, { history: "push" }), and navigate(nonCurrentURL, { history: "replace" }) do what you'd expect
  • navigate(currentURL, { history: "push" }) returns rejected promises for now, doing no navigation. In the future we could loosen this up.

TLDR for web developers wondering how the API will change: instead of { replace: true }, you'll do { history: "replace" }.

I will work on spec/implementation/test updates for this soon!

@domenic
Copy link
Collaborator Author

domenic commented Mar 24, 2022

In the course of writing the spec (#219) it became clear that current URL is not the only case we care about; initial about:blank, javascript: URLs, and not-completely-loaded also have similar implicitly-convert-push-to-replace behavior. I've specced those to return rejected promises for "push" as well.

domenic added a commit that referenced this issue Mar 25, 2022
We can then introduce { history: "push" } which explicitly errors if we're in a convert-to-replace situation. Closes #111.

This also fixes a preexisting omission where we were not properly doing replace navigations before the document is loaded.
@domenic
Copy link
Collaborator Author

domenic commented Jun 3, 2022

We've received reports from an app adopting the navigation API that same-URL pushes are actually desired.

The issue is that they are migrating code from history.pushState(newState, "", currentURL), which is a same-document URL push, to navigation.navigate(currentURL, { state: newState, history: "push" }) + navigateEvent.transitionWhile(), which is intended to be the same behavior. But due to what we specced here, this will throw an exception.

IMO we should just allow same-URL push navigations. As mentioned above we basically just omitted this from the MVP because we thought it was not necessary. It's still not necessary cross-document, but it is necessary same-document, and part of the point of the navigation API is to let you decouple the same-document vs. cross-documentness of your navigation from the initiator.

@domenic domenic reopened this Jun 3, 2022
@uasan
Copy link

uasan commented Oct 6, 2022

Unfortunately, our experience shows the negative effect of pushing the current url to the navigation history.

In practice, especially on the mobile version of the web, the client double clicks on a link or button, for various reasons why clients do this, especially a high percentage of such clicks from clients with disabilities. Because of pushing the same urls into the history, when you click on the back button, nothing happens, customers then complain that the native back button does not work, the reason is that a lot of the same urls we are pushed in the history.

We are trying to process this programmatically, but I don’t know by chance who needs to push the same urls into the story, it looks like bad design and your original suggestion of the same urls always doing a post replacement instead of a push was correct.

If you don't use navigation handlers.
When you click on a link that leads to the current document, all browsers will reload the document, but do not add a new step to the history, i.e. all browser do reload not push

@gustavopch
Copy link

I have a use case for same-URL pushes. It's a page that's dynamically rendered according to the params that are passed by history state. So the URL is the same, but the state is different.

BTW, React Navigation (popular React Native lib) addresses this the following way:

[...] you'll notice that when you tap "Go to Details... again" that it doesn't do anything! This is because we are already on the Details route. The navigate function roughly means "go to this screen", and if you are already on that screen then it makes sense that it would do nothing.

Let's suppose that we actually want to add another details screen. This is pretty common in cases where you pass in some unique data to each route (more on that later when we talk about params!). To do this, we can change navigate to push. This allows us to express the intent to add another route regardless of the existing navigation history.
https://reactnavigation.org/docs/navigating/#navigate-to-a-route-multiple-times

@uasan
Copy link

uasan commented Oct 6, 2022

I understand your case, but you don’t need clients to stay on the current page when they click on the back button?

I want this behavior to be fixed, just bring it in line with the history behavior when there are no history handlers, so that the back button works the same way it works when there are no history handlers.

P.S.
Click three times on the github logo and then click on the back button, everything works correctly, in cases of using API navigation handlers, clicking on the back button 3 times will not do anything.

@gustavopch
Copy link

gustavopch commented Oct 6, 2022

Required behavior for my use case described above:

History after entering the first page:
/something (state: { id: 123 })

History after entering the second page:
/something (state: { id: 123 })
/something (state: { id: 234 })

History after clicking the back button:
/something (state: { id: 123 })

The URL is the same, but they need to be treated as 2 different pages and I need to re-render whenever the history changes.

@uasan
Copy link

uasan commented Oct 6, 2022

I understand that when you click back, you need the user to remain on the same url, but with a new non-explicit state, you probably know that this will not work if the user passes the link, the non-explicit state is worse than the explicit one, but this is your choice.

In my case, there are no hidden states, a compromise solution, if no states are passed to the navigation method and the specified url is equal to the current url, in this case always replace the history instead of pushing, so everyone is happy?

@gustavopch
Copy link

gustavopch commented Oct 6, 2022

If I'm navigating to the same URL and passing different values in the state, then I have the intention of pushing a new navigation entry. If the values in the state are the same, then it was a double click.

Not sure if there would be some use case where someone would want to push a new entry with the same URL and same state.

@uasan
Copy link

uasan commented Oct 6, 2022

I propose extends NavigationInterceptOptions add optional property history - value from enum NavigationType.
this will allow implementation of custom logic for similar and other cases.

navigation.addEventListener('navigate', e => {
  //...
  e.intercept({
    history: navigation.currentEntry.url === e.destination.url ? 'replace' : e.navigationType,
    //...
  });
});

What do you think?

@tbondwilkinson
Copy link
Contributor

The boat has long sailed on whether or not we can allow same-URL pushes - this is something that history.pushState() allows, it's something location allows, I do not think we should change this behavior.

But, I think the API makes this pretty easy to configure for users who do NOT want to allow same-URL navigations, for instance for debouncing link clicks. There are lots of ways you can prevent navigations that are the "same" as the previous navigation, but here's one simple way:

navigation.addEventListener('navigate', e => {
  if (navigation.currentEntry.url === e.destination.url) {
    e.preventDefault();
  }
});

That is, if the url is the same, just preventDefault the navigation. There are lots of other complicated ways you might do this as well, but that would be app specific about what is and is not a "same" navigation.

@uasan
Copy link

uasan commented Oct 6, 2022

it's something location allows

No, try this 3 times after each reload:

location.href = location.href;

and then 1 click on the back button and you will get a page that, according to your logic, is at step -3.
It does not cancel the transition, it performed it in replace mode, this is what I want, do not cancel events, execute it in replace mode.

@tbondwilkinson
Copy link
Contributor

Yes, true, I'm just saying you could have the same URL multiple times in history by location.replace but there is no "push" API in location.

@uasan
Copy link

uasan commented Oct 6, 2022

Yes, true, I'm just saying you could have the same URL multiple times in history by location.replace but there is no "push" API in location.

You suggested me another compromise solution )

Now for all browsers, clicking on a link means push transition mode, except if the url is equal to the current one, I suggest if the event is caused by a click on the link and the url is equal to the current one, then perform the transition in replacement mode, this will not break those cases where implicit state is passed, because clicking on a link has no implicit state and this is consistent with the actual behavior of all browsers

@tbondwilkinson
Copy link
Contributor

If you want to maintain current browser behavior of doing nothing or doing a reload when the URL is the same, I think that use case is satisfied with the existing API. But if you want to re-do some part of your application logic on same-URL navigations and they should be replace rather than push, I agree that seems trickier. If you want some way intercept yet also transform a (currently) push navigation into a replace navigation, without cancelling and trigger a replace navigation, I don't think there's any API to do that today. It may be infeasible to do so based on the timing of when the navigate event is sent, but maybe not. Perhaps a new issue highlighting that specific use case?

@uasan
Copy link

uasan commented Oct 6, 2022

We have a SPA application, we cannot do nothing when the client clicks on the link that leads to the current url.

If we do not call the intercept, our application will restart and all client states will be lost, we must not allow the document to be reloaded.

We must execute the intercept, because that, judging by the behavior of users, they click on links (for example, on the site logo) to load new content, so we need to call the intercept (fetch by RestAPI new content), but in the history replacement mode, so that the back button works correctly.

@domenic
Copy link
Collaborator Author

domenic commented Oct 7, 2022

Isn't this handled pretty simply with code such e.preventDefault(); navigation.navigate(e.destination.url, { history: "replace" });? Maybe with a bit extra to copy over state and/or info appropriately.

@uasan
Copy link

uasan commented Oct 7, 2022

e.preventDefault(); navigation.navigate(e.destination.url, { history: "replace" });

Yes, we use something similar, since the use of the old API history history.pushState().
Quote from your resume, what shortcomings of old interfaces are solved by the new navigation API

is hard to deal with in practice, especially for single-page applications. In the best case, developers can work around this with various hacks.
https://github.com/WICG/navigation-api#summary

But in practice, you suggest using the same hacks, instead of solving the problem at the level of a new interface.

All I want is that the behavior of browsers without navigation event handlers matches the behavior with the handler, if the browsers click on the link with url equal to the current url, make the transition in replace mode, the same thing should happen in the handler without the need to write extra hacks.

But it’s even better to give the ability to specify the type of history in the interceptor, the interface that I proposed will give imperative flexibility to custom implementations of such cases.

navigation.addEventListener('navigate', e => {
  //...
  e.intercept({
    history: navigation.currentEntry.url === e.destination.url ? 'replace' : e.navigationType,
    //...
  });
});
@domenic
Copy link
Collaborator Author

domenic commented Oct 7, 2022

I don't think using two methods in a row counts as a hack?

I know you really like adding new options to intercept, but that's not generally how intercept() options work; they control the interception, they don't start a new navigation with new properties.

@uasan
Copy link

uasan commented Oct 7, 2022

I don't think using two methods in a row counts as a hack?

Yes, this is a hack because these methods are used to get around the current interface limitations.

e.preventDefault() Raises exceptions for those who caused the event.
navigation.navigate(e.destination.url, { history: "replace" }) Then we create a new event which nobody await.

These two actions do not do what we need, we do not need to cancel events and we do not need to create a new event.

but that's not generally how intercept() options work; they control the interception, they don't start a new navigation with new properties.

In my opinion this is a hijacking of the default behavior i.e. we intercept the type of history change and replace it with the one we want, it seems logical to me, but if you have a better interface, I will agree.

@domenic
Copy link
Collaborator Author

domenic commented Oct 7, 2022

Fair point about the negative consequences of combining those two. Perhaps this is something to consider when designing the redirect #124. But it's pretty off-topic for this issue which is about the behavior of navigate(currentURL, { replace }).

@uasan
Copy link

uasan commented Oct 7, 2022

It discusses #124 the replacement of url, here it discusses the changing of the type of the history, these are different problems, but solutions to both in the new optional properties for the intercept method )

@domenic
Copy link
Collaborator Author

domenic commented Oct 7, 2022

I think they are more closely related, since they are both about changing an in-progress navigation. And as I stated above, that's not really what the intercept() options are for...

@uasan
Copy link

uasan commented Oct 7, 2022

But do you basically agree that there should be an interface to change the current history type in the navigation handler or not?

@domenic
Copy link
Collaborator Author

domenic commented Oct 7, 2022

Yep! I just wish it wasn't in this issue, but instead was in a dedicated issue :)

@domenic
Copy link
Collaborator Author

domenic commented Apr 28, 2023

This was fixed in whatwg/html@0e2536f.

@domenic domenic closed this as completed Apr 28, 2023
@uasan
Copy link

uasan commented Apr 28, 2023

@domenic Thanks, do I need to create a bug report in Chrome, on the implementation of new spec changes?

@domenic
Copy link
Collaborator Author

domenic commented Apr 28, 2023

Nope, it's implemented in Chromium 114+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change A proposed change to the API, not foundational, but deeper than the surface feedback wanted
7 participants