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

Initial spec for appHistory.navigate() and event.destination #96

Merged
merged 24 commits into from
May 4, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Apr 15, 2021

Regarding #95, for now the promise never settles for cross-document navigations.

Regarding #97, for now I specified url, sameDocument (not changing), and getState().


Preview | Diff

@@ -135,12 +136,32 @@ interface AppHistory : EventTarget {
readonly attribute boolean canGoBack;
readonly attribute boolean canGoForward;

Promise<undefined> navigate(USVString url, optional AppHistoryNavigateOptions options = {});
Promise<undefined> navigate(optional AppHistoryNavigateOptions options = {});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replace is not actually consulted for this overload. Perhaps we should use a fresh dictionary? This is actually observable in that the getter for replace will be called, whereas if we used a fresh dictionary it would not be called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ok for now, but we should think more about this.

Should options be required for the no-url variant? It seems nonsensical to call navigate() with 0-args.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Web IDL requires that trailing dictionaries be optional, even if logically they are not optional. See whatwg/webidl#903.

Probably we should include a comment since this is confusing to people.

spec.bs Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@domenic domenic changed the title Initial spec for appHistory.navigate() Initial spec for appHistory.navigate() and event.destination Apr 16, 2021
This has the impact of making appHistory.navigate("#foo", { state }) work instead of ignore the state (even when there's no navigate event handler)
README.md Show resolved Hide resolved
@@ -135,12 +136,32 @@ interface AppHistory : EventTarget {
readonly attribute boolean canGoBack;
readonly attribute boolean canGoForward;

Promise<undefined> navigate(USVString url, optional AppHistoryNavigateOptions options = {});
Promise<undefined> navigate(optional AppHistoryNavigateOptions options = {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is ok for now, but we should think more about this.

Should options be required for the no-url variant? It seems nonsensical to call navigate() with 0-args.

spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
@domenic
Copy link
Collaborator Author

domenic commented Apr 22, 2021

Note to self: if the navigate event detaches the window we need to figure out what the returned promise should do. Probably forever-pending makes the most sense because any task queued to resolve the promise would be associated to a dead window/document and so would never actually happen.

domenic added a commit that referenced this pull request Apr 23, 2021
(This is in the broad sense of "navigation", which includes the navigate algorithm, history traversal, and pushState()/replaceState(). I.e., anything that would fire a navigate event, even if it doesn't.)

Discussed in #96 (comment).
@domenic
Copy link
Collaborator Author

domenic commented Apr 23, 2021

The "AbortError" cases (including ones triggered by #104) need more thinking here. Should they fire a corresponding navigateerror event? I think so; it seems simplest to keep them always in sync. I'll try to spec that out, although until #104 lands it'll be a bit messy.

domenic added a commit that referenced this pull request Apr 23, 2021
I.e., make sure that the rest of the algorithm doesn't think we called respondWith(). This is important for #96.
@natechapin
Copy link
Collaborator

Note to self: if the navigate event detaches the window we need to figure out what the returned promise should do. Probably forever-pending makes the most sense because any task queued to resolve the promise would be associated to a dead window/document and so would never actually happen.

The behavior in https://chromium-review.googlesource.com/c/chromium/src/+/2822057 is to reject the promise, on the logic that detaching the window effectively aborts all navigations.

@domenic
Copy link
Collaborator Author

domenic commented Apr 23, 2021

Hmm, it seems we've run into whatwg/html#2621 ...

@domenic
Copy link
Collaborator Author

domenic commented Apr 23, 2021

Ah OK, it still works. Microtasks, unlike tasks, are not document-bound.

domenic added a commit that referenced this pull request Apr 23, 2021
(This is in the broad sense of "navigation", which includes the navigate algorithm, history traversal, and pushState()/replaceState(). I.e., anything that would fire a navigate event, even if it doesn't.)

Discussed in #96 (comment).
@domenic
Copy link
Collaborator Author

domenic commented Apr 23, 2021

Great, this is rebased on top of all the somewhat-related changes and is ready for re-review.

@domenic domenic merged commit 99ac021 into main May 4, 2021
@domenic domenic deleted the navigate-spec branch May 4, 2021 17:49
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.

2 participants