Bug 209461 – [css-grid] Grid layout unstable with percentage margin

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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

[Oriol Brufau]

Created attachment 399527 [details]
Patch

[Oriol Brufau]

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.

[Manuel Rego Casasnovas]

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?

[Oriol Brufau]

Created attachment 399665 [details]
Patch

[Oriol Brufau]

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

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/63371690>

[Yusuke Suzuki]

Re-opened since this is blocked by bug 212991

[Oriol Brufau]

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.

[Oriol Brufau]

Created attachment 409485 [details]
Patch

[Manuel Rego Casasnovas]

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.

[Oriol Brufau]

Created attachment 409503 [details]
Patch

[Oriol Brufau]

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

Done.

[EWS]

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

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