Bug 189833

Summary:

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

Product:

WebKit

Reporter:

Ali Juma <ajuma>

Component:

Layout and Rendering

Assignee:

Ali Juma <ajuma>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, ews-watchlist, rniwa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

189624, 190899, 190905

Attachments:

Description	Flags
WIP for EWS	none
Archive of layout-test-results from ews102 for mac-sierra	none
Archive of layout-test-results from ews107 for mac-sierra-wk2	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Archive of layout-test-results from ews115 for mac-sierra	none
Archive of layout-test-results from ews200 for win-future	none
WIP for EWS	none
Archive of layout-test-results from ews125 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none

Ali Juma

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

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, EWS Watchlist	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, EWS Watchlist	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, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews115 for mac-sierra (3.34 MB, application/zip) 2018-09-21 11:51 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews200 for win-future (12.90 MB, application/zip) 2018-09-21 12:49 PDT, EWS Watchlist	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, EWS Watchlist	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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Ali Juma

Comment 1 2018-09-21 09:00:54 PDT

Created attachment 350370 [details] WIP for EWS

EWS Watchlist

Comment 2 2018-09-21 10:16:27 PDT Comment hidden (obsolete)

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

Comment 3 2018-09-21 10:16:30 PDT Comment hidden (obsolete)

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 4 2018-09-21 10:34:55 PDT Comment hidden (obsolete)

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

Comment 5 2018-09-21 10:34:56 PDT Comment hidden (obsolete)

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 6 2018-09-21 11:11:28 PDT Comment hidden (obsolete)

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

Comment 7 2018-09-21 11:11:30 PDT Comment hidden (obsolete)

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 8 2018-09-21 11:51:08 PDT Comment hidden (obsolete)

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

Comment 9 2018-09-21 11:51:10 PDT Comment hidden (obsolete)

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 10 2018-09-21 12:49:37 PDT Comment hidden (obsolete)

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

Comment 11 2018-09-21 12:49:48 PDT Comment hidden (obsolete)

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

Comment 12 2018-09-25 17:03:10 PDT

Created attachment 350814 [details] WIP for EWS

EWS Watchlist

Comment 13 2018-09-25 19:26:48 PDT Comment hidden (obsolete)

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

Comment 14 2018-09-25 19:26:50 PDT Comment hidden (obsolete)

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

Comment 15 2018-09-26 09:05:56 PDT

Created attachment 350864 [details] Patch

Ali Juma

Comment 16 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.

Radar WebKit Bug Importer

Comment 17 2018-10-01 11:55:09 PDT

<rdar://problem/44916139>

Ali Juma

Comment 18 2018-10-02 10:12:44 PDT

Created attachment 351409 [details] Patch Rebased

Simon Fraser (smfr)

Comment 19 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<>

Ali Juma

Comment 20 2018-10-11 23:02:17 PDT

Created attachment 352140 [details] Patch

Ali Juma

Comment 21 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.

WebKit Commit Bot

Comment 22 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>

WebKit Commit Bot

Comment 23 2018-10-18 06:51:04 PDT

All reviewed patches have been landed. Closing bug.

Simon Fraser (smfr)

Comment 24 2018-10-25 02:26:07 PDT

This broke text selection in form fields: bug 190899

Note You need to log in before you can comment on or make changes to this bug.