Bug 191656 – [css-grid] Consider scrollbars in populateGridPositionsForDirection()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

We never care about scrollbars in RenderGrid::populateGridPositionsForDirection(),
that's fine if the scrollbars are at the end (e.g. on the right in horizontal writing mode and LTR direction)
but it causes problems when they're at the beginning (e.g. on the left in horizontal writing mode and RTL direction).

[Manuel Rego Casasnovas]

Created attachment 354905 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 354908 [details]
Patch

Fix build.

[Javier Fernandez]

Comment on attachment 354908 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1008
> +    if (isRowAxis && style().isHorizontalWritingMode() && !style().isLeftToRightDirection())

Maybe a comment explaining why we only have to do it when computing the grid row axis would be useful here.

[Manuel Rego Casasnovas]

Created attachment 354909 [details]
=Patch for landing

[Manuel Rego Casasnovas]

Comment on attachment 354908 [details]
Patch

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

Thanks for the review.

>> Source/WebCore/rendering/RenderGrid.cpp:1008
>> +    if (isRowAxis && style().isHorizontalWritingMode() && !style().isLeftToRightDirection())
> 
> Maybe a comment explaining why we only have to do it when computing the grid row axis would be useful here.

Done.

[WebKit Commit Bot]

Comment on attachment 354909 [details]
=Patch for landing

Clearing flags on attachment: 354909

Committed r238220: <https://trac.webkit.org/changeset/238220>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46092390>

[Truitt Savell]

The three imported/w3c tests from https://trac.webkit.org/changeset/238220/webkit are now image failures on iOS

imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001.html 
imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001.html 
imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001.html

Image diffs:
https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/r238225%20(977)/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001-diffs.html

https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/r238225%20(977)/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001-diffs.html

https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/r238225%20(977)/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001-diffs.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-grid%2Fgrid-model%2Fgrid-container-scrollbar-001.html%20imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-grid%2Fgrid-model%2Fgrid-container-scrollbar-vertical-lr-001.html%20imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcss-grid%2Fgrid-model%2Fgrid-container-scrollbar-vertical-rl-001.html

[Truitt Savell]

Reverted r238220 for reason:

Introduced failing tests to iOS and is slowing down EWS

Committed r238242: <https://trac.webkit.org/changeset/238242>

[Manuel Rego Casasnovas]

Created attachment 354974 [details]
Patch for testing IOS EWS

[EWS Watchlist]

Comment on attachment 354974 [details]
Patch for testing IOS EWS

Attachment 354974 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10008731

New failing tests:
imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001.html
imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001.html
imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001.html

[EWS Watchlist]

Created attachment 354993 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Frédéric Wang (:fredw)]

Comment on attachment 354974 [details]
Patch for testing IOS EWS

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001-expected.html:35
> +<p>The test passes if it has the same output than the reference.</p>

