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

Add events for History API usage #402

Closed
juliandescottes opened this issue Apr 12, 2023 · 17 comments
Closed

Add events for History API usage #402

juliandescottes opened this issue Apr 12, 2023 · 17 comments
Labels
browsingContext Browsing Context module enhancement New feature or request needs-discussion Issues to be discussed by the working group

Comments

@juliandescottes
Copy link
Contributor

The BiDi events monitoring navigation and page load are not triggered when using the history API.
"history" navigations are quite different from real navigations, so it probably makes sense to avoid emitting the regular navigation events for those.

However it could be interesting to at least add a new event triggered when popstate is emitted by the History API. For instance, CDP offers the Page.navigatedWithinDocument event which is emitted in this case.

The BrowsingContext module could be a good fit for this new event.

@whimboo whimboo added enhancement New feature or request browsingContext Browsing Context module needs-discussion Issues to be discussed by the working group labels Apr 12, 2023
@juliandescottes
Copy link
Contributor Author

@OrKoN can you share if Page.navigatedWithinDocument is important to support for puppeteer? I can see a few hits in the puppeteer codebase, but not sure how critical that is for you?

@OrKoN
Copy link
Contributor

OrKoN commented Apr 13, 2023

In Puppeteer, we try to treat same-document navigations the same way as regular navigations. We use navigatedWithinDocument for this. For example, using the history API or navigating to page#hash would be treated as a navigation in Puppeteer.

@OrKoN
Copy link
Contributor

OrKoN commented Apr 13, 2023

@whimboo
Copy link
Contributor

whimboo commented Jun 29, 2023

Relevant parts of the code https://github.com/puppeteer/puppeteer/blob/main/packages/puppeteer-core/src/common/Frame.ts#L335

@OrKoN, I assume that you accidentally pasted a different link here? This one goes to childFrames.

@OrKoN
Copy link
Contributor

OrKoN commented Jun 29, 2023

To clarify: the support for Page.navigatedWithinDocument (or an alternative way to get events about same document navigations) is important to Puppeteer.

P.S. updated the link

@jgraham
Copy link
Member

jgraham commented Jun 29, 2023

Isn't that browsingContext.FragmentNavigated?

@OrKoN
Copy link
Contributor

OrKoN commented Jun 29, 2023

I think it might be it: perhaps not implemented by the implementations yet?

@OrKoN
Copy link
Contributor

OrKoN commented Jun 29, 2023

I think it is covered by this algorithm https://html.spec.whatwg.org/multipage/browsing-the-web.html#scroll-to-fragid (step 17.2)?

@whimboo
Copy link
Contributor

whimboo commented Jun 29, 2023

Yes, it's not implemented in Firefox yet. I just filed https://bugzilla.mozilla.org/show_bug.cgi?id=1841039.

@OrKoN does Chrome / CDP have it? Does it work as expected? If yes we potentially can close this issue.

@OrKoN
Copy link
Contributor

OrKoN commented Jun 29, 2023

I think it is implemented in chromium-bidi. Perhaps we need to add some WPT tests?

@whimboo
Copy link
Contributor

whimboo commented Jun 29, 2023

Yes, wdspec tests are missing for this event. Could someone from Google may implement those? I'm happy to review, and to include our implementation for the next milestone.

@OrKoN
Copy link
Contributor

OrKoN commented Jun 29, 2023

let me take this (I will close the issue once the WPT test have landed)

@whimboo
Copy link
Contributor

whimboo commented Jul 6, 2023

let me take this (I will close the issue once the WPT test have landed)

Which wpt tests do you reference here? Those for browsingContext.fragmentNavigated?

@OrKoN
Copy link
Contributor

OrKoN commented Jul 6, 2023

yes

@whimboo
Copy link
Contributor

whimboo commented Jul 6, 2023

Ok, so the related PR has been merged. Does it mean we can now also close this issue or do you have to run more checks first?

@OrKoN
Copy link
Contributor

OrKoN commented Jul 6, 2023

From my perspective, the current test set is sufficient (it only tests basic history API functions ) though but we already adjusted our implementation so it passes the tests. Unless someone sees a need for more History API tests, I would close now.

@OrKoN OrKoN closed this as completed Jul 6, 2023
@whimboo whimboo closed this as not planned Won't fix, can't repro, duplicate, stale Jul 6, 2023
@whimboo
Copy link
Contributor

whimboo commented Jul 6, 2023

We can / will certainly add more tests but this wouldn't require any new event for WebDriver BiDi. As such we can indeed close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browsingContext Browsing Context module enhancement New feature or request needs-discussion Issues to be discussed by the working group
Projects
None yet
Development

No branches or pull requests

4 participants