| Summary: | [css-grid] Consider scrollbars in populateGridPositionsForDirection() | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||||||||||
| Component: | Layout and Rendering | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, commit-queue, ews-watchlist, fred.wang, jfernandez, simon.fraser, svillar, tsavell, webkit-bug-importer, zalan | ||||||||||||||||
| Priority: | P2 | Keywords: | BlinkMergeCandidate, InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| See Also: | https://bugs.chromium.org/p/chromium/issues/detail?id=904438 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Manuel Rego Casasnovas
2018-11-14 14:19:27 PST
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. 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 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> All reviewed patches have been landed. Closing bug. |