Bug 241104 - Spacing after some posts is too large on Dead by Daylight forums
Summary: Spacing after some posts is too large on Dead by Daylight forums
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 15
Hardware: Mac (Apple Silicon) macOS 12
: P2 Normal
Assignee: zalan
URL:
Keywords: BrowserCompat, InRadar
Depends on:
Blocks:
 
Reported: 2022-05-30 11:04 PDT by Jason Hansen
Modified: 2023-10-29 07:19 PDT (History)
13 users (show)

See Also:


Attachments
spacing_issue (542.71 KB, image/png)
2022-05-30 11:04 PDT, Jason Hansen
no flags Details
Test reduction (308 bytes, text/html)
2022-05-31 11:16 PDT, zalan
no flags Details
Screenshots (1.49 MB, image/png)
2022-05-31 12:34 PDT, zalan
no flags Details
Patch (5.88 KB, patch)
2022-05-31 21:04 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (6.71 KB, patch)
2022-05-31 21:27 PDT, zalan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (7.04 KB, patch)
2022-06-01 07:54 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Hansen 2022-05-30 11:04:40 PDT
Created attachment 459867 [details]
spacing_issue

When viewing Dead by Daylight forums, the spacing after some posts are very large and need to scroll down a whole screen to see the next post.  This does not occur within latest versions of Chrome or Firefox.  An example can be found here - about half way down the page;

https://forum.deadbydaylight.com/en/discussion/325745/do-you-miss-anything-from-old-dbd#latest

I've also included a screenshot.
Comment 1 Alexey Proskuryakov 2022-05-30 19:02:27 PDT
A lot of the time, such issues are caused by websites having browser specific code that is supposed to work around WebKit bugs, but instead breaks the websites.