"same output as" (ok it's an imported test)

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001.html:7
> +<meta name="assert" content="This test verifes that scrollbars are properly painted on grid containers, and are shown in the expected position depending on the direction.">

verifies*

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001.html:32
> +<p>The test passes if it has the same output than the reference.</p>

as the reference

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001-expected.html:36
> +<p>The test passes if it has the same output than the reference.</p>

as*

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001.html:7
> +<meta name="assert" content="This test verifes that scrollbars are properly painted on grid containers, and are shown in the expected position depending on the direction.">

verifies*

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001.html:33
> +<p>The test passes if it has the same output than the reference.</p>

as*

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001-expected.html:36
> +<p>The test passes if it has the same output than the reference.</p>

as*

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001.html:7
> +<meta name="assert" content="This test verifes that scrollbars are properly painted on grid containers, and are shown in the expected position depending on the direction.">

verifies*

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001.html:33
> +<p>The test passes if it has the same output than the reference.</p>

as*

[Frédéric Wang (:fredw)]

(In reply to Build Bot from comment #12)
> Comment on attachment 354974 [details]
> Patch for testing IOS EWS
> 
> Attachment 354974 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/10008731
> 
> New failing tests:
> imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-
> scrollbar-vertical-rl-001.html
> imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-
> scrollbar-vertical-lr-001.html
> imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-
> scrollbar-001.html

These tests render correctly for be with run-safari --ios-simulator but I get the extra gray space when doing run-webkit-tests --ios-simulator. I suspect this is a bug in iOS so I would suggest just marking them as failing and opening a separate bug.

[Manuel Rego Casasnovas]

(In reply to Frédéric Wang (:fredw) from comment #14)
> Comment on attachment 354974 [details]
> Patch for testing IOS EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354974&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001-expected.html:35
> > +<p>The test passes if it has the same output than the reference.</p>
> 
> "same output as" (ok it's an imported test)
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001.html:7
> > +<meta name="assert" content="This test verifes that scrollbars are properly painted on grid containers, and are shown in the expected position depending on the direction.">
> 
> verifies*
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-001.html:32
> > +<p>The test passes if it has the same output than the reference.</p>
> 
> as the reference
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001-expected.html:36
> > +<p>The test passes if it has the same output than the reference.</p>
> 
> as*
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001.html:7
> > +<meta name="assert" content="This test verifes that scrollbars are properly painted on grid containers, and are shown in the expected position depending on the direction.">
> 
> verifies*
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-lr-001.html:33
> > +<p>The test passes if it has the same output than the reference.</p>
> 
> as*
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001-expected.html:36
> > +<p>The test passes if it has the same output than the reference.</p>
> 
> as*
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001.html:7
> > +<meta name="assert" content="This test verifes that scrollbars are properly painted on grid containers, and are shown in the expected position depending on the direction.">
> 
> verifies*
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-grid/grid-model/grid-container-scrollbar-vertical-rl-001.html:33
> > +<p>The test passes if it has the same output than the reference.</p>
> 
> as*

Thanks for the review, I've fixed these at:
https://github.com/web-platform-tests/wpt/pull/14092

I'm also using Ahem there as that fixes the small pixel differences on the vertical writing modes tests.

[Manuel Rego Casasnovas]

Created attachment 355035 [details]
Checking ios-sim EWS

[Manuel Rego Casasnovas]

Comment on attachment 355035 [details]
Checking ios-sim EWS

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

> Source/WebCore/rendering/RenderGrid.cpp:1008
> +#if !PLATFORM(IOS_FAMILY)

Fred what do you think about this?
With this change the test grid-container-scrollbar-001.html is passing in ios-sim EWS too.

The other tests were fixed by using Ahem font.

[Frédéric Wang (:fredw)]

Comment on attachment 355035 [details]
Checking ios-sim EWS

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

>> Source/WebCore/rendering/RenderGrid.cpp:1008
>> +#if !PLATFORM(IOS_FAMILY)
> 
> Fred what do you think about this?
> With this change the test grid-container-scrollbar-001.html is passing in ios-sim EWS too.
> 
> The other tests were fixed by using Ahem font.

I guess the proper fix would be that iOS returns 0 for scrollbarLogicalWidth(). Maybe smfr knows better what to do here.

[Manuel Rego Casasnovas]

Comment on attachment 355035 [details]
Checking ios-sim EWS

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

>>> Source/WebCore/rendering/RenderGrid.cpp:1008
>>> +#if !PLATFORM(IOS_FAMILY)
>> 
>> Fred what do you think about this?
>> With this change the test grid-container-scrollbar-001.html is passing in ios-sim EWS too.
>> 
>> The other tests were fixed by using Ahem font.
> 
> I guess the proper fix would be that iOS returns 0 for scrollbarLogicalWidth(). Maybe smfr knows better what to do here.

IMHO, this seems somehow out of scope of this fix. I could add a FIXME about that and report a bug, but should we block this change?

[Frédéric Wang (:fredw)]

(In reply to Manuel Rego Casasnovas from comment #20)
> Comment on attachment 355035 [details]
> Checking ios-sim EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355035&action=review
> 
> >>> Source/WebCore/rendering/RenderGrid.cpp:1008
> >>> +#if !PLATFORM(IOS_FAMILY)
> >> 
> >> Fred what do you think about this?
> >> With this change the test grid-container-scrollbar-001.html is passing in ios-sim EWS too.
> >> 
> >> The other tests were fixed by using Ahem font.
> > 
> > I guess the proper fix would be that iOS returns 0 for scrollbarLogicalWidth(). Maybe smfr knows better what to do here.
> 
> IMHO, this seems somehow out of scope of this fix. I could add a FIXME about
> that and report a bug, but should we block this change?

Ah, no I did not mean to do this in this bug. As I said in comment 15, I would rather open a separate bug to handle the case of iOS. I guess a quick !PLATFORM hack is ok for now.

[Manuel Rego Casasnovas]

Created attachment 355330 [details]
Patch for landing

[Manuel Rego Casasnovas]

Comment on attachment 355035 [details]
Checking ios-sim EWS

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

>>>>> Source/WebCore/rendering/RenderGrid.cpp:1008
>>>>> +#if !PLATFORM(IOS_FAMILY)
>>>> 
>>>> Fred what do you think about this?
>>>> With this change the test grid-container-scrollbar-001.html is passing in ios-sim EWS too.
>>>> 
>>>> The other tests were fixed by using Ahem font.
>>> 
>>> I guess the proper fix would be that iOS returns 0 for scrollbarLogicalWidth(). Maybe smfr knows better what to do here.
>> 
>> IMHO, this seems somehow out of scope of this fix. I could add a FIXME about that and report a bug, but should we block this change?
> 
> Ah, no I did not mean to do this in this bug. As I said in comment 15, I would rather open a separate bug to handle the case of iOS. I guess a quick !PLATFORM hack is ok for now.

Ok, reported bug #191857 about that. Thanks.

[Manuel Rego Casasnovas]

Comment on attachment 355035 [details]
Checking ios-sim EWS

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

>>>>> Source/WebCore/rendering/RenderGrid.cpp:1008
>>>>> +#if !PLATFORM(IOS_FAMILY)
>>>> 
>>>> Fred what do you think about this?
>>>> With this change the test grid-container-scrollbar-001.html is passing in ios-sim EWS too.
>>>> 
>>>> The other tests were fixed by using Ahem font.
>>> 
>>> I guess the proper fix would be that iOS returns 0 for scrollbarLogicalWidth(). Maybe smfr knows better what to do here.
>> 
>> IMHO, this seems somehow out of scope of this fix. I could add a FIXME about that and report a bug, but should we block this change?
> 
> Ah, no I did not mean to do this in this bug. As I said in comment 15, I would rather open a separate bug to handle the case of iOS. I guess a quick !PLATFORM hack is ok for now.

Ok, reported bug #191857 about that. Thanks.

[WebKit Commit Bot]

Comment on attachment 355330 [details]
Patch for landing

Clearing flags on attachment: 355330

Committed r238395: <https://trac.webkit.org/changeset/238395>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.