Fix Locator.WaitFor for detached and hidden states

[ka3de]

This PR fixes the Locator.WaitFor functionality when the state defined is either hidden or detached. Previously, waiting for these two states would result in an infinite pooling loop and eventually a TO error.

Besides the integration tests included in the PR, this changes can be tested also using this test browser implemented by @ankur22 and executing the following script:

import { chromium } from 'k6/x/browser'

export default async function () {
    const browser = chromium.launch({
        headless: false,
    })
    const context = browser.newContext()
    const page = context.newPage()

    try {
        await page.goto('http://localhost/other', { waitUntil: 'networkidle' })
        
        page.locator("#attach-detach").waitFor({state: "detached"});
        console.log("detached");

        page.locator('#input-text-hidden').waitFor({state: "hidden"});
        console.log("hidden");
    } finally {
        page.close()
        browser.close()
    }
}

Take into account that this test will require user interaction to click on the Detach button on the top left of the page in order to remove the element that we are waiting for from the DOM.

Closes: #736.
Closes: #472.

[ka3de]

Some things to consider:

1. I added a nolint directive to this return nil, nil in Frame.waitForSelector. We could maybe consider defining an specific error for this condition of "nil handler", but initially I did not consider it necessary.
   Also in relation with this, I think we should try to comply with the same linting rules as k6, as we discussed recently.

2. I was considering removing this console.log that is in the injected_script.js, that looks like a warning but in the case of waiting for a detached state, that is perfectly valid. Also I believe these kind of errors are better to be handled from the Go code (e.g.: like here).

3. It was difficult for me to decide what to return from the waitForSelector function for the detached state, as in this case we do not have a valid DOM element to return that then will be transformed into an ElementHandle. In this case I decided to return true to complete the polling loop and actually return from the eval CDP call. Eventually this will be transformed into a JSHandle as a representation of the true value, but then in ElementHandle.waitForSelector implementation, because we do not receive a valid ElementHandlerepresentation, we return a nil element handle. Therefore the need in frame to handle this especial case for detached.
   Maybe there are better alternatives to this that I was not able to understand properly.

[ankur22]

Thanks for explaining your thought process on coming to the current implementation.

I think we should try to comply with the same linting rules as k6

Agreed.

Also I believe these kind of errors are better to be handled from the Go code (e.g.: like here)

Agreed. Is this something that we can tackle now or maybe best suited for another PR?

3. It was difficult for me to decide what to return from the waitForSelector function for the detached state, as in this case we do not have a valid DOM element to return that then will be transformed into an ElementHandle.

This is a tricky situation if we try to work with the current methods we have. Since locator.waitFor doesn't have a return value, could we create a new waitFor method in frame.go so that we don't check whether handle is nil? Something like:

func (f *Frame) waitFor(selector string, opts *FrameWaitForSelectorOptions) (error) {
	f.log.Debugf("Frame:waitFor", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector)

	document, err := f.document()
	if err != nil {
		return err
	}

	_, err = document.waitForSelector(f.ctx, selector, opts)
	if err != nil {
		return err
	}

        return nil
}

It is duplicating some of the functionality but in this case it feel like a safer option which is more maintainable, with less behavioural changes than if we extend the current waitForSelector. WDYT?

[ankur22]

Nicely done! Thanks for adding the succinct commit messages which helped explain the change.

My only feedback is on what i've already commented on regarding whether to extend the waitForSelector to add a new behaviour. If possible, it would be good to separate the behaviour for waitFor so that it's clear that it doesn't require the elementHandle.

[ka3de]

Agreed. Is this something that we can tackle now or maybe best suited for another PR?

If you mean deleting this line specifically from the injected_script.js, I can take care of that in this same PR if you agree. As I mentioned, I think it can be misleading considering that waiting for a detached element is valid, and in that case the user would see that log line.

This is a tricky situation if we try to work with the current methods we have. Since locator.waitFor doesn't have a return value, could we create a new waitFor method in frame.go so that we don't check whether handle is nil?

It can make sense, although we would have the same situation in ElementHandle.waitForSelector here.
Should we add a new waitFor method at the Frame level but keep the waitForSelector at the ElementHandle level? I believe that's a good compromise to not "duplicate" too much code but have more clear "decision branches" in the code. WDYT?

See the following diff for the changes to apply from this PRs current version:

diff

diff --git a/common/frame.go b/common/frame.go
index 0f36a2d..a857b6a 100644
--- a/common/frame.go
+++ b/common/frame.go
@@ -467,13 +467,7 @@ func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptio
                return nil, err
        }
        if handle == nil {
-               // Handle can be nil only for selectors waiting for 'detached' state,
-               // as there is no longer an ElementHandle representing that element
-               // in the DOM. Otherwise return error.
-               if opts.State != DOMElementStateDetached {
-                       return nil, fmt.Errorf("waiting for selector %q did not result in any nodes", selector)
-               }
-               return nil, nil //nolint:nilnil
+               return nil, fmt.Errorf("waiting for selector %q did not result in any nodes", selector)
        }
 
        // We always return ElementHandles in the main execution context (aka "DOM world")
@@ -496,6 +490,18 @@ func (f *Frame) waitForSelector(selector string, opts *FrameWaitForSelectorOptio
        return handle, nil
 }
 
+func (f *Frame) waitFor(selector string, opts *FrameWaitForSelectorOptions) error {
+       f.log.Debugf("Frame:waitFor", "fid:%s furl:%q sel:%q", f.ID(), f.URL(), selector)
+
+       document, err := f.document()
+       if err != nil {
+               return err
+       }
+
+       _, err = document.waitForSelector(f.ctx, selector, opts)
+       return err
+}
+
 // AddScriptTag is not implemented.
 func (f *Frame) AddScriptTag(opts goja.Value) {
        k6ext.Panic(f.ctx, "Frame.AddScriptTag() has not been implemented yet")
diff --git a/common/locator.go b/common/locator.go
index 9720e56..155c67a 100644
--- a/common/locator.go
+++ b/common/locator.go
@@ -620,6 +620,5 @@ func (l *Locator) WaitFor(opts goja.Value) {
 
 func (l *Locator) waitFor(opts *FrameWaitForSelectorOptions) error {
        opts.Strict = true
-       _, err := l.frame.waitForSelector(l.selector, opts)
-       return err
+       return l.frame.waitFor(l.selector, opts)
 }

[inancgumus]

Nice fix 👍

[ankur22]

If you mean deleting this line specifically from the injected_script.js, I can take care of that in this same PR if you agree. As I mentioned, I think it can be misleading considering that waiting for a detached element is valid, and in that case the user would see that log line.

Actually, let's leave it for now... we might lose some valuable information if we delete this log.

See the following diff for the changes to apply from this PRs current version:

Yep, that's exactly what i was trying to get across, nice 👍

[ka3de]

Actually, let's leave it for now... we might lose some valuable information if we delete this log.

👍

Yep, that's exactly what i was trying to get across, nice +1

Ok! Let me apply these changes, I think you are right, they simplify the implementation at the Frame level.

[ka3de]

Ok! Let me apply these changes, I think you are right, they simplify the implementation at the Frame level.

See 8d6e69a and a1dada8 @ankur22 .

[ankur22]

LGTM

[ka3de]

Rebasing main and merging.