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).
Created attachment 354905 [details] Patch
Created attachment 354908 [details] Patch Fix build.
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.
Created attachment 354909 [details] =Patch for landing
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 on attachment 354909 [details] =Patch for landing Clearing flags on attachment: 354909 Committed r238220: <https://trac.webkit.org/changeset/238220>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46092390>
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
Reverted r238220 for reason: Introduced failing tests to iOS and is slowing down EWS Committed r238242: <https://trac.webkit.org/changeset/238242>
Created attachment 354974 [details] Patch for testing IOS EWS
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
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 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*
(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.
(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.
Created attachment 355035 [details] Checking ios-sim EWS
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 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 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?
(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.
Created attachment 355330 [details] Patch for landing
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 on attachment 355330 [details] Patch for landing Clearing flags on attachment: 355330 Committed r238395: <https://trac.webkit.org/changeset/238395>