-
Notifications
You must be signed in to change notification settings - Fork 1.5k
browser: implement page.goBack() and page.goForward()
#5494
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
Conversation
inancgumus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution ❤️ Please watch the test results, and see if they pass, as I ran them. I have some suggestions.
|
@inancgumus Thanks a lot for the review and feedback.
Regarding this, indeed you are right. The test are timing out. I overlooked this. |
|
@inancgumus, I have implemented all the suggestions, thanks for pointing out a critical issue with the implementation.
What ?
FIXChanged the implementation to poll for URL change instead of waiting for
Also I am not sure why 2 test were failing in ci. please guide me here. |
inancgumus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @pkalsi97.
I have some suggestions, and we need to fix the git conflicts.
Don't mind those flaky tests. Your PR is fine as long as your tests pass.
|
Hi @pkalsi97, can you fix the linter issue? |
|
@inancgumus Done. |
inancgumus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work, @pkalsi97 🙇 Can you merge the Page.GoBack and Page.GoForward methods into a single method called Page.GoBackForward. We'll still keep the mapping layer intact (i.e., goBack and goForward). Thanks again!
|
@inancgumus done, yes this completely makes sense ! thanks. |
inancgumus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 🚀
|
@pkalsi97, it would be great if you could provide updates to k6-docs and TypeScript definitions for this feature. Thanks! |
|
@inancgumus sure. Will do by tomorrow. |
ankur22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pkalsi97 for the work on goBack and goForward! I think most of it looks good. I think I would like some clarification on a question and rethink on how we can work with lifecycle events. I took a look at Playwright's implementation and they seem to be waiting on navigation events.
| // Poll for URL change, don't rely on lifecycle events since bfcache | ||
| // restorations don't re-fire them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure about this change to working with polling.
- Why can't we rely on the lifecycle event?
- The
WaitUntiloption isn't used anywhere in theGoBackForwardmethod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ankur22 Thanks for the review!
- my initial implementation did use lifecycle events similar to Reload(), but test keep on timing out for real html pages and iframes, as per my knowledge
EventFrameNavigationwasn't being emitted for history navigations (due to Chrome's bfcache), when we try to navigate forward or backward chrome often restore pages from memory snapshots instead or reloading themPage.frameNavigatedonly fires when bfcache navigation fails.
polling was used here as we know the target URL from navigation history and it updates immediately after NavigateToHistoryEntry(), making it reliable regardless of bfcache state.
- Regarding
WaitUntilyou're right, I can remove it from the options since it's not applicable with the polling approach.
I'm fairly new to this , i'll greatly appreciate if you could fill me in on an approach that does not use polling/ any other approach that you think will be a better implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkalsi97 I'll need to take a close look at this PR and the problem you've outlined to understand why we can't work with lifecycle events and why they weren't firing -- I can see why that might be the case due to the cache. What's not clear is whether Playwright is also hitting this issue and how they have resolved it, this is important since the behaviour of k6 browser should match Playwright's if we're copying their API. I'll add my findings in this PR once I've got an answer for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll also look into it and figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not certain if we are still waiting on this?
cc @ankur22 @inancgumus Shall we merge this as is?
mstoykov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from one nit and the discussion by @ankur22 on lifcycle events.
| wrapTimeoutError := func(err error) error { | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| err = &k6ext.UserFriendlyError{ | ||
| Err: err, | ||
| Timeout: opts.Timeout, | ||
| } | ||
| } | ||
| p.logger.Debugf("Page:GoBackForward", "timeoutCtx done: %v", err) | ||
| return fmt.Errorf("navigating %s to history entry %d: %w", direction, historyEntryID, err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this eitehr needs to be a helper function or just be inlined in the opne place it is actually used in this function
| // Poll for URL change, don't rely on lifecycle events since bfcache | ||
| // restorations don't re-fire them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not certain if we are still waiting on this?
cc @ankur22 @inancgumus Shall we merge this as is?
|
@pkalsi97, we can merge this PR if you fix the conflicts (they happened in the meantime). Thank you! |
|
@pkalsi97 @inancgumus I moved this pull request to v1.7.0 release. If in the meantime we manage to fix the conflicts then we'll move back to the upcoming v1.6.0 release. |
|
@mstoykov Thanks for the review! @inancgumus @codebien i'll work on this shortly and try to fix it. Thanks |
|
Hi @pkalsi97, since we merged the PR, can you work on the Typescript definitions and k6-docs for this feature if possible? Thank you! |
|
@inancgumus Already implemented! Thanks |
What?
This PR implements
page.goBack()andpage.goForward()to allow users to navigate through the browser's session history.I have kept the implementation inline with
Reload()and followed guidelines mentioned in the issue.Why?
This functionality was missing and users had to rely on
page.evaluate(() => window.history.back()), which is racy and unreliable for testing purposes as mentioned in the issue #5400Example
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #5400