Fix WaitForNavigation within the same document

[imiric]

Previously WaitForNavigation assumed that a LifecycleEvent would always be fired, but this is not the case for navigation within the same document (e.g. via anchor links or the History API), so in those cases the call would timeout after 30s.

The fix simply checks if we received a new document, otherwise it skips waiting for the LifecycleEvent. Even if that wait didn't time out, it would've failed with a nil pointer panic accessing event.newDocument.request.

Closes #226

[imiric]

One thing I'm not sure about is if waiting for navigation events within the same document should be done with a separate method call (e.g. waitForNavigationWithinDocument()), or some option passed to waitForNavigation(). My rationale is that it would be useful to want to ignore same-document navigations and explicitly wait for full navigations, or viceversa. Otherwise it would be annoying/surprising if that's the expected behavior and the page does some anchor or history navigation.

Playwright doesn't do this and they emit the same navigation event they use for Page.frameNavigated, and also check if newDocument is set on it, but we don't have to follow their API exactly, IMO.

Ah, actually, they allow waiting for a specific URL, which from looking at the code we don't seem to do, even though we have FrameWaitForNavigationOptions.URL defined. Should we expand the scope of this a bit and try to implement that feature as well?

[inancgumus]

Good to see that you solved the issue 🥳

Should we expand the scope of this and try to implement that feature as well?

I'm on the side of making it in another PR (with another issue beforehand).

We might also stop waiting if:

• The page was closed
• The page was crashed
• The frame was detached

I'm not sure whether we handle these gracefully?

Besides, Playwright waits for the EventFrameNavigated event but we wait for the EventFrameNavigation event :/ I changed code to listen for the prior and our test timed out. Should we consider this?

[imiric]

I'm on the side of making it in another PR (with another issue beforehand).

Agreed. I'll create the issue, and we can discuss what priority it should have.

We might also stop waiting if:

• The page was closed
• The page was crashed
• The frame was detached

I would expect those to be handled by cancelling m.ctx, but I can add some tests.

Playwright waits for the EventFrameNavigated event but we wait for the EventFrameNavigation event :/

I'm not concerned with minor discrepancies like this. We don't have to follow their code exactly. That said, we do have EventPageFrameNavigated, so I suppose it makes sense for this to be EventFrameNavigation. I'll see if I can rename it in this PR.

[inancgumus]

I'm not concerned with minor discrepancies like this. We don't have to follow their code exactly. That said, we do have EventPageFrameNavigated, so I suppose it makes sense for this to be EventFrameNavigation. I'll see if I can rename it in this PR.

I didn't bring it up for renaming the event :) I wondered whether we handle the protocol events correctly 🤔

It turned that those are not protocol events but PW internals.

[inancgumus]

LGTM 👏