Bug 227285

Summary: Pixel snapping is incorrect for async scrolled overflow layers
Product: WebKit Reporter: Chris Lord <clord>
Component: CompositingAssignee: Chris Lord <clord>
Status: NEW    
Severity: Normal CC: bfulgham, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, kondapallykalyan, pdr, rego, simon.fraser, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 224596    
Attachments:
Description Flags
Patch
none
Patch
clord: review?
Subpixel scroller test
none
Test case
none
Screenshot none

Chris Lord
Reported 2021-06-23 02:34:05 PDT
I'm not sure if this is a side-effect of fractional coordinates, a bug specifically in the WPE(/GTK) layer rendering or a bug in layout/rendering, but there appear to be 1-pixel differences in clipping in some situations, where the clip rectangle seems too tall. I expect this bug won't be WPE-specific or CSS grid-specific, but I'm labelling this bug as such until I have confirmation. This is causing imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001.html and at least the rl variant of the same test to fail on WPE (which runs these tests with async overflow scrolling enabled).
Attachments
Patch (6.14 KB, patch)
2021-06-30 02:46 PDT, Chris Lord 		no flags
DetailsFormatted DiffDiff
Patch (6.14 KB, patch)
2021-07-01 04:44 PDT, Chris Lord 		clord: review?
DetailsFormatted DiffDiff
Subpixel scroller test (1.09 KB, text/html)
2021-07-01 12:05 PDT, Simon Fraser (smfr) 		no flags
Details
Test case (1.16 KB, text/html)
2021-07-01 14:22 PDT, Simon Fraser (smfr) 		no flags
Details
Screenshot (115.21 KB, image/png)
2021-07-01 14:23 PDT, Simon Fraser (smfr) 		no flags
Details
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chris Lord
Comment 1 2021-06-23 07:41:09 PDT
I'm more convinced there's a legitimate bug here (maybe just a rounding error?), but I've not found it yet... If you force mock scrollbars and load this test in minibrowser with async overflow scrolling enabled, it's clear to me that behaviour isn't quite right with respect to positioning. I'm examining positions and sizes at the moment, but I've not found any odd fractional sizes that might be causing this yet.
Chris Lord
Comment 2 2021-06-29 04:18:28 PDT
I have a patch for this now and it is indeed a general problem (with an associated FIXME comment), but I'd like to get bug 227061 resolved first.
Chris Lord
Comment 3 2021-06-29 08:59:05 PDT
Small update, it looks like fixing this bug fixes all the remaining failures in bug 224596 :)
Simon Fraser (smfr)
Comment 4 2021-06-29 09:54:02 PDT
Can we do this one first?
Chris Lord
Comment 5 2021-06-29 09:56:06 PDT
(In reply to Simon Fraser (smfr) from comment #4) > Can we do this one first? We can, just it's two awkward rebases for me :) I'll do that tomorrow assuming there's no movement on bug 227061 though.
Radar WebKit Bug Importer
Comment 6 2021-06-30 02:35:20 PDT
<rdar://problem/79957479>
Chris Lord
Comment 7 2021-06-30 02:46:43 PDT
Created attachment 432579 [details] Patch
Chris Lord
Comment 8 2021-06-30 02:47:54 PDT
(In reply to Chris Lord from comment #3) > Small update, it looks like fixing this bug fixes all the remaining failures > in bug 224596 :) Whoops, so when I ran tests, I was using the wrong platform - this only fixes 2 of those failures, not all of them. On the plus side, I was also wrong about the rebases being awkward, they were actually trivial.
Chris Lord
Comment 9 2021-06-30 03:45:37 PDT
Comment on attachment 432579 [details] Patch Not quite correct yet.
Chris Lord
Comment 10 2021-07-01 04:44:54 PDT
Created attachment 432679 [details] Patch
Simon Fraser (smfr)
Comment 11 2021-07-01 12:05:31 PDT
Comment on attachment 432679 [details] Patch With async overflow scrolling disabled, we render scrollbars in the wrong place too (I think because of integral snapping of Widget?) so I'm not sure how we expect scrollbars to render correctly.
Simon Fraser (smfr)
Comment 12 2021-07-01 12:05:51 PDT
Created attachment 432715 [details] Subpixel scroller test
Simon Fraser (smfr)
Comment 13 2021-07-01 14:22:44 PDT
Created attachment 432725 [details] Test case
Simon Fraser (smfr)
Comment 14 2021-07-01 14:23:00 PDT
Tested with the patch, things don't seem right. See test and screenshot.
Simon Fraser (smfr)
Comment 15 2021-07-01 14:23:16 PDT
Created attachment 432726 [details] Screenshot
Chris Lord
Comment 16 2021-07-06 02:27:51 PDT
(In reply to Simon Fraser (smfr) from comment #14) > Tested with the patch, things don't seem right. See test and screenshot. This displays correctly for me, but I'm guessing this is affected by a hidpi screen - thanks for this, I'll figure something out.
Chris Lord
Comment 17 2021-07-06 04:48:04 PDT
(In reply to Simon Fraser (smfr) from comment #14) > Tested with the patch, things don't seem right. See test and screenshot. Using a device scale factor of 1.5 I reproduce this issue, but it seems to happen prior to this patch too. Is the issue that you expect this patch to also fix this issue?
Chris Lord
Comment 18 2021-07-06 04:49:04 PDT
(In reply to Chris Lord from comment #17) > (In reply to Simon Fraser (smfr) from comment #14) > > Tested with the patch, things don't seem right. See test and screenshot. > > Using a device scale factor of 1.5 I reproduce this issue, but it seems to > happen prior to this patch too. Is the issue that you expect this patch to > also fix this issue? (just an extra note; this patch only touches scrolled contents rendering, it doesn't touch scrollbar rendering - so this behaviour is expected to me)
Note You need to log in before you can comment on or make changes to this bug.