Bug 189624 - [IntersectionObserver] Handle zero-area intersections
Summary: [IntersectionObserver] Handle zero-area intersections
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on: 189833
Blocks: 159475
  Show dependency treegraph
 
Reported: 2018-09-14 10:50 PDT by Ali Juma
Modified: 2018-10-19 06:48 PDT (History)
10 users (show)

See Also:


Attachments
Patch (54.72 KB, patch)
2018-09-14 11:01 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (depends on bug 189833) (14.10 KB, patch)
2018-10-03 08:28 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch for landing (14.29 KB, patch)
2018-10-18 14:51 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2018-09-14 10:50:56 PDT
Use edge-inclusive intersection so that two rects that touch each other are considered intersecting even if the area of their intersection is 0.
Comment 1 Ali Juma 2018-09-14 11:01:40 PDT
Created attachment 349778 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-09-14 13:32:22 PDT
Comment on attachment 349778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349778&action=review

> Source/WebCore/dom/Document.cpp:7500
> +static bool computeIntersectionRects(FrameView& frameView, IntersectionObserver& observer, Element& target, FloatRect& absTargetRect, FloatRect& absIntersectionRect, FloatRect& absRootBounds, bool& isIntersecting)

Please add a comment saying what the return value means. Or maybe you can use an optional<> return value?

> Source/WebCore/dom/Document.cpp:7541
> +    FloatRect rootLocalIntersectionRect = targetRenderer->computeRectForRepaint(localTargetBounds, rootRenderer, {false /* hasPositionFixedDescendant */, false /* dirtyRectIsFlipped */, true /* useEdgeInclusiveIntersection */}, &isIntersecting);

I know we were using computeRectForRepaint before, but it's wrong to complexify all the repaint logic with edge intersection for this purpose. The name "computeRectForRepaint" makes it clear that the function is about repaint; we should not be piggy-backing on it for intersection observer (we may want to change how repaint works in future, so we should not make intersection observer dependent on the repaint code).

You need to factor the "compute repaint rect" code into functions that you can call for intersection observer, and can be used by our current repaint logic.

> Source/WebCore/dom/Document.cpp:7589
> +                intersectionRatio = 1;

This is really confusing. Is this the "touching" rects case? Why would intersectionRatio be 1 if they just touch?

> Source/WebCore/dom/Document.cpp:7606
> +                    clientIntersectionRect = isIntersecting ? frameView->absoluteToClientRect(absIntersectionRect) : FloatRect();

": FloatRect()" can be ":  { }"

> Source/WebCore/platform/graphics/LayoutRect.cpp:77
> +    LayoutPoint newLocation(std::max(x(), other.x()), std::max(y(), other.y()));
> +    LayoutPoint newMaxPoint(std::min(maxX(), other.maxX()), std::min(maxY(), other.maxY()));

Shame that this code looks different than the FloatRect code.
Comment 3 Ali Juma 2018-10-03 08:28:00 PDT
Created attachment 351522 [details]
Patch (depends on bug 189833)
Comment 4 Ali Juma 2018-10-03 08:39:13 PDT
Comment on attachment 349778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349778&action=review

>> Source/WebCore/dom/Document.cpp:7500
>> +static bool computeIntersectionRects(FrameView& frameView, IntersectionObserver& observer, Element& target, FloatRect& absTargetRect, FloatRect& absIntersectionRect, FloatRect& absRootBounds, bool& isIntersecting)
> 
> Please add a comment saying what the return value means. Or maybe you can use an optional<> return value?

Changed to using an optional<> return value.

>> Source/WebCore/dom/Document.cpp:7589
>> +                intersectionRatio = 1;
> 
> This is really confusing. Is this the "touching" rects case? Why would intersectionRatio be 1 if they just touch?

This is the case of a target whose width or height is zero (so its area is zero). In this case, we can't compute the intersection ratio in the usual way because of division-by-zero, so the spec defines it to be 1 if the target intersects the root and 0 otherwise. I've reorganized this code a bit to try to make it clearer.

>> Source/WebCore/dom/Document.cpp:7606
>> +                    clientIntersectionRect = isIntersecting ? frameView->absoluteToClientRect(absIntersectionRect) : FloatRect();
> 
> ": FloatRect()" can be ":  { }"

The compiler doesn't like this ("Initializer list cannot be used on the right hand side of operator '?'"). So changed to using an if statement (clientIntersectionRect is already the empty rect to begin with).
Comment 5 Simon Fraser (smfr) 2018-10-11 16:22:09 PDT
Comment on attachment 351522 [details]
Patch (depends on bug 189833)

View in context: https://bugs.webkit.org/attachment.cgi?id=351522&action=review

> Source/WebCore/dom/Document.cpp:7521
> +    bool isIntersecting;

Initialize isIntersecting with { false }?

> Source/WebCore/dom/Document.cpp:7524
> +static std::optional<IntersectionObservationState> computeIntersectionState(FrameView& frameView, IntersectionObserver& observer, Element& target)

Can observer be a const&?
Comment 6 Ali Juma 2018-10-18 14:51:45 PDT
Created attachment 352736 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2018-10-19 06:47:34 PDT
Comment on attachment 352736 [details]
Patch for landing

Clearing flags on attachment: 352736

Committed r237284: <https://trac.webkit.org/changeset/237284>
Comment 8 WebKit Commit Bot 2018-10-19 06:47:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-10-19 06:48:28 PDT
<rdar://problem/45404221>