RESOLVED FIXED 214315
[css-grid] CSSOM gridTemplateColumns/Rows reports wrong line names when leading implicit tracks are present
https://bugs.webkit.org/show_bug.cgi?id=214315
Summary [css-grid] CSSOM gridTemplateColumns/Rows reports wrong line names when leadi...
Oriol Brufau
Reported 2020-07-14 13:08:12 PDT
These WPT are failing: imported/w3c/web-platform-tests/css/css-grid/parsing/grid-template-columns-computed-implicit-track.html imported/w3c/web-platform-tests/css/css-grid/parsing/grid-template-rows-computed-implicit-track.html They were added in https://crbug.com/790988, the Chromium patch needs to be ported to WebKit.
Attachments
Patch (30.93 KB, patch)
2020-07-14 13:31 PDT, Oriol Brufau
no flags
Patch (20.63 KB, patch)
2020-07-15 05:27 PDT, Oriol Brufau
no flags
Patch (20.57 KB, patch)
2020-07-15 14:17 PDT, Oriol Brufau
no flags
Patch (20.62 KB, patch)
2020-07-16 05:33 PDT, Oriol Brufau
no flags
Patch (20.64 KB, patch)
2020-07-16 09:33 PDT, Oriol Brufau
no flags
Oriol Brufau
Comment 1 2020-07-14 13:31:17 PDT
Manuel Rego Casasnovas
Comment 2 2020-07-15 01:07:53 PDT
Comment on attachment 404272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404272&action=review On a quick look, it seems good. But I'd split it in 2 patches (one for the refactoring and another for the actual fix). Thanks! > Source/WebCore/ChangeLog:20 > + This patch also renames SmallestTrackStart to ExplicitGridStart. > + The SmallestTrackStart method used to return a signed int which was > + never positive, representing the smallest untranslated start among all > + grid items, clamped by 0 as a maximum. Can we do this change on a different patch? This is basically a refactoring and should have no behavior changes, so we can land that one first, and then the actual fix for this bug. WDYT?
Oriol Brufau
Comment 3 2020-07-15 05:09:21 PDT
(In reply to Manuel Rego Casasnovas from comment #2) > This is basically a refactoring and should have no behavior changes, so we > can land that one first, and then the actual fix for this bug. WDYT? OK, filed bug 214347.
Oriol Brufau
Comment 4 2020-07-15 05:27:53 PDT
Manuel Rego Casasnovas
Comment 5 2020-07-15 13:56:28 PDT
Comment on attachment 404335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404335&action=review Thanks for the fix. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:875 > + ASSERT((unsigned)end <= tracks.size()); Do we need this cast? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:920 > + // Named grid line indices are relative to the explicit grid, but we are > + // including all tracks. So we need to subtract the number of leading > + // implicit tracks in order to get the proper line index. Nit: Comments lines can be longer in WebKit, no need to wrap to 80-chars.
Oriol Brufau
Comment 6 2020-07-15 14:17:03 PDT
Manuel Rego Casasnovas
Comment 7 2020-07-16 00:49:31 PDT
Comment on attachment 404393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404393&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:875 > ASSERT(end <= tracks.size()); So it looks like the cast was actually needed: ./css/CSSComputedStyleDeclaration.cpp:875:16: error: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare] ASSERT(end <= tracks.size()); Probably you can do something like: ASSERT(end <= static_cast<int>(tracks.size()));
Oriol Brufau
Comment 8 2020-07-16 05:33:40 PDT
Oriol Brufau
Comment 9 2020-07-16 09:33:55 PDT
EWS
Comment 10 2020-07-16 09:59:23 PDT
Committed r264465: <https://trac.webkit.org/changeset/264465> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404451 [details].
Radar WebKit Bug Importer
Comment 11 2020-07-16 10:00:24 PDT
Note You need to log in before you can comment on or make changes to this bug.