Bug 242079 - REGRESSION (r295701): top border is not visible
Summary: REGRESSION (r295701): top border is not visible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sammy Gill
URL:
Keywords: InRadar
Depends on: 206161
Blocks:
  Show dependency treegraph
 
Reported: 2022-06-28 12:20 PDT by Yury Semikhatsky
Modified: 2022-06-30 13:31 PDT (History)
7 users (show)

See Also:


Attachments
example page (1.39 KB, text/html)
2022-06-28 12:20 PDT, Yury Semikhatsky
no flags Details
before - border visible (7.52 KB, image/png)
2022-06-28 12:20 PDT, Yury Semikhatsky
no flags Details
after - border is not in the viewport (6.11 KB, image/png)
2022-06-28 12:21 PDT, Yury Semikhatsky
no flags Details
test page (6.51 KB, application/zip)
2022-06-29 18:02 PDT, Yury Semikhatsky
no flags Details
macos (106.90 KB, image/png)
2022-06-30 08:37 PDT, Yury Semikhatsky
no flags Details
gtk (73.26 KB, image/png)
2022-06-30 08:39 PDT, Yury Semikhatsky
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2022-06-28 12:20:10 PDT
Created attachment 460521 [details]
example page

After https://github.com/WebKit/WebKit/commit/e7abdadb1b69209d90e51b46caab32d4cc6bb093 the top border of the image grid is outside of the viewport. I can reproduce it on all platforms.
Comment 1 Yury Semikhatsky 2022-06-28 12:20:45 PDT
Created attachment 460522 [details]
before - border visible
Comment 2 Yury Semikhatsky 2022-06-28 12:21:09 PDT
Created attachment 460523 [details]
after - border is not in the viewport
Comment 3 Radar WebKit Bug Importer 2022-06-28 12:51:40 PDT
<rdar://problem/96083869>
Comment 4 Yury Semikhatsky 2022-06-29 09:45:47 PDT
This looks like a quite serious regression, should the patch be reverted for now?
Comment 5 zalan 2022-06-29 09:47:57 PDT
(In reply to Yury Semikhatsky from comment #4)
> This looks like a quite serious regression, should the patch be reverted for
> now?
Yes, I believe so.
Comment 6 Sammy Gill 2022-06-29 10:53:22 PDT
The patch has been reverted and I will work on the original bug report again. Thanks for the report Yury!
Comment 7 Sammy Gill 2022-06-29 16:45:46 PDT
Hi Yury,

We have been trying to reproduce this issue but have been unsuccessful so far. Would you be able to provide us with additional information on what you did to produce the issue? Any additional information on the steps you took along with information regarding the environment in which the tests ran (e.g. if you had 1x display) would be helpful in tracking down the problem.

Since the code changes were related to rendering replaced elements, would you be able to provide us with some details on the images that were used? It would be nice to try to recreate our test case as close as possible to the one you ran.
Comment 8 Yury Semikhatsky 2022-06-29 18:02:05 PDT
(In reply to Sammy Gill from comment #7)
> Hi Yury,
> 
> We have been trying to reproduce this issue but have been unsuccessful so
> far. Would you be able to provide us with additional information on what you
> did to produce the issue? Any additional information on the steps you took
> along with information regarding the environment in which the tests ran
> (e.g. if you had 1x display) would be helpful in tracking down the problem.
> 

I reproduced it in playwright builds on mac (with dpr=2) and on linux (WPE) with dpr=1.   Let me try to reproduce it with stock MiniBrowser.


> Since the code changes were related to rendering replaced elements, would
> you be able to provide us with some details on the images that were used? It
> would be nice to try to recreate our test case as close as possible to the
> one you ran.

Sorry about that, attaching .zip file with all the images.
Comment 9 Yury Semikhatsky 2022-06-29 18:02:26 PDT
Created attachment 460564 [details]
test page
Comment 10 Yury Semikhatsky 2022-06-30 08:37:03 PDT
(In reply to Yury Semikhatsky from comment #8)
> (In reply to Sammy Gill from comment #7)
> > Hi Yury,
> > 
> > We have been trying to reproduce this issue but have been unsuccessful so
> > far. Would you be able to provide us with additional information on what you
> > did to produce the issue? Any additional information on the steps you took
> > along with information regarding the environment in which the tests ran
> > (e.g. if you had 1x display) would be helpful in tracking down the problem.
> > 
> 
> I reproduced it in playwright builds on mac (with dpr=2) and on linux (WPE)
> with dpr=1.   Let me try to reproduce it with stock MiniBrowser.
> 

I can reproduce it with MiniBrowser on macOS (default resolution on m1 macbook pro) and Linux GTK. I can't reproduce it with ./Tools/Scripts/run-safari though.
Comment 11 Yury Semikhatsky 2022-06-30 08:37:53 PDT
Created attachment 460582 [details]
macos
Comment 12 Yury Semikhatsky 2022-06-30 08:39:53 PDT
Created attachment 460583 [details]
gtk
Comment 13 Brent Fulgham 2022-06-30 10:12:35 PDT
Does anyone know if there is a WPT that covers this behavior? It's surprising that the blamed change passes all tests, but exposes this problem.

Clearly we have a lack of test coverage here -- it would be good to identify a suitable test and make sure we start tracking it.
Comment 14 zalan 2022-06-30 10:23:24 PDT
This change must have exposed some other (non-layout) bug in Webkit. Setting margin on the body reveals the top border, while it should make no difference -when it comes to laying out/painting a replaced _border_ box. Quite a mystery.
Comment 15 Simon Fraser (smfr) 2022-06-30 10:27:05 PDT
If the top border is more than 1px tall does it hide the entire border, or just a 1px sliver?
Comment 16 zalan 2022-06-30 10:52:48 PDT
(In reply to Simon Fraser (smfr) from comment #15)
> If the top border is more than 1px tall does it hide the entire border, or
> just a 1px sliver?
border-width: 1.5px produces a hairline border on 2x (so always hides 1px).
Comment 17 Sammy Gill 2022-06-30 13:31:31 PDT
Since I can reproduce the bug now and it seems like I have enough information to move forward with this, I am going to resolve this bug report once again. The patch has already been reverted so I will continue on the original bug (206161).

I agree that we should have a test case for this so I will try to add one once I can find the underlying bug.

Sorry for reopening I believe this should be the last time.