feat(elementhandle): introduce elementHandle.isIntersectingViewport() method. #2673

b-ponomarenko · 2018-06-04T13:11:07Z

Add API to define is element visible in viewport via Intersection Observer

References #2629

googlebot · 2018-06-04T13:11:11Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

b-ponomarenko · 2018-06-04T13:13:26Z

I signed it!

googlebot · 2018-06-04T13:13:28Z

CLAs look good, thanks!

b-ponomarenko · 2018-06-04T13:28:00Z

Can I rerun node6 (windows)?

b-ponomarenko · 2018-06-05T13:23:15Z

@aslushnikov please check

aslushnikov

Thanks, this looks promising

As part of this PR, let's change waitForSelector implementation to use intersection observer; this way we won't have inconsistencies like this:

const e = await page.waitForSelector('div', {visible: true});
await e.isVisible(); // false

aslushnikov · 2018-06-06T14:53:22Z

docs/api.md

@@ -219,6 +219,7 @@
  * [elementHandle.getProperties()](#elementhandlegetproperties)
  * [elementHandle.getProperty(propertyName)](#elementhandlegetpropertypropertyname)
  * [elementHandle.hover()](#elementhandlehover)
+  * [elementHandle.isDisplayed()](#elementhandleisdisplayed)


elementHandle.isVisible() is better since it aligns with waitForSelector's "visible" option.

Ok, I will rename to isVisible

aslushnikov · 2018-06-06T14:57:07Z

lib/ElementHandle.js

+            resolve(entries[0].isIntersecting);
+            observer.unobserve(node);
+          };
+          const observer = new IntersectionObserver(callback, { root: null, rootMargin: '0px', threshold: 1.0 });


so when will the callback be called? Is it always called initially?

Yes, it will be called initially and then unobserve from the node

aslushnikov · 2018-06-06T15:00:41Z

test/elementhandle.spec.js

+      const box = await page.$('.box');
+
+      expect(await box.isDisplayed()).toBe(false);
+    });


can we also add a test that checks for the truthy value?

Yes, I'll add this test case soon

aslushnikov · 2018-06-06T15:00:58Z

test/assets/hidden-and-visible.html

@@ -0,0 +1,23 @@
+<html>


let's use page.setContent instead of a separate asset.

JoelEinbinder · 2018-06-06T15:11:30Z

If we change waitForSelector to use intersection observer, won’t that cause elements below the fold to be considered invisible? That’s a pretty major change, and I’m not sure if it makes that feature more or less useful. “Visible” vs “would visible if you scroll down” is hard to define. And it conflicts with the original use case for this feature, which was to report elements ina carousel as invisible even though they could be scrolled to horizontally.

aslushnikov · 2018-06-06T15:37:31Z

won’t that cause elements below the fold to be considered invisible?

@JoelEinbinder These will be considered invisible. This is a very good point though, we might need to postpone this till 2.0.

That’s a pretty major change, and I’m not sure if it makes that feature more or less useful.

Usefulness is debatable, but API sanity will improve:

Our current isVisible is more like isClickable, it's quite quirky with a huge grey area of what is considered "visible".
Visibility defined by intersection observer is aligned with a natural perception of visibility, it's easy to explain and understand.

Add API to define is element visible in viewport via Intersection Observer References #2629

b-ponomarenko · 2018-06-12T20:09:18Z

@aslushnikov change implementation of _waitForSelectorOrXPath, but in some cases this test fails (when I run this test in single mode almost always test passed), what can be wrong?

upd
no longer relevant, understood the problem and fix

b-ponomarenko · 2018-06-18T08:00:02Z

@aslushnikov please check PR

aslushnikov · 2018-06-19T17:15:06Z

@aslushnikov please check PR

@b-ponomarenko We discussed this PR with @JoelEinbinder one more time.

Summary:

there are two types of visibility: visible-in-viewport (this implementation) and displayed-in-page (currently used for visible option in waitForSelector)
both make perfect sense from API standpoint: visible-in-viewport comes handy to detect what user sees, and displayed-in-page is more like "interactable", e.g. something that could be clicked.
changing waitForSelector's visible semantics from displayed-in-page to visible-in-viewport is a breaking change; we can't afford it yet.

So the way forward with this:

disregard my suggestion on using IntersectionObserver for waitForSelector. (Sorry for misguiding you)
introduce a new distinct method on ElementHandle, e.g. ElementHandle.isVisibleInViewport, that uses Intersection Observer.

As a part of 2.0 effort, we might want to do a better job separating "interactivity" vs "visibility", e.g. renaming visible into interactable. I've added a bullet.

…ElementHandle.isVisible to ElementHandle.isVisibleInViewport

b-ponomarenko · 2018-06-20T12:02:39Z

@aslushnikov I canceled using Intersection Observer in _waitForSelectorOrXPath. What's next? Really, maybe rename visible property to interactable or some other property name, and move current implementation via Intersection Observer to visible property?

aslushnikov

We discussed this with @JoelEinbinder and renamed into elementHandle.isIntersectingViewport(). Thanks for the PR!

justnpT · 2019-03-07T09:12:09Z

I wouldn't be so sure that inputElement.isIntersectingViewport() is true only when an element is visible for human eyes, I have a case where element didn't yet appear as visible for eyes and
await element.isIntersectingViewport()
returns true

aslushnikov · 2019-03-08T16:45:57Z

I have a case where element didn't yet appear as visible for eyes and
await element.isIntersectingViewport()
returns true

@justnpT interesting! Can you please file a new issue with a repro for your usecase?

b-ponomarenko changed the title ~~feat(ElementHandle) Add isDisplayed method to ElementHandle~~ featutre: Add isDisplayed method to ElementHandle Jun 4, 2018

b-ponomarenko changed the title ~~featutre: Add isDisplayed method to ElementHandle~~ feat: Add isDisplayed method to ElementHandle Jun 4, 2018

aslushnikov reviewed Jun 6, 2018

View reviewed changes

feat(ElementHandle) Add isDisplayed method to ElementHandle

4cdb09a

Add API to define is element visible in viewport via Intersection Observer References #2629

Fix failed test: 'hidden should wait for display: none'

d780e25

aslushnikov mentioned this pull request Jun 19, 2018

v2.0 Breaking Changes #2079

Closed

13 tasks

Revert using IntersectionObserver in _waitForSelectorOrXPath, rename …

c0ad836

…ElementHandle.isVisible to ElementHandle.isVisibleInViewport

aslushnikov added 4 commits July 11, 2018 16:58

Update api.md

920ef3e

Update ElementHandle.js

55de870

Update elementhandle.spec.js

546a13f

Update api.md

b51dd7b

aslushnikov changed the title ~~feat: Add isDisplayed method to ElementHandle~~ feat(elementhandle): introduce elementHandle.isIntersectingViewport() method. Jul 12, 2018

aslushnikov approved these changes Jul 12, 2018

View reviewed changes

aslushnikov merged commit 96c558d into puppeteer:master Jul 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(elementhandle): introduce elementHandle.isIntersectingViewport() method. #2673

feat(elementhandle): introduce elementHandle.isIntersectingViewport() method. #2673

b-ponomarenko commented Jun 4, 2018

googlebot commented Jun 4, 2018

b-ponomarenko commented Jun 4, 2018

googlebot commented Jun 4, 2018

b-ponomarenko commented Jun 4, 2018

b-ponomarenko commented Jun 5, 2018

aslushnikov left a comment

aslushnikov Jun 6, 2018

b-ponomarenko Jun 7, 2018

aslushnikov Jun 6, 2018

b-ponomarenko Jun 7, 2018

aslushnikov Jun 6, 2018

b-ponomarenko Jun 7, 2018 •

edited

aslushnikov Jun 6, 2018

JoelEinbinder commented Jun 6, 2018

aslushnikov commented Jun 6, 2018

b-ponomarenko commented Jun 12, 2018 •

edited

b-ponomarenko commented Jun 18, 2018

aslushnikov commented Jun 19, 2018

b-ponomarenko commented Jun 20, 2018 •

edited

aslushnikov left a comment

justnpT commented Mar 7, 2019 •

edited

aslushnikov commented Mar 8, 2019

feat(elementhandle): introduce elementHandle.isIntersectingViewport() method. #2673

feat(elementhandle): introduce elementHandle.isIntersectingViewport() method. #2673

Conversation

b-ponomarenko commented Jun 4, 2018

googlebot commented Jun 4, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

b-ponomarenko commented Jun 4, 2018

googlebot commented Jun 4, 2018

b-ponomarenko commented Jun 4, 2018

b-ponomarenko commented Jun 5, 2018

aslushnikov left a comment

Choose a reason for hiding this comment

aslushnikov Jun 6, 2018

Choose a reason for hiding this comment

b-ponomarenko Jun 7, 2018

Choose a reason for hiding this comment

aslushnikov Jun 6, 2018

Choose a reason for hiding this comment

b-ponomarenko Jun 7, 2018

Choose a reason for hiding this comment

aslushnikov Jun 6, 2018

Choose a reason for hiding this comment

b-ponomarenko Jun 7, 2018 • edited

Choose a reason for hiding this comment

aslushnikov Jun 6, 2018

Choose a reason for hiding this comment

JoelEinbinder commented Jun 6, 2018

aslushnikov commented Jun 6, 2018

b-ponomarenko commented Jun 12, 2018 • edited

b-ponomarenko commented Jun 18, 2018

aslushnikov commented Jun 19, 2018

b-ponomarenko commented Jun 20, 2018 • edited

aslushnikov left a comment

Choose a reason for hiding this comment

justnpT commented Mar 7, 2019 • edited

aslushnikov commented Mar 8, 2019

b-ponomarenko Jun 7, 2018 •

edited

b-ponomarenko commented Jun 12, 2018 •

edited

b-ponomarenko commented Jun 20, 2018 •

edited

justnpT commented Mar 7, 2019 •

edited