feat(page): waitForSelector hidden option #967
Conversation
|
I'm curious as to what the use case for this is. |
|
I'm using it for a screen where a modal is covering a button, click doesn't work till the modal is hidden |
|
It's really hard to determine proper visibility of an element. For example, these checks fail if an ancestor node has I almost wish |
|
@ebidel I agree, it's tricky. And it doesn't take opacity into account, but I haven't seen any implementations that consider opacity. But, I disagree that it should be left out. Developer time being the most valuable asset, visibility is an easy way to time events on a page that might be complicated otherwise. I added reverse and hidden options around the visible feature, both options visible and hidden climb the DOM to check for a hidden ancestor. #549 |
if the options visible or hidden are used, the waitForSelector function climbs up the DOM to check if each parent is display:none.
|
I'm using |
|
Hey @ChristianDavis, thanks for the PR. I like the await page.waitForSelector('.dialog', {hidden: true});However, I'd love it to be a full opposite of I don't like the I'd like to scope this PR to only add a |
|
@aslushnikov yeah, looks confusing. Someone had pointed out that webdriverio has an implementation { reverse: true }, but I should just combine the functionality into hidden |
|
{hidden: true} checks for NOT present, or hidden, or ancestors hidden |
|
@ChristianDavis This PR now seems to be doing two things:
Could you please split the PR into two separate ones? This will make for a nicer history and smaller diffs. Also, I feel like we've agreed on 'hidden' option, but there might be some questions on the hierarchy traversal. |
|
@ebidel didn't think hidden OR visible makes sense without the ancestor check, and that's probably true. But I could split it up |
|
My point was really that checking "visibility" is hard to get right and we should consider removing the option. But may people like the convenience and it's pretty nice for the simpler cases. I'm a fan of making the existing checks more reliable. So traversing the DOM to check ancestors makes sense to me. To save some work, you could also cut out early in the case of Splitting these up into separate PRs sgtm. |
Dropped the ancestor check
|
feature is now just the hidden option, waits for not present or hidden |
lib/FrameManager.js
Outdated
| return style && style.display !== 'none' && style.visibility !== 'hidden'; | ||
| if (waitForVisible) | ||
| return style && style.display !== 'none' && style.visibility !== 'hidden'; | ||
| return style && (style.display === 'none' || style.visibility === 'hidden'); |
There was a problem hiding this comment.
Technically the !style makes more sense here.
How about:
const style = window.getComputedStyle(node);
const isVisible = style && style.display !== 'none' && style.visibility !== 'hidden';
return waitForVisible === isVisible;There was a problem hiding this comment.
looks better
return ( waitForVisible === isVisible || waitForHidden === !isVisible);
test/test.js
Outdated
| it('hidden should wait for visibility: hidden', SX(async function() { | ||
| let divHidden = false; | ||
| await page.setContent(`<div style='display: block;'></div>`); | ||
| expect(divHidden).toBe(false); |
There was a problem hiding this comment.
nit - here and in other tests: please drop this check, it's a noop
|
@ChristianDavis thanks! |
|
@aslushnikov When will this be released to npm? |
|
Is this valid for |
|
@vsemozhetbyt You're right, I didn't notice that. Page.waitfor is actually just abstracting the frame implementation. Maybe the options for the frame.waitFor could be linked, or vice versa? |
|
I am not sure, but it seems all the other descriptions are copied when similar. |
|
@ChristianDavis we copy descriptions: it's less clicks for users to navigate documentation. |
This patch adds a 'hidden' option for the `page.waitForSelector` method.
Waits for a selector to be present and hidden, opposite behavior of the option visible.