Bug 241104 – Spacing after some posts is too large on Dead by Daylight forums

• Status: REOPENED
• Component: Layout and Rendering

[Jason Hansen]

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.

[Alexey Proskuryakov]

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.

[alan]

Created attachment 459892 [details]
Test reduction

[alan]

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.

[alan]

(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).

[alan]

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.

[alan]

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.

[alan]

Created attachment 459895 [details]
Screenshots

[alan]

<rdar://88110302>

[alan]

Created attachment 459918 [details]
Patch

[Antti Koivisto]

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?

[Antti Koivisto]

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!

[Antti Koivisto]

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

[Antti Koivisto]

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

[alan]

Created attachment 459919 [details]
Patch

[alan]

(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!

[alan]

(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.

[alan]

(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.

[alan]

Created attachment 459928 [details]
Patch

[EWS]

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].

[Jason Hansen]

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.

[alan]

(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.

[Jason Hansen]

(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.

[Jason Hansen]

I can confirm that Safari Tech Preview 148 fixed this issue.

[Jason Hansen]

(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. :/

[Brent Fulgham]

(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!

[alan]

(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.

[Jason Hansen]

This bug back in the latest Tech Preview 153 and Safari 16.0.  Why?

https://forum.deadbydaylight.com/en/discussion/339058/i-watched-the-entire-behaviour-beyond-2022-stream-so-you-dont-have-to?__cf_chl_tk=0ibaSiaoPJJdRBm3Jt4MGPKothnFKBCpWj4UhnDKLEQ-1663026583-0-gaNycGzNCBE