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

Fix navigate to link in new page #1334

Merged
merged 3 commits into from
May 15, 2024
Merged

Fix navigate to link in new page #1334

merged 3 commits into from
May 15, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented May 15, 2024

What?

Move the CDP request RunIfWaitingForDebugger to be called first when creating a FrameSession.

Why?

We have identified that the call to RunIfWaitingForDebugger wasn't being performed at the correct time when a new page was created after a link was clicked on that opened the page.

This is currently the simplest and safest solution. I tried to place some of the CDP actions that are in NewFrameSession within their own goroutines but there were data races.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #827

We have identified that the call to RunIfWaitingForDebugger wasn't
being performed at the correct time when a new page was created after a
link was clicked on that opened the page.

The first step is to move out of the initOptions so that we can
eventually run it independently of the other setup/init steps.
@ankur22 ankur22 marked this pull request as draft May 15, 2024 10:54
@ankur22 ankur22 marked this pull request as ready for review May 15, 2024 13:35
@ankur22 ankur22 requested a review from inancgumus May 15, 2024 13:37
@ankur22 ankur22 changed the title Refactor RunIfWaitingForDebugger Fix navigate to link in new page May 15, 2024
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Nice fix and test :) A few suggestions.

tests/page_test.go Outdated Show resolved Hide resolved
tests/page_test.go Show resolved Hide resolved
tests/page_test.go Show resolved Hide resolved
tests/page_test.go Show resolved Hide resolved
tests/page_test.go Show resolved Hide resolved
This test ensures that a new page is opened and correctly navigated to
when a link with target=_blank is clicked on.
Instead of basing the test on waiting a certain time and retrying every
so often, we're now working with the built in APIs that help with this
wait action in a more stable manner.
@ankur22 ankur22 merged commit c5c9435 into main May 15, 2024
17 checks passed
@ankur22 ankur22 deleted the fix/827-link-new-window branch May 15, 2024 15:35
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