Bug 222581 - [css-flexbox] CDC COVID Vaccine Tracker: Safari garbles data table
Summary: [css-flexbox] CDC COVID Vaccine Tracker: Safari garbles data table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL: https://covid.cdc.gov/covid-data-trac...
Keywords: InRadar
Depends on: 223438
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-01 16:26 PST by zalan
Modified: 2021-04-13 02:14 PDT (History)
20 users (show)

See Also:


Attachments
Broken site (webarchive) (1.89 MB, application/zip)
2021-03-02 09:47 PST, Simon Fraser (smfr)
no flags Details
Reduced test case (6.02 KB, text/html)
2021-03-02 16:16 PST, Brent Fulgham
no flags Details
Super-reduced test case (475 bytes, text/html)
2021-03-04 03:05 PST, Sergio Villar Senin
no flags Details
Patch (12.18 KB, patch)
2021-03-25 10:55 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2021-04-01 04:35 PDT, Sergio Villar Senin
zalan: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (14.08 KB, patch)
2021-04-12 04:01 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (12.52 KB, patch)
2021-04-12 04:42 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2021-03-01 16:26:04 PST
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>
Comment 1 Sergio Villar Senin 2021-03-01 23:25:25 PST
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.
Comment 2 Simon Fraser (smfr) 2021-03-02 09:46:15 PST
The live site was fixed. I'm not sure we captured the older, broken version.
Comment 3 Simon Fraser (smfr) 2021-03-02 09:47:00 PST
Created attachment 421953 [details]
Broken site (webarchive)
Comment 4 Simon Fraser (smfr) 2021-03-02 09:47:18 PST
Old site attached as a web archive (sorry, you'll need a Mac to open this).
Comment 5 Sergio Villar Senin 2021-03-02 10:48:08 PST
(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.
Comment 6 Brent Fulgham 2021-03-02 16:16:48 PST
Created attachment 422018 [details]
Reduced test case
Comment 7 Sergio Villar Senin 2021-03-04 03:05:38 PST
Created attachment 422211 [details]
Super-reduced test case
Comment 8 Sergio Villar Senin 2021-03-04 09:59:44 PST
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.
Comment 9 Sergio Villar Senin 2021-03-15 11:45:24 PDT
Adding the test case to WPT https://github.com/web-platform-tests/wpt/pull/28084
Comment 10 Sergio Villar Senin 2021-03-25 10:55:18 PDT
Created attachment 424259 [details]
Patch
Comment 11 Sergio Villar Senin 2021-03-31 02:47:10 PDT
Ping reviewers
Comment 12 Brent Fulgham 2021-03-31 16:50:18 PDT
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'?
Comment 13 Sergio Villar Senin 2021-04-01 00:56:35 PDT
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.
Comment 14 Sergio Villar Senin 2021-04-01 04:35:54 PDT
Created attachment 424885 [details]
Patch

Also replaced bool by enum in the parameter type to make the API saner
Comment 15 Sergio Villar Senin 2021-04-06 01:54:25 PDT
Ping reviewers.
Comment 16 EWS 2021-04-10 16:33:08 PDT
Tools/Scripts/svn-apply failed to apply attachment 424885 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 17 Sergio Villar Senin 2021-04-12 04:01:33 PDT
Created attachment 425728 [details]
Patch for landing
Comment 18 Sergio Villar Senin 2021-04-12 04:42:11 PDT
Created attachment 425731 [details]
Patch for landing
Comment 19 Sergio Villar Senin 2021-04-13 02:14:33 PDT
Committed r275873 (236438@main): <https://commits.webkit.org/236438@main>