LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html - Issue 532203002: Test: Don't add scrollbar width to intrinsic width of fixed-width grid items

Side by Side Diff: LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid.html

Issue 532203002: Test: Don't add scrollbar width to intrinsic width of fixed-width grid items (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Created 6 years, 3 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « no previous file | LayoutTests/fast/css-grid-layout/fixed-width-intrinsic-width-should-exclude-scrollbar-width-in-grid-expected.txt » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <link href="resources/grid.css" rel="stylesheet">

	4 <style>

	5 .grid {

	6 grid-template-columns: 200px 200px;

	7 grid-template-rows: 200px 200px;

	8 }

	9

	10 .firstRowFirstColumn {

	11 color: blue;

	12 background: red;

	13 overflow: scroll;

	14 width:50px;

	15 height:50px;

	16 }

	17

	18 .firstRowSecondColumn {

	19 color: yellow;

	20 background: green;

	21 overflow: scroll;

	22 width:50px;

	23 height:50px;

	24 }

	25

	26 .secondRowFirstColumn {

	27 color: gray;

	28 background: pink;

	29 overflow: scroll;

	30 width:50px;

	31 height:50px;

	32 }

	33

	34 .secondRowSecondColumn {

	35 color: orange;

	36 background: brown;

	37 overflow: scroll;

	38 width:50px;

	39 height:50px;
	Julien - ping for review 2014/09/03 15:35:14 All the styles are the same (apart from the colors 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). Julien - ping for review 2014/09/03 15:35:14 All the styles are the same (apart from the colors 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). Sunil Ratnu 2014/09/04 10:29:21 Done. Show quoted text 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.
	40 }

	41 </style>

	42 <script src="../../resources/check-layout.js"></script>

	43 <body onload="checkLayout('.grid')">

	44

	45 <p>This test checks that scrollbar width should not be added to the intrinsic wi dth of fixed-width grid items.</p>
	Julien - ping for review 2014/09/03 15:35:14 data-expected-width measures the actual width, not data-expected-width measures the actual width, not the intrinsic width, so this sentence looks suspicious. Julien - ping for review 2014/09/03 15:35:14 data-expected-width measures the actual width, not data-expected-width measures the actual width, not the intrinsic width, so this sentence looks suspicious. Sunil Ratnu 2014/09/04 10:29:21 Done.The main thing which I wish to check here is Show quoted text 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...) Julien - ping for review 2014/09/05 17:32:12 Thanks for the context. It would be way better to Show quoted text 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.
	46

	47 <div class="grid">

	48 <div class="firstRowFirstColumn" data-expected-width="50" data-expected-heig ht="50"></div>

	49 <div class="firstRowSecondColumn" data-expected-width="50" data-expected-hei ght="50"></div>

	50 <div class="secondRowFirstColumn" data-expected-width="50" data-expected-hei ght="50"></div>

	51 <div class="secondRowSecondColumn" data-expected-width="50" data-expected-he ight="50"></div>

	52 </div>

	53

	54 </body>

	55 </html>

OLD	NEW