Intersection Observer first draft

Intersection Observer first draft

https://github.com/slightlyoff/PositionObserver/blob/master/explainer.md

BUG=

[MikeB, 2015-09-03 22:54:59]

mpb@chromium.org changed reviewers:
+ igrigorik@chromium.org, liberato@chromium.org, ojan@chromium.org

[MikeB, 2015-09-03 22:56:46]

This wasn't based on 
https://codereview.chromium.org/1246173002/ or
https://codereview.chromium.org/1179223002/ so it may need to be changed some,
but this is the basic idea I had so far.

Any comments or suggestions are more than welcome!
Thanks,
-Mike

[liberato (no reviews please), 2015-09-04 15:07:46]

this is really exciting stuff!  thanks for adding me to the CL.

mainly i have two high-level questions. certainly, i'm unfamiliar with this
stuff, so please don't hesitate to assume that i've entirely missed some point
or other.  :)

apart from a few nits in FrameView that happened to be in front of me, i didn't
try to do a thorough review until i'm sure that i'm on the same page.

thanks
-fl

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1131: if (FrameView* parent = parentFrameView())
{
how will you handle different containing views ("viewport:" in the doc)?

if every PositionObserver can specify its own element as its viewport, then
FrameView might not be the right place for this.  perhaps just notify LayoutView
what the default viewport bounds are currently, and let it iterate over position
observers.

also, relatedly, do we really need viewportModifierLength at all?  one can just
stick the content in a larger div, itself inside an overflow:hidden div, then
set up an observer using the inner, larger one.

i realize i'm making a few assumptions in there about interpretation of
"visible".  seems like a useful thing.

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1139: m_clippedBounds =
pixelSnappedIntRect(paintInvalidationState.clipRect());
is this correct if one zooms?  i believe so.

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1143: for (auto& obj :
rootForPaintInvalidation.intersectionObserverTargets()) {
independently of the above comment about moving this entire fn into LayoutView,
this loop might still go better in LayoutView --
LayoutView::someVerbIntersectionObservers(viewport).  the loop just accesses
LayoutView state indirectly anyway.

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1144: Node* node = obj.key->node();
for clarity, "LayoutObject* layoutObect = obj.key"

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1155: quads.append(bounds);
this brings up a question i had about the doc.  i'm a little unclear about the
vector of quads per observation.  are you picturing eventually checking for 
"element is in viewport but partially occluded by this other element", so that
it has a non-rectangular visible region?  or are there other ways to get
non-rectangular intersections that i'm missing?

personally, i don't quite see the value in that.  given notification that
something has moved / scrolled / etc., JS can fairly easily do additional
intersection tests as it likes.  the key value that i see is detecting those
changes in an efficient way with respect to some enclosing element or default
viewport, and including the "thresholdCallbacks" debounce logic.

same for questions about alpha / opacity raised in the doc.  seems really tricky
and hard to get behavior that makes everybody happy.

[MikeB, 2015-09-04 17:59:34]

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1131: if (FrameView* parent = parentFrameView())
{
On 2015/09/04 15:07:46, liberato wrote:
> how will you handle different containing views ("viewport:" in the doc)?
> 
> if every PositionObserver can specify its own element as its viewport, then
> FrameView might not be the right place for this.  perhaps just notify
LayoutView
> what the default viewport bounds are currently, and let it iterate over
position
> observers.
> 
> also, relatedly, do we really need viewportModifierLength at all?  one can
just
> stick the content in a larger div, itself inside an overflow:hidden div, then
> set up an observer using the inner, larger one.
> 
> i realize i'm making a few assumptions in there about interpretation of
> "visible".  seems like a useful thing.

I think viewportModifierLength is being dropped. I'm not entirely convinced we
need the 'viewport' option either.

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1139: m_clippedBounds =
pixelSnappedIntRect(paintInvalidationState.clipRect());
On 2015/09/04 15:07:46, liberato wrote:
> is this correct if one zooms?  i believe so.

Good question. I can test!

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1143: for (auto& obj :
rootForPaintInvalidation.intersectionObserverTargets()) {
On 2015/09/04 15:07:46, liberato wrote:
> independently of the above comment about moving this entire fn into
LayoutView,
> this loop might still go better in LayoutView --
> LayoutView::someVerbIntersectionObservers(viewport).  the loop just accesses
> LayoutView state indirectly anyway.

Fair point.

https://codereview.chromium.org/1330633003/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1155: quads.append(bounds);
On 2015/09/04 15:07:46, liberato wrote:
> this brings up a question i had about the doc.  i'm a little unclear about the
> vector of quads per observation.  are you picturing eventually checking for 
> "element is in viewport but partially occluded by this other element", so that
> it has a non-rectangular visible region?  or are there other ways to get
> non-rectangular intersections that i'm missing?
> 
> personally, i don't quite see the value in that.  given notification that
> something has moved / scrolled / etc., JS can fairly easily do additional
> intersection tests as it likes.  the key value that i see is detecting those
> changes in an efficient way with respect to some enclosing element or default
> viewport, and including the "thresholdCallbacks" debounce logic.
> 
> same for questions about alpha / opacity raised in the doc.  seems really
tricky
> and hard to get behavior that makes everybody happy.

According to the guys who drafted that particular piece, it was intended to
account for skew transforms and things like that. I'm actually passing a vector
of a single rect every time right now. We'll iterate with the spec a bit.

[ojan, 2015-09-21 03:49:07]

ojan@chromium.org changed reviewers:
+ adamk@chromium.org, esprehn@chromium.org, vollick@chromium.org
- igrigorik@chromium.org, liberato@chromium.org

[ojan, 2015-09-21 03:49:09]

This looks close to me.

Adam, since a lot of this is a clone of the MutationObserver code, is there any
chance you could take a look? Specifically at the bits in the v8 directory and
the ActiveDOMObject stuff.

Vollick: Please see my question about clipping.

Also, FYI, we won't land this patch until we've sent the intent to implement.
We're finalizing the spec before we go there though.

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
File LayoutTests/fast/dom/intersection-observer.html (right):

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
LayoutTests/fast/dom/intersection-observer.html:4: <body>
Nit: No need for the body element

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
LayoutTests/fast/dom/intersection-observer.html:9: <div id="target_div" 
style="background-color:red;position:absolute; top:855px; left:66px;
width:300px; height:250px">
Please add test cases for the following or add appropriate TODOs. I'm not sure,
but I think this patch doesn't get these right.

1. Removing an element from the DOM removes its observation.
2. Clipping across frame boundaries works (you can use <iframe srcdoc> to make
these tests a bit easier to write)
3. Clipping from overflow:hidden/scroll/auto ancestors works.
4. Observing an element that's not appended to the DOM (should throw).

I think the patch gets the following right, but we should have test coverage for
it anyways:
1. Create an element, append it to the DOM, register an observer. Make sure that
the first callback you get has the right rects.
2. Calling observe leaving out the second argument should work.

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
LayoutTests/fast/dom/intersection-observer.html:56: var config = { entryTypes:
['mark', 'measure'] };
I think this is probably leftover from the test case you ported this from?

Which brings up a question actually, should passing in a dict with invalid
fields throw? I think the answer is no, but I'm not sure.

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
LayoutTests/fast/dom/intersection-observer.html:59: window.setTimeout(function()
{ window.scrollBy(0, 400); }, 0);
Do you need the setTimeout? I think you should be able to just force a layout
and then sync call scrollBy:

document.documentElement.offsetTop;
window.scrollBy(0, 400);

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
File Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserver.cpp:56: if (target)
How can target be null?

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserver.cpp:77: bool
IntersectionObserver::shouldBeSuspended() const
I see that MutationObservers are also ActiveDOMObjects, but it's not clear to me
why they are and whether IntersectionObserver should be also. Adam?

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
File Source/core/dom/IntersectionObserver.idl (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserver.idl:8: // https://
?

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserver.idl:16: sequence
<IntersectionObserverEntry> takeRecords();
Nit: no space after "sequence".

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
File Source/core/dom/IntersectionObserverEntry.idl (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserverEntry.idl:5: // https://
?

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserverEntry.idl:11: readonly attribute
ClientRectList quads;
I know this was after you uploaded the patch, but we changes this from quads to
boundingClientRect.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
File Source/core/dom/IntersectionObserverInit.idl (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserverInit.idl:5: // Spec:
http://dom.spec.whatwg.org/#idl-def-IntersectionObserverInit
This URL isn't right.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1129: if (FrameView* parent = parentFrameView())
{
Does this handle clipping correctly if you have multiple levels of nested
frames?

https://codereview.chromium.org/1330633003/diff/80001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1133: if (!m_clippedBounds.isEmpty()) {
Nit: no curlies

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:2490: if (view() &&
hasIntersectionObserver()) {
Ditto nits about curlies here and below

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:2555: if (view() &&
hasIntersectionObserver()) {
I'm not sure, but I believe this function gets called from
LayoutObject::willBeDestroyed() via the "remove()" call inside it. So, either
this one or the one in willBeDestroyed isn't needed (I think).

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutView.cpp:1029: LayoutObject* layoutObect = obj.key;
Typo: layoutObect

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutView.cpp:1033: FloatRect bounds =
layoutObect->absoluteBoundingBoxFloatRect();
I don't think this handles clipping from ancestor scrollers, does it? Vollick,
what's the right way to get an elements bounding box intersected with all its
ancestor clips? At the moment, the spec just says overflow clips, but we can
consider changing it to include clip-path, etc if that makes more sense.

[adamk, 2015-09-21 23:52:07]

https://codereview.chromium.org/1330633003/diff/80001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8IntersectionObserverCallback.h (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/bindings/core/v8...
Source/bindings/core/v8/V8IntersectionObserverCallback.h:18: class
V8IntersectionObserverCallback final : public IntersectionObserverCallback,
public ActiveDOMCallback {
This is now the second copy of V8MutationCallback over the past couple months,
it's really high time we generated it.

https://code.google.com/p/chromium/issues/detail?id=513855

is the bug for that, I'd like to see it fixed before this lands.

[MikeB, 2015-09-24 19:04:04]

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
File LayoutTests/fast/dom/intersection-observer.html (right):

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
LayoutTests/fast/dom/intersection-observer.html:4: <body>
On 2015/09/21 03:49:08, ojan wrote:
> Nit: No need for the body element

Done.

https://codereview.chromium.org/1330633003/diff/80001/LayoutTests/fast/dom/in...
LayoutTests/fast/dom/intersection-observer.html:56: var config = { entryTypes:
['mark', 'measure'] };
On 2015/09/21 03:49:08, ojan wrote:
> I think this is probably leftover from the test case you ported this from?
> 
> Which brings up a question actually, should passing in a dict with invalid
> fields throw? I think the answer is no, but I'm not sure.

Done.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
File Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserver.cpp:56: if (target)
On 2015/09/21 03:49:09, ojan wrote:
> How can target be null?

RefPtrWillBeWeakMember will become WeakMember when Oilpan is enabled. In that
case, it can become null if the referenced object is GC'ed.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
File Source/core/dom/IntersectionObserver.idl (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserver.idl:8: // https://
On 2015/09/21 03:49:09, ojan wrote:
> ?

Done.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserver.idl:16: sequence
<IntersectionObserverEntry> takeRecords();
On 2015/09/21 03:49:09, ojan wrote:
> Nit: no space after "sequence".

Done.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
File Source/core/dom/IntersectionObserverEntry.idl (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserverEntry.idl:5: // https://
On 2015/09/21 03:49:09, ojan wrote:
> ?

Done.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/dom/Interse...
Source/core/dom/IntersectionObserverEntry.idl:11: readonly attribute
ClientRectList quads;
On 2015/09/21 03:49:09, ojan wrote:
> I know this was after you uploaded the patch, but we changes this from quads
to
> boundingClientRect.

Done.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1133: if (!m_clippedBounds.isEmpty()) {
On 2015/09/21 03:49:09, ojan wrote:
> Nit: no curlies

Done.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:2490: if (view() &&
hasIntersectionObserver()) {
On 2015/09/21 03:49:09, ojan wrote:
> Ditto nits about curlies here and below

Done.

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1330633003/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutView.cpp:1029: LayoutObject* layoutObect = obj.key;
On 2015/09/21 03:49:09, ojan wrote:
> Typo: layoutObect

Done.