Bug 209461

Summary:

[css-grid] Grid layout unstable with percentage margin

Product:

WebKit

Reporter:

Carlos Alberto Lopez Perez <clopez>

Component:

CSS

Assignee:

Oriol Brufau <obrufau>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

changseok, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, kyle.bavender, obrufau, pdr, rego, simon.fraser, svillar, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.chromium.org/p/chromium/issues/detail?id=834643
https://bugs.chromium.org/p/chromium/issues/detail?id=942811
https://bugs.chromium.org/p/chromium/issues/detail?id=808758
https://bugs.webkit.org/show_bug.cgi?id=212983

Bug Depends on:

210089, 212991

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Description Carlos Alberto Lopez Perez 2020-03-23 20:37:18 PDT

What steps will reproduce the problem?
1. Visit test case: https://jsfiddle.net/xm7rn3wj/
2. Move the mouse pointer on and off the text ("Hover here.") several times.

What is the expected result?
The text should not continuously move towards the bottom right.


What happens instead of that?
Every time the mouse pointer moves on and off the text it moves further towards the bottom right.

Firefox does it right and Chrome does it since crbug.com/834643

Comment 1 Carlos Alberto Lopez Perez 2020-03-23 20:40:42 PDT

This is covered by WPT tests:
css/css-grid/grid-items/grid-items-percentage-margins-???.html
css/css-grid/grid-items/grid-items-percentage-paddings-???.html
css/css-grid/grid-items/grid-item-dynamic-min-contribution-001.html

Comment 2 Carlos Alberto Lopez Perez 2020-03-23 20:46:19 PDT

Also consider https://bugs.chromium.org/p/chromium/issues/detail?id=808758 which added WPT tests:
css/css-grid/grid-items/grid-items-percentage-margins-vertical-lr-???.html
css/css-grid/grid-items/grid-items-percentage-paddings-vertical-lr-???.html

Comment 3 Oriol Brufau 2020-05-15 16:50:42 PDT

Created attachment 399527 [details]
Patch

Comment 4 Oriol Brufau 2020-05-15 16:53:23 PDT

The patch is a port of these Chromium commits:
https://chromium-review.googlesource.com/c/chromium/src/+/1796323
https://chromium-review.googlesource.com/c/chromium/src/+/2033507

But the base code in WebKit was a slightly different so I had to improvise a bit.

Comment 5 Manuel Rego Casasnovas 2020-05-17 23:54:02 PDT

Comment on attachment 399527 [details]
Patch

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

This looks good, I have a question inline but otherwise r=me.
Please clarify that before landing.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:920
> +    GridTrackSizingDirection childBlockDirection = GridLayoutFunctions::flowAwareDirectionForChild(*renderGrid(), child, ForRows);

Why you need this here and not before?

Comment 6 Oriol Brufau 2020-05-18 12:03:09 PDT

Created attachment 399665 [details]
Patch

Comment 7 Oriol Brufau 2020-05-18 12:09:50 PDT

(In reply to Manuel Rego Casasnovas from comment #5)
> Comment on attachment 399527 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399527&action=review
> 
> This looks good, I have a question inline but otherwise r=me.
> Please clarify that before landing.
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:920
> > +    GridTrackSizingDirection childBlockDirection = GridLayoutFunctions::flowAwareDirectionForChild(*renderGrid(), child, ForRows);
> 
> Why you need this here and not before?

Good catch, the previous call needs to use childInlineDirection instead of ForColumns, I missed it since grid-items-minimum-width-orthogonal-001.html only covers the other axis.

Comment 8 EWS 2020-05-18 17:52:27 PDT

Committed r261841: <https://trac.webkit.org/changeset/261841>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399665 [details].

Comment 9 Radar WebKit Bug Importer 2020-05-18 17:53:19 PDT

<rdar://problem/63371690>

Comment 10 Yusuke Suzuki 2020-06-09 13:50:54 PDT

Re-opened since this is blocked by bug 212991

Comment 11 Oriol Brufau 2020-09-23 10:23:03 PDT

The spotify problem was because of bug 210089.
Now that it has been fixed, my patch doesn't break spotify.
So it should be fine to reland, I will post a rebased patch.

Comment 12 Oriol Brufau 2020-09-23 11:10:14 PDT

Created attachment 409485 [details]
Patch

Comment 13 Manuel Rego Casasnovas 2020-09-23 13:52:03 PDT

Comment on attachment 409485 [details]
Patch

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

r=me

Let's hope this time we don't get new regressions. :-)

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:115
> +static bool hasRelativeOrIntrinsicSizeForChild(const RenderBox& child, GridTrackSizingDirection direction)

Nit: Missing new line before method name.

Comment 14 Oriol Brufau 2020-09-23 14:37:38 PDT

Created attachment 409503 [details]
Patch

Comment 15 Oriol Brufau 2020-09-23 14:38:29 PDT

(In reply to Manuel Rego Casasnovas from comment #13)
> Nit: Missing new line before method name.

Done.

Comment 16 EWS 2020-09-23 15:31:52 PDT

Committed r267503: <https://trac.webkit.org/changeset/267503>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409503 [details].