Bug 189833 – [IntersectionObserver] Factor out rect mapping and clipping logic from computeRectForRepaint

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ali Juma]

Split out from bug 189624.

For IntersectionObserver, we need a version of computeRectForRepaint that uses edge-inclusive intersection and always applies all clips and scrolls, regardless of what's composited. So we need to extract the rect mapping and clipping logic out of computeRectForRepaint into another method that can be called both by computeRectForRepaint and by IntersectionObserver.

[Ali Juma]

Created attachment 350370 [details]
WIP for EWS

[EWS Watchlist]

Comment on attachment 350370 [details]
WIP for EWS

Attachment 350370 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9297545

New failing tests:
svg/custom/image-parent-translation.xhtml
fast/repaint/selection-ruby-rl.html

[EWS Watchlist]

Created attachment 350375 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350370 [details]
WIP for EWS

Attachment 350370 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9297611

New failing tests:
svg/custom/image-parent-translation.xhtml

[EWS Watchlist]

Created attachment 350377 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350370 [details]
WIP for EWS

Attachment 350370 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9297726

New failing tests:
svg/custom/image-parent-translation.xhtml

[EWS Watchlist]

Created attachment 350390 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 350370 [details]
WIP for EWS

Attachment 350370 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9297938

New failing tests:
svg/custom/image-parent-translation.xhtml
fast/repaint/selection-ruby-rl.html

[EWS Watchlist]

Created attachment 350396 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 350370 [details]
WIP for EWS

Attachment 350370 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9299550

New failing tests:
fast/repaint/selection-ruby-rl.html
svg/custom/image-parent-translation.xhtml

[EWS Watchlist]

Created attachment 350406 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ali Juma]

Created attachment 350814 [details]
WIP for EWS

[EWS Watchlist]

Comment on attachment 350814 [details]
WIP for EWS

Attachment 350814 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9350713

New failing tests:
fast/frames/lots-of-iframes.html
imported/w3c/web-platform-tests/encoding/single-byte-decoder.html
fast/frames/frame-limit.html

[EWS Watchlist]

Created attachment 350834 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Ali Juma]

Created attachment 350864 [details]
Patch

[Ali Juma]

Comment on attachment 350864 [details]
Patch

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

> Source/WebCore/rendering/RenderInline.cpp:915
> +        // FIXME: Respect the value of context.m_applyCompositedContainerScrolls.

We weren't calling shouldApplyCachedClipAndScrollPositionForRepaint() here before, though it seems like that was a bug since everywhere else we do call that method before calling applyCachedClipAndScrollPositionForRepaint(). But to avoid a behavior change in this patch, I've left this as a FIXME.

[Radar WebKit Bug Importer]

<rdar://problem/44916139>

[Ali Juma]

Created attachment 351409 [details]
Patch

Rebased

[Simon Fraser (smfr)]

