Bug 227285 - Pixel snapping is incorrect for async scrolled overflow layers
Summary: Pixel snapping is incorrect for async scrolled overflow layers
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks: 224596
  Show dependency treegraph
 
Reported: 2021-06-23 02:34 PDT by Chris Lord
Modified: 2021-07-06 04:49 PDT (History)
15 users (show)

See Also:


Attachments
Patch (6.14 KB, patch)
2021-06-30 02:46 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (6.14 KB, patch)
2021-07-01 04:44 PDT, Chris Lord
clord: review?
Details | Formatted Diff | Diff
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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 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).
Comment 1 Chris Lord 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.
Comment 2 Chris Lord 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.
Comment 3 Chris Lord 2021-06-29 08:59:05 PDT
Small update, it looks like fixing this bug fixes all the remaining failures in bug 224596 :)
Comment 4 Simon Fraser (smfr) 2021-06-29 09:54:02 PDT
Can we do this one first?
Comment 5 Chris Lord 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.
Comment 6 Radar WebKit Bug Importer 2021-06-30 02:35:20 PDT
<rdar://problem/79957479>
Comment 7 Chris Lord 2021-06-30 02:46:43 PDT
Created attachment 432579 [details]
Patch
Comment 8 Chris Lord 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.
Comment 9 Chris Lord 2021-06-30 03:45:37 PDT
Comment on attachment 432579 [details]
Patch

Not quite correct yet.
Comment 10 Chris Lord 2021-07-01 04:44:54 PDT
Created attachment 432679 [details]
Patch
Comment 11 Simon Fraser (smfr) 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.
Comment 12 Simon Fraser (smfr) 2021-07-01 12:05:51 PDT
Created attachment 432715 [details]
Subpixel scroller test
Comment 13 Simon Fraser (smfr) 2021-07-01 14:22:44 PDT
Created attachment 432725 [details]
Test case
Comment 14 Simon Fraser (smfr) 2021-07-01 14:23:00 PDT
Tested with the patch, things don't seem right. See test and screenshot.
Comment 15 Simon Fraser (smfr) 2021-07-01 14:23:16 PDT
Created attachment 432726 [details]
Screenshot
Comment 16 Chris Lord 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.
Comment 17 Chris Lord 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?
Comment 18 Chris Lord 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)