third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html - Issue 2670603002: Avoid pathological layout on nested percent height tables

Side by Side Diff: third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.html

Issue 2670603002: Avoid pathological layout on nested percent height tables (Closed)

Patch Set: bug 687061 Created 3 years, 10 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:

« no previous file with comments | « third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/height-css-tables-expected.html ('k') | third_party/WebKit/LayoutTests/paint/invalidation/table-two-pass-layout-overpaint-expected.txt » ('j') | third_party/WebKit/PerformanceTests/Layout/nested-percent-height-tables.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 <html>	1 <html>

2 <head>	2 <head>

3 <style>	3 <style>

4 table { background-color: lightblue; height: 100px; }	4 table { background-color: lightblue; height: 99px; }
	mstensho (USE GERRIT) 2017/02/06 21:11:07 This seems wrong. The table in the test really is This seems wrong. The table in the test really is 100px tall, isn't it? I don't think this is an acceptable regression. mstensho (USE GERRIT) 2017/02/07 19:52:33 This is still unresolved. Show quoted text On 2017/02/06 21:11:07, mstensho wrote: > This seems wrong. The table in the test really is 100px tall, isn't it? I don't > think this is an acceptable regression. This is still unresolved. rhogan 2017/02/07 20:27:42 Without my CL the rendered height on the table in Show quoted text On 2017/02/07 at 19:52:33, mstensho wrote: > On 2017/02/06 21:11:07, mstensho wrote: > > This seems wrong. The table in the test really is 100px tall, isn't it? I don't > > think this is an acceptable regression. > > This is still unresolved. Without my CL the rendered height on the table in table-two-pass-layout-overpaint-expected.html file is 98px. The extra 2px get discarded because distributeExtraLogicalHeight() fails to allocate them from the 'extra height available' of 46px left over by calcRowLogicalHeight(). Likewise for LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html. With my CL (and leaving table-two-pass-layout-overpaint-expected.html unchanged) the rendered height on the table in table-two-pass-layout-overpaint-expected.html is 99px. This is because we are using the 98px we arrived at the last time we laid out the section and trying to redistribute the 2px. We manage to allocate 1px and are left with 1px left over. On LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html we are still left with 2px left over because we're doing a single layout pass as a result of the style change, so visiting calcRowLogicalHeight(), getting 46px to reallocate, and getting left with 2px. So on the one hand we're not rendering the 100px table as 100px in the first place, but we are left with an 1px inconsistency in the height depending on how many layouts we trigger. Taking on the FIXME in distributeExtraLogicalHeight() as a follow-up seems like the way to address this? I suspect it will be a tricky one. mstensho (USE GERRIT) 2017/02/07 21:09:10 Oh dear. I had no idea it was this broken. Thanks Show quoted text On 2017/02/07 20:27:42, rhogan wrote: > On 2017/02/07 at 19:52:33, mstensho wrote: > > On 2017/02/06 21:11:07, mstensho wrote: > > > This seems wrong. The table in the test really is 100px tall, isn't it? I > don't > > > think this is an acceptable regression. > > > > This is still unresolved. > > Without my CL the rendered height on the table in > table-two-pass-layout-overpaint-expected.html file is 98px. The extra 2px get > discarded because distributeExtraLogicalHeight() fails to allocate them from the > 'extra height available' of 46px left over by calcRowLogicalHeight(). Likewise > for LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html. > > With my CL (and leaving table-two-pass-layout-overpaint-expected.html unchanged) > the rendered height on the table in > table-two-pass-layout-overpaint-expected.html is 99px. This is because we are > using the 98px we arrived at the last time we laid out the section and trying to > redistribute the 2px. We manage to allocate 1px and are left with 1px left over. > > On LayoutTests/paint/invalidation/table-two-pass-layout-overpaint.html we are > still left with 2px left over because we're doing a single layout pass as a > result of the style change, so visiting calcRowLogicalHeight(), getting 46px to > reallocate, and getting left with 2px. > > So on the one hand we're not rendering the 100px table as 100px in the first > place, but we are left with an 1px inconsistency in the height depending on how > many layouts we trigger. > > Taking on the FIXME in distributeExtraLogicalHeight() as a follow-up seems like > the way to address this? I suspect it will be a tricky one. > Oh dear. I had no idea it was this broken. Thanks for the explanation. Yeah, attacking that FIXME sounds like the proper way to fix this (and it might become more urgent with this fix landed... then again, nobody has been complaining loudly about us losing a couple of pixels either, so...). So the test used to pass by accident. It's confusing that the test and the ref don't use the same table height, though. I think you should either: A: Keep the ref at 100px and change it to also use two-pass layout (by changing the table width, or by setting display:none on some new sibling of #target, or whatever), and document why you do this. or: B: document why it has to be 99px. A is silly, but, on the other hand, the specified height mismatch between the test and the ref is hacky. We probably need a bug report for the FIXME at this point, so that we can refer to it.
5 td { width: 100px; height: 50px; }	5 td { width: 100px; height: 50px; }

6 div#target { height: 25px; width: 50px; background-color: lightgreen; }	6 div#target { height: 25px; width: 50px; background-color: lightgreen; }

7 </style>	7 </style>

8 </head>	8 </head>

9 <body>	9 <body>

10 <table>	10 <table>

11 <tr>	11 <tr>

12 <td>	12 <td>

13 <div id="target"></div>	13 <div id="target"></div>

14 </td>	14 </td>

15 </tr>	15 </tr>

16 </table>	16 </table>

17 </body>	17 </body>

18 </html>	18 </html>

OLD	NEW