Comment on attachment 351409 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:973
> +bool RenderBox::applyCachedClipAndScrollPosition(LayoutRect& rect, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const

I think this needs a better name, or maybe shouldn't return a bool, since only some callers care about intersection. Perhaps the intersection-related stuff should be in a 'context' object pass as an argument?

> Source/WebCore/rendering/RenderBox.cpp:2138
> +LayoutRect RenderBox::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const

I think this is relative to the repaint container, not "absolute". The comments say that layoutState->paintOffset() is offset from the layout root, but I think it's probably the repaint root.

Maybe just computeVisibleRectUsingPaintOffset()

> Source/WebCore/rendering/RenderBox.cpp:2259
> +        bool isEmpty = !containerBox.applyCachedClipAndScrollPosition(adjustedRect, container, context.m_applyCompositedClips, context.m_applyCompositedContainerScrolls, context.m_useEdgeInclusiveIntersection);

Maybe just pass VisibleRectContext down

> Source/WebCore/rendering/RenderBox.h:574
> +    bool applyCachedClipAndScrollPosition(LayoutRect&, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const;

A comment should say what the return value means.

> Source/WebCore/rendering/RenderInline.cpp:849
> +        containingBlock->applyCachedClipAndScrollPosition(repaintRect, repaintContainer, false /* applyCompositedClips */, shouldApplyCompositedContainerScrollsForRepaint(), false /* useEdgeInclusiveIntersection */);

Ick!

> Source/WebCore/rendering/RenderInline.cpp:874
> +LayoutRect RenderInline::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const

Needs same rename.

> Source/WebCore/rendering/RenderObject.h:699
> +    // Given a rect in the object's coordinate space, compute the location in container space where this rect is visible,
> +    // when clipping and scrolling as specified by the context. When using edge-inclusive intersection, return std::nullopt
> +    // rather than an empty rect if the rect is completely clipped out in container space.
> +    struct VisibleRectContext {
> +        VisibleRectContext(bool hasPositionFixedDescendant = false, bool dirtyRectIsFlipped = false, bool useEdgeInclusiveIntersection = false, bool applyCompositedClips = true, bool applyCompositedContainerScrolls = true)
>              : m_hasPositionFixedDescendant(hasPositionFixedDescendant)
>              , m_dirtyRectIsFlipped(dirtyRectIsFlipped)
> +            , m_useEdgeInclusiveIntersection(useEdgeInclusiveIntersection)
> +            , m_applyCompositedClips(applyCompositedClips)
> +            , m_applyCompositedContainerScrolls(applyCompositedContainerScrolls)
>              {
>              }
>          bool m_hasPositionFixedDescendant;
>          bool m_dirtyRectIsFlipped;
> +        bool m_useEdgeInclusiveIntersection;
> +        bool m_applyCompositedClips;
> +        bool m_applyCompositedContainerScrolls;

I think this could just be an enum and an OptionSet<>

[Ali Juma]

Created attachment 352140 [details]
Patch

[Ali Juma]

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

>> Source/WebCore/rendering/RenderBox.cpp:973
>> +bool RenderBox::applyCachedClipAndScrollPosition(LayoutRect& rect, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const
> 
> I think this needs a better name, or maybe shouldn't return a bool, since only some callers care about intersection. Perhaps the intersection-related stuff should be in a 'context' object pass as an argument?

Based on our offline discussion, I added a comment saying what the return value means, but left the name as-is.

>> Source/WebCore/rendering/RenderBox.cpp:2138
>> +LayoutRect RenderBox::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const
> 
> I think this is relative to the repaint container, not "absolute". The comments say that layoutState->paintOffset() is offset from the layout root, but I think it's probably the repaint root.
> 
> Maybe just computeVisibleRectUsingPaintOffset()

Done.

>> Source/WebCore/rendering/RenderBox.cpp:2259
>> +        bool isEmpty = !containerBox.applyCachedClipAndScrollPosition(adjustedRect, container, context.m_applyCompositedClips, context.m_applyCompositedContainerScrolls, context.m_useEdgeInclusiveIntersection);
> 
> Maybe just pass VisibleRectContext down

Done.

>> Source/WebCore/rendering/RenderBox.h:574
>> +    bool applyCachedClipAndScrollPosition(LayoutRect&, const RenderLayerModelObject* container, bool applyCompositedClips, bool applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const;
> 
> A comment should say what the return value means.

Done.

>> Source/WebCore/rendering/RenderInline.cpp:849
>> +        containingBlock->applyCachedClipAndScrollPosition(repaintRect, repaintContainer, false /* applyCompositedClips */, shouldApplyCompositedContainerScrollsForRepaint(), false /* useEdgeInclusiveIntersection */);
> 
> Ick!

Fixed now that the VisibleRectContext gets passed here.

>> Source/WebCore/rendering/RenderInline.cpp:874
>> +LayoutRect RenderInline::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect& rect) const
> 
> Needs same rename.

Done.

>> Source/WebCore/rendering/RenderObject.h:699
>> +        bool m_applyCompositedContainerScrolls;
> 
> I think this could just be an enum and an OptionSet<>

Done.

[WebKit Commit Bot]

Comment on attachment 352140 [details]
Patch

Clearing flags on attachment: 352140

Committed r237255: <https://trac.webkit.org/changeset/237255>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This broke text selection in form fields: bug 190899