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 isVisible #1108

Merged
merged 4 commits into from Nov 23, 2023
Merged

Fix isVisible #1108

merged 4 commits into from Nov 23, 2023

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Nov 22, 2023

What?

This fixes isVisible so that it does not wait for an element to match with the given selector, and returns straight away. This makes the timeout option obsolete.

Why?

There are two reason to make this change:

  1. It doesn't make sense to wait for an element to match before checking whether it is visible;
  2. It will match Playwrights behaviour which is what some users will expect.

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: #981

@ankur22 ankur22 changed the base branch from main to refactor/query November 22, 2023 15:01
@ankur22 ankur22 force-pushed the fix/isVisible branch 2 times, most recently from b0a3d61 to c368543 Compare November 22, 2023 15:11
@ankur22 ankur22 changed the title Fix isVisible on frame and page Fix isVisible Nov 22, 2023
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.

👍 Some suggestions.

@ankur22 Does this conform to auto-waiting actionability checks? https://playwright.dev/docs/actionability

common/frame.go Outdated Show resolved Hide resolved
@ankur22
Copy link
Collaborator Author

ankur22 commented Nov 23, 2023

@ankur22 Does this conform to auto-waiting actionability checks? https://playwright.dev/docs/actionability

No, this isn't a prerequisite for the isVisiblity check. In the link you posted, visible is an actionability check for other APIs.

This method is to be used when an API needs to:
1. query for a handler given a selector with or without strict mode
   enabled.
2. Perform an action on the handler if one was found, otherwise return
   false.
3. Not wait for the element that matches the given selector.

This will make the timeout option obsolete on APIs that work with this.
isVisible is changed to work with the new runActionOnSelector method,
which does not wait for any elements to match the given selector. It
will just return false if there are no matching elements.

This also means that timeout is an obsolete option, which means that
the test in locator_test is not valid and has been removed.
@ankur22 ankur22 merged commit 7a28b3a into main-next Nov 23, 2023
14 checks passed
@ankur22 ankur22 deleted the fix/isVisible branch November 23, 2023 10:57
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.

None yet

2 participants