I tried to check if this is the case here by changing user agent string via Developer menu, but this forum software blocks UA spoofing, so the page doesn't open when using Chrome or Firefox UA.
Comment 2 zalan 2022-05-31 11:16:33 PDT
Created attachment 459892 [details]
Test reduction
Comment 3 zalan 2022-05-31 11:36:22 PDT
Normally "height: -webkit-fill-available" stretches the block box to the height of the containing block (in a simple one child only case). Here the containing block's height in not definite meaning that it depends on the descendant content, while the child box says "stretch me all the way" (a bit of a circular dependency). It looks like we pick the viewport height as the base for such stretching to resolve this circular dependency.
Comment 4 zalan 2022-05-31 11:40:53 PDT
(In reply to zalan from comment #3)
> Normally "height: -webkit-fill-available" stretches the block box to the
> height of the containing block (in a simple one child only case). Here the
> containing block's height in not definite meaning that it depends on the
> descendant content, while the child box says "stretch me all the way" (a bit
> of a circular dependency). It looks like we pick the viewport height as the
> base for such stretching to resolve this circular dependency.
The gap changes as the browser window gets taller/shorter (after reload).
Comment 5 zalan 2022-05-31 12:08:55 PDT
so yeah, apparently RenderBox::computeIntrinsicLogicalContentHeightUsing (with fill-available) walks the containing block tree all the way up to the first block container with definite height. In many cases that's the ICB -> viewport size.
Comment 6 zalan 2022-05-31 12:30:38 PDT
Checking if available height is resolvable within the containing block fixes this issue, but I need to read this code a bit more to understand why we let this function climb across containing blocks.
Comment 7 zalan 2022-05-31 12:34:17 PDT
Created attachment 459895 [details]
Screenshots
Comment 8 zalan 2022-05-31 21:04:10 PDT
<rdar://88110302>
Comment 9 zalan 2022-05-31 21:04:44 PDT
Created attachment 459918 [details]
Patch
Comment 10 Antti Koivisto 2022-05-31 21:09:12 PDT
Comment on attachment 459918 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3262
> +// FIXME: evaluate whether this should be a method of RenderObject instead.

why does that need to be evaluated?
Comment 11 Antti Koivisto 2022-05-31 21:11:02 PDT
Comment on attachment 459918 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3282
> +            // Until then, this is mostly just guesswork.

Sounds solid!
Comment 12 Antti Koivisto 2022-05-31 21:14:18 PDT
Comment on attachment 459918 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3262
>> +// FIXME: evaluate whether this should be a method of RenderObject instead.
> 
> why does that need to be evaluated?

Also this can probably take RenderBox
Comment 13 Antti Koivisto 2022-05-31 21:14:19 PDT
Comment on attachment 459918 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3262
>> +// FIXME: evaluate whether this should be a method of RenderObject instead.
> 
> why does that need to be evaluated?

Also this can probably take RenderBox
Comment 14 zalan 2022-05-31 21:27:22 PDT
Created attachment 459919 [details]
Patch
Comment 15 zalan 2022-05-31 21:27:59 PDT
(In reply to Antti Koivisto from comment #10)
> Comment on attachment 459918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459918&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:3262
> > +// FIXME: evaluate whether this should be a method of RenderObject instead.
> 
> why does that need to be evaluated?
Done! Evaluated!
Comment 16 zalan 2022-05-31 21:28:19 PDT
(In reply to Antti Koivisto from comment #13)
> Comment on attachment 459918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459918&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:3262
> >> +// FIXME: evaluate whether this should be a method of RenderObject instead.
> > 
> > why does that need to be evaluated?
> 
> Also this can probably take RenderBox
Good point! Done.
Comment 17 zalan 2022-05-31 21:36:27 PDT
(In reply to Antti Koivisto from comment #11)
> Comment on attachment 459918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459918&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:3282
> > +            // Until then, this is mostly just guesswork.
> 
> Sounds solid!
Yeah, so the proper way to resolve this (and this is exactly what LFC does) is instead of climbing the ancestor chain to resolve certain values (e.g. height), we should pass in a constraint value (e.g. containing block's height if available).
It would make this code look like:
if (height.isFillAvailable() && availableSpace)
  return availableSpace;
return { };
vs.
trying to figure out all the conditions where the containing block may have a valid value to resolve against.
Comment 18 zalan 2022-06-01 07:54:56 PDT
Created attachment 459928 [details]
Patch
Comment 19 EWS 2022-06-01 10:26:23 PDT
Committed r295094 (251189@main): <https://commits.webkit.org/251189@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459928 [details].
Comment 20 Jason Hansen 2022-06-22 09:39:10 PDT
There isn't a way to know when or what release this will appear in for me to test, right? 147 came out today, but no mention of this bug.
Comment 21 zalan 2022-06-22 10:29:18 PDT
(In reply to Jason Hansen from comment #20)
> There isn't a way to know when or what release this will appear in for me to
> test, right? 147 came out today, but no mention of this bug.
You are right, 147 does not have this fix yet :/ I'd come back and check when 148 is out. Sorry about that.
Comment 22 Jason Hansen 2022-06-22 10:31:16 PDT
(In reply to zalan from comment #21)
> (In reply to Jason Hansen from comment #20)
> > There isn't a way to know when or what release this will appear in for me to
> > test, right? 147 came out today, but no mention of this bug.
> You are right, 147 does not have this fix yet :/ I'd come back and check
> when 148 is out. Sorry about that.

No need to apologize and appreciate the response.

Thank you.
Comment 23 Jason Hansen 2022-07-01 12:41:29 PDT
I can confirm that Safari Tech Preview 148 fixed this issue.
Comment 24 Jason Hansen 2022-07-01 13:05:15 PDT
(In reply to Jason Hansen from comment #23)
> I can confirm that Safari Tech Preview 148 fixed this issue.

Even thought it wasn't mentioned in the release notes. :/
Comment 25 Brent Fulgham 2022-07-01 14:01:53 PDT
(In reply to Jason Hansen from comment #24)
> (In reply to Jason Hansen from comment #23)
> > I can confirm that Safari Tech Preview 148 fixed this issue.
> 
> Even thought it wasn't mentioned in the release notes. :/

We should have! Doh!
Comment 26 zalan 2022-07-01 15:42:48 PDT
(In reply to Brent Fulgham from comment #25)
> (In reply to Jason Hansen from comment #24)
> > (In reply to Jason Hansen from comment #23)
> > > I can confirm that Safari Tech Preview 148 fixed this issue.
> > 
> > Even thought it wasn't mentioned in the release notes. :/
> 
> We should have! Doh!
Yeah how did we miss this.