Bug 222581 – [css-flexbox] CDC COVID Vaccine Tracker: Safari garbles data table

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[zalan]

Visit https://covid.cdc.gov/covid-data-tracker/#vaccinations
In Safari, the data table is totally garbled and missing nearly all data (In Chrome, it renders as expected) 

<rdar://problem/74653849>

[Sergio Villar Senin]

Hmm which version are you using? Is it STP 121? That one had issues with flexbox due to a revert that went into the release.

It seems to work fine for me with the current trunk.

[Simon Fraser (smfr)]

The live site was fixed. I'm not sure we captured the older, broken version.

[Simon Fraser (smfr)]

Created attachment 421953 [details]
Broken site (webarchive)

[Simon Fraser (smfr)]

Old site attached as a web archive (sorry, you'll need a Mac to open this).

[Sergio Villar Senin]

(In reply to Simon Fraser (smfr) from comment #4)
> Old site attached as a web archive (sorry, you'll need a Mac to open this).

It'd be awesome if someone could provide a reduced test case.

[Brent Fulgham]

Created attachment 422018 [details]
Reduced test case

[Sergio Villar Senin]

Created attachment 422211 [details]
Super-reduced test case

[Sergio Villar Senin]

OK this is a well known issue in flexbox. It happens with nested flexboxes when the flex-basis is a percentage. The current code that stretches the item forces a relayout of the children because it thinks it has percentage height descendants. That happens because we call computePercentageLogicalHeights() with a mock percentage length to check whether a size is definite and that call performs the addPercentageHeightDescendants() call. We should avoid calling the latter in those cases as we're just trying to figure out whether we can compute the flex-basis used value or not.

Fixing this bug will likely improve a bit the performance in those cases as we'll be saving some layouts.

[Sergio Villar Senin]

Adding the test case to WPT https://github.com/web-platform-tests/wpt/pull/28084

[Sergio Villar Senin]

Created attachment 424259 [details]
Patch

[Sergio Villar Senin]

Ping reviewers

[Brent Fulgham]

Comment on attachment 424259 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Adding a new parametter to the aforementioned method so that the percentage height descendants map could be left untouched

parameter :-)

> Source/WebCore/rendering/RenderFlexibleBox.cpp:857
> +        bool definite = canComputePercentageFlexBasis(child, flexBasis, false);

We no longer do an early return here. Is that an important part of the fix? It seems like this bug fix is mostly about the change made to 'useChildOverridingMainSizeForPercentageResolution'?

[Sergio Villar Senin]

Comment on attachment 424259 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        Adding a new parametter to the aforementioned method so that the percentage height descendants map could be left untouched
> 
> parameter :-)

:)

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:857
>> +        bool definite = canComputePercentageFlexBasis(child, flexBasis, false);
> 
> We no longer do an early return here. Is that an important part of the fix? It seems like this bug fix is mostly about the change made to 'useChildOverridingMainSizeForPercentageResolution'?

That's right, the important part are the changes made to RenderBox so that it does not unconditionally add a child to the list of percentage height descendants just because we're calling a method to resolve a percentage.

We don't do early returns indeed but that isn't really important because we do them in canComputePercentageFlexBasis(). What matters here is whether the size is definite or not. That said I have just noticed that we're changing the behaviour slightly here. In the case of !isColumnFlow() we were early returning true without updating the m_hasDefiniteHeight value. However with the refactoring we'd be setting true. I have to change that.

[Sergio Villar Senin]

Created attachment 424885 [details]
Patch

Also replaced bool by enum in the parameter type to make the API saner

[Sergio Villar Senin]

Ping reviewers.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 424885 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Sergio Villar Senin]

Created attachment 425728 [details]
Patch for landing

[Sergio Villar Senin]

Created attachment 425731 [details]
Patch for landing

[Sergio Villar Senin]

Committed r275873 (236438@main): <https://commits.webkit.org/236438@main>