Bug 189833 - [IntersectionObserver] Factor out rect mapping and clipping logic from computeRectForRepaint
Summary: [IntersectionObserver] Factor out rect mapping and clipping logic from comput...
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:
Blocks: 189624 190899 190905
  Show dependency treegraph
 
Reported: 2018-09-21 08:40 PDT by Ali Juma
Modified: 2018-10-25 11:30 PDT (History)
7 users (show)

See Also:


Attachments
WIP for EWS (58.65 KB, patch)
2018-09-21 09:00 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.54 MB, application/zip)
2018-09-21 10:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.29 MB, application/zip)
2018-09-21 10:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.49 MB, application/zip)
2018-09-21 11:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.34 MB, application/zip)
2018-09-21 11:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (12.90 MB, application/zip)
2018-09-21 12:49 PDT, Build Bot
no flags Details
WIP for EWS (58.69 KB, patch)
2018-09-25 17:03 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (24.08 MB, application/zip)
2018-09-25 19:26 PDT, Build Bot
no flags Details
Patch (58.69 KB, patch)
2018-09-26 09:05 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (58.64 KB, patch)
2018-10-02 10:12 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (59.01 KB, patch)
2018-10-11 23:02 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-21 08:40:56 PDT
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.
Comment 1 Ali Juma 2018-09-21 09:00:54 PDT
Created attachment 350370 [details]
WIP for EWS
Comment 2 Build Bot 2018-09-21 10:16:27 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2018-09-21 10:16:30 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2018-09-21 10:34:55 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2018-09-21 10:34:56 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2018-09-21 11:11:28 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2018-09-21 11:11:30 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2018-09-21 11:51:08 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2018-09-21 11:51:10 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2018-09-21 12:49:37 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2018-09-21 12:49:48 PDT Comment hidden (obsolete)
Comment 12 Ali Juma 2018-09-25 17:03:10 PDT
Created attachment 350814 [details]
WIP for EWS
Comment 13 Build Bot 2018-09-25 19:26:48 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2018-09-25 19:26:50 PDT Comment hidden (obsolete)
Comment 15 Ali Juma 2018-09-26 09:05:56 PDT
Created attachment 350864 [details]
Patch
Comment 16 Ali Juma 2018-09-26 09:09:49 PDT
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.
Comment 17 Radar WebKit Bug Importer 2018-10-01 11:55:09 PDT
<rdar://problem/44916139>
Comment 18 Ali Juma 2018-10-02 10:12:44 PDT
Created attachment 351409 [details]
Patch

Rebased
Comment 19 Simon Fraser (smfr) 2018-10-11 16:19:19 PDT
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<>
Comment 20 Ali Juma 2018-10-11 23:02:17 PDT
Created attachment 352140 [details]
Patch
Comment 21 Ali Juma 2018-10-11 23:04:19 PDT
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.
Comment 22 WebKit Commit Bot 2018-10-18 06:51:02 PDT
Comment on attachment 352140 [details]
Patch

Clearing flags on attachment: 352140

Committed r237255: <https://trac.webkit.org/changeset/237255>
Comment 23 WebKit Commit Bot 2018-10-18 06:51:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Simon Fraser (smfr) 2018-10-25 02:26:07 PDT
This broke text selection in form fields: bug 190899