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

feat(waitFor): update various waitFor options to be a single boolean #1066

Merged
merged 1 commit into from
Feb 22, 2020
Merged

feat(waitFor): update various waitFor options to be a single boolean #1066

merged 1 commit into from
Feb 22, 2020

Conversation

dgozman
Copy link
Contributor

@dgozman dgozman commented Feb 19, 2020

Default waitFor: true now waits for element to exist, stop moving and become a hit target in input actions, and only only exist in non-input actions.

Note that this removes the ability to wait for visible from fill() and select() methods.

@dgozman dgozman requested a review from pavelfeldman February 19, 2020 21:25
@pavelfeldman
Copy link
Member

At the very minimum you should throw upon old options being passed

@dgozman
Copy link
Contributor Author

dgozman commented Feb 19, 2020

At the very minimum you should throw upon old options being passed

I already do.

Copy link
Member

@pavelfeldman pavelfeldman left a comment

Choose a reason for hiding this comment

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

  • Why not to leave the visible option?

  • How do I wait for node to be hidden?

  • When I say page.click({waitFor:{exists:false}), does it mean I wait for it to hide or I don't wait? It is not clear whether I should be clearing static and hittarget bits if I just don't want to wait for anything. In this unlikely-to-occur case, I would prefer page.click(waitFor:nowait).

It seems like we have at least three input wait strategies:

  • Wait by all means until it appears, stops and is clear to click. Time out if that did not happen. This option is most convenient, but it fails with a timeout. Since we know timeout is not a thing in the cloud, we need better signals to fail upon. Like no pending XHRs, timers or animations.

  • Don't wait at all, assert that it is there and receives the click, throw if it isn't. Resolves immediately.

  • Something in the middle for efficiency? Wait for dom, but don't wait for visible, hit target or static? Does it really make it more efficient? Do I ever want it?

docs/api.md Outdated
- `waitFor` <[Object]>
- `exists` <[boolean]> Wait for the element to be present in the dom. Defaults to `true`.
- `static` <[boolean]> Wait for the element to stop moving (for example, until css transition finishes). Defaults to `true`.
- `hittarget` <[boolean]> Wait for the element to potentially receive pointer evetns at the click point (for example, to become non-obscured by other elements). Defaults to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

evetns

@dgozman
Copy link
Contributor Author

dgozman commented Feb 20, 2020

  • Why not to leave the visible option?

visible is not clearly defined, and is superseded by static + hittarget.

  • How do I wait for node to be hidden?

page.waitForSelector({visibility: 'hidden'}). That said, I would prefer a separate method like page.waitForSelectorHidden or page.waitForSelectorMissing.

  • When I say page.click({waitFor:{exists:false}), does it mean I wait for it to hide or I don't wait?
    This means don't wait.

It is not clear whether I should be clearing static and hittarget bits if I just don't want to wait for anything. In this unlikely-to-occur case, I would prefer page.click(waitFor:nowait).

Sounds reasonable.

  • Something in the middle for efficiency? Wait for dom, but don't wait for visible, hit target or static? Does it really make it more efficient? Do I ever want it?

I don't think efficiency is a concern here. The usecase I can think of is "I would like to assert that it's clickable in the state the page is in". That would be the "no wait" scenario. If we don't find compelling usecases for middle ground, we can go with wait: boolean option instead. WDYT?

@pavelfeldman
Copy link
Member

If we don't find compelling usecases for middle ground, we can go with wait: boolean option instead. WDYT?

Yes, this is what I was hinting at. With respect to present API, it would mean that waitForVisible gains waitForInterqctable semantics and becomes waitFor. It can be boolean today, and we will be able to say boolean|object in the future to include all your fine-grained bits. I don't think we should commit to bits in v1, but we leave ourselves this option.

Default waitFor: true now waits for element to exist, stop moving and
become a hit target in input actions, and only only exist in non-input actions.

Note that this removes the ability to wait for visible from fill() and select()
methods.
@dgozman dgozman changed the title feat(waitFor): update various waitFor options feat(waitFor): update various waitFor options to be a single boolean Feb 20, 2020
@dgozman
Copy link
Contributor Author

dgozman commented Feb 20, 2020

Please take another look, using a boolean now.

test/click.spec.js Show resolved Hide resolved
@pavelfeldman pavelfeldman merged commit 1f8508d into microsoft:master Feb 22, 2020
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