Bug 189624 – [IntersectionObserver] Handle zero-area intersections

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ali Juma]

Use edge-inclusive intersection so that two rects that touch each other are considered intersecting even if the area of their intersection is 0.

[Ali Juma]

Created attachment 349778 [details]
Patch

[Simon Fraser (smfr)]

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.

[Ali Juma]

Created attachment 351522 [details]
Patch (depends on bug 189833)

[Ali Juma]

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).

[Simon Fraser (smfr)]

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&?

[Ali Juma]

Created attachment 352736 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 352736 [details]
Patch for landing

Clearing flags on attachment: 352736

Committed r237284: <https://trac.webkit.org/changeset/237284>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45404221>