Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
532203002:
Test: Don't add scrollbar width to intrinsic width of fixed-width grid items (Closed)
|
Created:
6 years, 3 months ago by Sunil Ratnu Modified:
6 years, 3 months ago CC:
blink-reviews, jfernandez, Manuel Rego, svillar Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionTest: Don't add scrollbar width to intrinsic width of fixed-width grid items
The scroll bar width should only be added if the width of the grid item is
not fixed.
This test coverage has been inspired from the webkit patch for flexboxes at
https://bugs.webkit.org/attachment.cgi?id=182187&action=diff
BUG=395788
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181590
Patch Set 1 #
Total comments: 7
Patch Set 2 : Incorporating review comments #
Total comments: 5
Patch Set 3 : More variety in test coverage #
Messages
Total messages: 12 (2 generated)
sunil.ratnu@samsung.com changed reviewers: + jchaffraix@chromium.org, ojan@chromium.org
Please review this. Thanks!
We prefer the description to be below 80 characters (I advise 72 based on the discussion in https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY ). Ideally you would want to put a reference to the section of the specification that you're testing in the description. I think this would be helpful to understand what you are actually trying to test as it's not totally clear to me. https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... File LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html (right): https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:39: height:50px; All the styles are the same (apart from the colors but we don't really care about them anyway, they are just a visual cue). That means that we should factor the common style into a single class to apply to all items. More conceptually, this means that we are doing a lot of redundant checks as there is really nothing that changes between the grid items (the grid tracks are all sized the same). https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:39: height:50px; All the styles are the same (apart from the colors but we don't really care about them anyway, they are just a visual cue). That means that we should factor the common style into a single class to apply to all items. More conceptually, this means that we are doing a lot of redundant checks as there is really nothing that changes between the grid items (the grid tracks are all sized the same). https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:45: <p>This test checks that scrollbar width should not be added to the intrinsic width of fixed-width grid items.</p> data-expected-width measures the actual width, not the intrinsic width, so this sentence looks suspicious. https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:45: <p>This test checks that scrollbar width should not be added to the intrinsic width of fixed-width grid items.</p> data-expected-width measures the actual width, not the intrinsic width, so this sentence looks suspicious.
On 2014/09/03 15:35:14, Julien Chaffraix - PST wrote: > We prefer the description to be below 80 characters (I advise 72 based on the > discussion in > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY ). > > Ideally you would want to put a reference to the section of the specification > that you're testing in the description. I think this would be helpful to > understand what you are actually trying to test as it's not totally clear to me. > > https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... > File > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html > (right): > > https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:39: > height:50px; > All the styles are the same (apart from the colors but we don't really care > about them anyway, they are just a visual cue). > > That means that we should factor the common style into a single class to apply > to all items. > > More conceptually, this means that we are doing a lot of redundant checks as > there is really nothing that changes between the grid items (the grid tracks are > all sized the same). > > https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:39: > height:50px; > All the styles are the same (apart from the colors but we don't really care > about them anyway, they are just a visual cue). > > That means that we should factor the common style into a single class to apply > to all items. > > More conceptually, this means that we are doing a lot of redundant checks as > there is really nothing that changes between the grid items (the grid tracks are > all sized the same). > > https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:45: > <p>This test checks that scrollbar width should not be added to the intrinsic > width of fixed-width grid items.</p> > data-expected-width measures the actual width, not the intrinsic width, so this > sentence looks suspicious. > > https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:45: > <p>This test checks that scrollbar width should not be added to the intrinsic > width of fixed-width grid items.</p> > data-expected-width measures the actual width, not the intrinsic width, so this > sentence looks suspicious. Thanks Julien for the review. This change is inspired from one of Ojan's change for flexbox at https://bugs.webkit.org/attachment.cgi?id=182187&action=diff#a/LayoutTests/fa... . Maybe that might help as I was not able to find the spec part for this. I will make the other suggested changes tomorrow morning and upload a new patch.
https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... File LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html (right): https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:39: height:50px; On 2014/09/03 15:35:14, Julien Chaffraix - PST wrote: > All the styles are the same (apart from the colors but we don't really care > about them anyway, they are just a visual cue). > > That means that we should factor the common style into a single class to apply > to all items. > > More conceptually, this means that we are doing a lot of redundant checks as > there is really nothing that changes between the grid items (the grid tracks are > all sized the same). Done. https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:45: <p>This test checks that scrollbar width should not be added to the intrinsic width of fixed-width grid items.</p> On 2014/09/03 15:35:14, Julien Chaffraix - PST wrote: > data-expected-width measures the actual width, not the intrinsic width, so this > sentence looks suspicious. Done.The main thing which I wish to check here is that in case of fixed width GRID ITEMS, the scroll bar width does not get added. Contrary to this case, in case of blocks, the scroll bar width gets added to the width of fixed width item i.e. when we have fixed width box item of 50px then on overflow:scroll, scroll bar width also gets added. (https://bugs.webkit.org/attachment.cgi?id=182187&action=diff#a/LayoutTests/fa...)
https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... File LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html (right): https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:45: <p>This test checks that scrollbar width should not be added to the intrinsic width of fixed-width grid items.</p> On 2014/09/04 10:29:21, Sunil Ratnu wrote: > On 2014/09/03 15:35:14, Julien Chaffraix - PST wrote: > > data-expected-width measures the actual width, not the intrinsic width, so > this > > sentence looks suspicious. > > Done.The main thing which I wish to check here is that in case of fixed width > GRID ITEMS, the scroll bar width does not get added. Contrary to this case, in > case of blocks, the scroll bar width gets added to the width of fixed width item > i.e. when we have fixed width box item of 50px then on overflow:scroll, scroll > bar width also gets added. > (https://bugs.webkit.org/attachment.cgi?id=182187&action=diff#a/LayoutTests/fa...) Thanks for the context. It would be way better to fix the FIXME that Ojan added to RenderGrid as part of this change and have correct testing for both cases IMO. Also we should mention this change in the description as it's good information. https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html (right): https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:7: grid-template-rows: 200px 200px; I would change the grid cells' sizes to have more variety. https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:26: <div class="secondRowSecondColumn gridItemScrollOverflow" data-expected-width="50" data-expected-height="50"></div> We will want one of the grid items to be also a grid container for better coverage. https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:26: <div class="secondRowSecondColumn gridItemScrollOverflow" data-expected-width="50" data-expected-height="50"></div> We will want one of the grid items to be also a grid container for better coverage.
On 2014/09/05 17:32:13, Julien Chaffraix - PST wrote: > https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... > File > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html > (right): > > https://codereview.chromium.org/532203002/diff/1/LayoutTests/fast/css-grid-la... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:45: > <p>This test checks that scrollbar width should not be added to the intrinsic > width of fixed-width grid items.</p> > On 2014/09/04 10:29:21, Sunil Ratnu wrote: > > On 2014/09/03 15:35:14, Julien Chaffraix - PST wrote: > > > data-expected-width measures the actual width, not the intrinsic width, so > > this > > > sentence looks suspicious. > > > > Done.The main thing which I wish to check here is that in case of fixed width > > GRID ITEMS, the scroll bar width does not get added. Contrary to this case, in > > case of blocks, the scroll bar width gets added to the width of fixed width > item > > i.e. when we have fixed width box item of 50px then on overflow:scroll, scroll > > bar width also gets added. > > > (https://bugs.webkit.org/attachment.cgi?id=182187&action=diff#a/LayoutTests/fa...) > > Thanks for the context. It would be way better to fix the FIXME that Ojan added > to RenderGrid as part of this change and have correct testing for both cases > IMO. Thanks Julien for the review. I've already submitted the patch covering that part in one of my other patches.(Link to patch: https://codereview.chromium.org/535913002/ > Also we should mention this change in the description as it's good information. Done. I've added this in the description as well. > https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... > File > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html > (right): > > https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:7: > grid-template-rows: 200px 200px; > I would change the grid cells' sizes to have more variety. > > https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:26: > <div class="secondRowSecondColumn gridItemScrollOverflow" > data-expected-width="50" data-expected-height="50"></div> > We will want one of the grid items to be also a grid container for better > coverage. > > https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... > LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:26: > <div class="secondRowSecondColumn gridItemScrollOverflow" > data-expected-width="50" data-expected-height="50"></div> > We will want one of the grid items to be also a grid container for better > coverage. I will do rest of the changes the first thing in the morning tomorrow and upload a new patchset.
Hi Julien, I've made the changes you suggested. Please review the latest patch. Thanks! https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... File LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html (right): https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:7: grid-template-rows: 200px 200px; On 2014/09/05 17:32:13, Julien Chaffraix - PST wrote: > I would change the grid cells' sizes to have more variety. Done. https://codereview.chromium.org/532203002/diff/20001/LayoutTests/fast/css-gri... LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html:26: <div class="secondRowSecondColumn gridItemScrollOverflow" data-expected-width="50" data-expected-height="50"></div> On 2014/09/05 17:32:13, Julien Chaffraix - PST wrote: > We will want one of the grid items to be also a grid container for better > coverage. Done.
The CQ bit was checked by jchaffraix@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sunil.ratnu@samsung.com/532203002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 181590 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
