Bug 183040 – Separate paint and scroll offsets for RenderLayerBacking::m_scrollingContentsLayer

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Source/WebCore/rendering/RenderLayerBacking.cpp
// FIXME: The paint offset and the scroll offset should really be separate concepts.

[Frédéric Wang (:fredw)]

Created attachment 334454 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 334454 [details]
Patch

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

@Simon, Daniel: This patch just extracts the scrollOffset from offsetFromRender, but I'm not sure it is what was meant in the FIXME nor whether it's really an improvement... WDYT?

> Source/WebCore/rendering/RenderLayerBacking.cpp:2471
>          // but the repaint rect is computed without taking the scroll position into account (see shouldApplyClipAndScrollPositionForRepaint()).

I'm a bit confused by this comment. The first line (from Daniel, bug 125239) seems to say it is done because scrollOffset is included in the offsetFromRenderer in RenderLayerBacking::updateGeometry(), which actually happens on non-iOS platforms too and what I'm trying to remove here. The second line (from Simon, bug 159186) seems to say it is done because scrollPosition in applyCachedClipAndScrollPositionForRepaint (rather than shouldApplyClipAndScrollPositionForRepaint). So it does not seem we can get rid of this...

[Frédéric Wang (:fredw)]

Created attachment 340071 [details]
Patch

Rebasing

[Frédéric Wang (:fredw)]

Created attachment 348912 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 354929 [details]
Patch

Rebasing...

[EWS Watchlist]

Comment on attachment 354929 [details]
Patch

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

New failing tests:
compositing/overflow/scrolling-without-painting.html
compositing/overflow/textarea-scroll-touch.html

[EWS Watchlist]

Created attachment 354943 [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 354929 [details]
Patch

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

New failing tests:
compositing/overflow/textarea-scroll-touch.html
compositing/overflow/scrolling-without-painting.html

[EWS Watchlist]

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

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

[Frédéric Wang (:fredw)]

Created attachment 355336 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 355474 [details]
Patch

[Frédéric Wang (:fredw)]

@smfr: review ping?

[Simon Fraser (smfr)]

Comment on attachment 355474 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests, already covered by existing tests.

Can you explain here why the scroll offset needs to be store separately from the paint offset?

[Frédéric Wang (:fredw)]

Comment on attachment 355474 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests, already covered by existing tests.
> 
> Can you explain here why the scroll offset needs to be store separately from the paint offset?

I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME?

[Frédéric Wang (:fredw)]

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests, already covered by existing tests.
> 
> Can you explain here why the scroll offset needs to be store separately from the paint offset?

I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME?

[Simon Fraser (smfr)]

Comment on attachment 355474 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        No new tests, already covered by existing tests.
>> 
>> Can you explain here why the scroll offset needs to be store separately from the paint offset?
> 
> I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME?

Oh ha, that was my fixme. I mean, it makes sense, but do we need to do it to avoid repaints on composited scroll, for example?

[Frédéric Wang (:fredw)]

Comment on attachment 355474 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:8
>>>> +        No new tests, already covered by existing tests.
>>> 
>>> Can you explain here why the scroll offset needs to be store separately from the paint offset?
>> 
>> I'm not exactly sure why it needs to be that way (seem comment 2) but I see it makes the API cleaner and maybe help to optimize re-display. Can you please explain the rationale of this FIXME?
> 
> Oh ha, that was my fixme. I mean, it makes sense, but do we need to do it to avoid repaints on composited scroll, for example?

That was what I mean by optimizing redisplay but I'm not sure at all to be honest... 

As a reminder, this originates from the discussion about iOS frame scrolling on 12/02/2018: 

23:21:32 - fredw: I also tried to copy the stuff with offsetFromRenderer, but again, I'm not sure about that
23:21:34 - smfr: right, that's to match how UIScrollVIew works
23:21:41 - smfr: don't use offsetFromRenderer! it's special
23:21:55 - smfr: it's to size layers to take account of borders, shadows etc
23:22:18 - fredw: mmh, but I think it was used for more...
23:23:00 - smfr: maybe you could draw some ASCII art in a bug to show how the web process layer tree maps into UIScrollVIews for a subframe, across the frame boundary
23:23:11 - smfr: it's critical to get that part right
23:24:44 - fredw: This is only taking padding for m_scrollingLayer: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L1140
23:25:13 - fredw: but this is taking offset into account for the content layer: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L1163
23:25:42 - smfr: "FIXME: The paint offset and the scroll offset should really be separate concepts." yes
23:26:44 - fredw: So for now I just tried to copy the logic of overflow node, but maybe that was not a good idea...
23:27:04 - smfr: i think it will be different in some ways
23:27:54 - fredw: I think I just ignored the padding stuff in this code, since it is taken into account somewhere else
23:28:20 - smfr: be sure to test content with iframes which have padding, thick borders and box shadows
23:28:40 - fredw: Yes, I already did did that in my patch. Not box shadows, though
23:30:08 - fredw: So do you think I should keep "setBoundsOrigin" like for overflow node or try to use "setPosition" instead?
23:30:29 - smfr: you'll have to use bounds origin since that's what UIScrollView uses
23:30:44 - smfr: maybe we should try to get that distinction out of the WebCore codeethough
23:30:53 - smfr: would be nice to hide that difference in the scrolling tree for WK2
23:31:01 - smfr: harder with iOS WK1
23:31:46 - fredw: and what about these offsetFromRenderer's?
23:32:49 - fredw: IIUC, I need it unless i do the FIXME you mentioned
23:32:52 - smfr: we should address the FIXME :)
23:33:09 - smfr: i'd have to study more to see what that involves
23:33:37 - fredw: OK, maybe I could do that for overflow node first too. Although, I don't really understand it :-)
23:34:10 - fredw: it seems it's  not specific to iOS, though, there is another one in the #ekse
23:34:18 - smfr: so, for normal layers, offsetFromRenderer is the offset between the top left of the GraphicsLayer (the backing store) to the top left of the renderer's border box
23:34:36 - smfr: basically if you add box-shadow or something, offsetFromRenderer accounts for the extra space you need

[Frédéric Wang (:fredw)]

Created attachment 356130 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 356130 [details]:

workers/bomb.html bug 171985 (author: [e-mail])
webgl/2.0.0/conformance/more/functions/vertexAttribPointerBadArgs.html bug 192218 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 356130 [details]
Patch for landing

Clearing flags on attachment: 356130

Committed r238727: <https://trac.webkit.org/changeset/238727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46365397>