Bug 191656 - [css-grid] Consider scrollbars in populateGridPositionsForDirection()
Summary: [css-grid] Consider scrollbars in populateGridPositionsForDirection()
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: Manuel Rego Casasnovas
URL:
Keywords: BlinkMergeCandidate, InRadar
Depends on:
Blocks:
 
Reported: 2018-11-14 14:19 PST by Manuel Rego Casasnovas
Modified: 2018-11-20 03:58 PST (History)
10 users (show)

See Also:


Attachments
Patch (19.85 KB, patch)
2018-11-15 02:45 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (19.85 KB, patch)
2018-11-15 03:08 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
=Patch for landing (20.03 KB, patch)
2018-11-15 03:39 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for testing IOS EWS (20.03 KB, patch)
2018-11-15 12:52 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (9.96 MB, application/zip)
2018-11-15 15:43 PST, Build Bot
no flags Details
Checking ios-sim EWS (20.20 KB, patch)
2018-11-16 01:53 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (20.34 KB, patch)
2018-11-20 03:16 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2018-11-14 14:19:27 PST
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).
Comment 1 Manuel Rego Casasnovas 2018-11-15 02:45:59 PST
Created attachment 354905 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2018-11-15 03:08:38 PST
Created attachment 354908 [details]
Patch

Fix build.
Comment 3 Javier Fernandez 2018-11-15 03:16:40 PST
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.
Comment 4 Manuel Rego Casasnovas 2018-11-15 03:39:20 PST
Created attachment 354909 [details]
=Patch for landing
Comment 5 Manuel Rego Casasnovas 2018-11-15 03:40:12 PST
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.
Comment 6 WebKit Commit Bot 2018-11-15 04:19:31 PST
Comment on attachment 354909 [details]
=Patch for landing

Clearing flags on attachment: 354909

Committed r238220: <https://trac.webkit.org/changeset/238220>
Comment 7 WebKit Commit Bot 2018-11-15 04:19:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-11-15 04:20:31 PST
<rdar://problem/46092390>
Comment 9 Truitt Savell 2018-11-15 08:47:54 PST
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
Comment 10 Truitt Savell 2018-11-15 12:40:47 PST
Reverted r238220 for reason:

Introduced failing tests to iOS and is slowing down EWS

Committed r238242: <https://trac.webkit.org/changeset/238242>
Comment 11 Manuel Rego Casasnovas 2018-11-15 12:52:25 PST
Created attachment 354974 [details]
Patch for testing IOS EWS
Comment 12 Build Bot 2018-11-15 15:43:16 PST
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
Comment 13 Build Bot 2018-11-15 15:43:19 PST
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
Comment 14 Frédéric Wang (:fredw) 2018-11-15 22:23:24 PST
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*
Comment 15 Frédéric Wang (:fredw) 2018-11-15 22:57:06 PST
(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.
Comment 16 Manuel Rego Casasnovas 2018-11-16 01:01:06 PST
(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.
Comment 17 Manuel Rego Casasnovas 2018-11-16 01:53:50 PST
Created attachment 355035 [details]
Checking ios-sim EWS
Comment 18 Manuel Rego Casasnovas 2018-11-16 03:51:57 PST
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.
Comment 19 Frédéric Wang (:fredw) 2018-11-19 07:08:33 PST
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.
Comment 20 Manuel Rego Casasnovas 2018-11-20 01:41:41 PST
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?
Comment 21 Frédéric Wang (:fredw) 2018-11-20 01:46:03 PST
(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.
Comment 22 Manuel Rego Casasnovas 2018-11-20 03:16:17 PST
Created attachment 355330 [details]
Patch for landing
Comment 23 Manuel Rego Casasnovas 2018-11-20 03:46:32 PST
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.
Comment 24 Manuel Rego Casasnovas 2018-11-20 03:47:05 PST
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.
Comment 25 WebKit Commit Bot 2018-11-20 03:58:38 PST
Comment on attachment 355330 [details]
Patch for landing

Clearing flags on attachment: 355330

Committed r238395: <https://trac.webkit.org/changeset/238395>
Comment 26 WebKit Commit Bot 2018-11-20 03:58:40 PST
All reviewed patches have been landed.  Closing bug.