Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1102953002:
BR styled clear is ignored while partially relayouting. (Closed)
|
Created:
5 years, 8 months ago by changseok Modified:
5 years, 7 months ago CC:
blink-reviews, blink-reviews-rendering, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionBR styled clear is ignored while partially relayouting.
If a br with a clear style is placed on the end of a line it is collapsed
and subsequently not taken into account when matching the ends of lines.
When partially relayouting, we find out which line is a start and a end.
But the tailing br is ignored so that its height is not included in a calculated
logical height of LayoutBlockFlow. To fix this, we check if the end of a line is
a br with a clear style and if its container has a floating box. If that's the case,
we force the block to layout entire lines not to skip the br.
BUG=468568
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195993
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rename needsToCheckEndline and update the test case #Patch Set 3 : Update the test not to use focus nor blur. #Patch Set 4 : Updated patch #Patch Set 5 : Added one more test ended with empty span. #Patch Set 6 : Better approach. #
Total comments: 12
Patch Set 7 : Addressed comments #
Total comments: 4
Patch Set 8 : Rebased #Patch Set 9 : Adressed comments #
Total comments: 2
Patch Set 10 : Updated comment. #
Total comments: 4
Patch Set 11 : Rebased #Patch Set 12 : Comment updated. #
Messages
Total messages: 47 (3 generated)
shivamidow@gmail.com changed reviewers: + eae@chromium.org, esprehn@chromium.org, mstensho@opera.com
Please have a look
leviw@chromium.org changed reviewers: + leviw@chromium.org
https://codereview.chromium.org/1102953002/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlockFlowLine.cpp:799: const bool needsToCheckEndLine = containsFloats() && lastChild() && lastChild()->isBR() && lastChild()->style()->clear(); Driveby: This needs a better name. The addition of "needs" to the variable doesn't make it clear what the difference between this and the bool below is. Why are you checking lastChild directly? Does this only happen when the br in question is the last child? That seems suspicious...
Are you sure this is specific to br? When I changed it to a span in testing it still happened. On Apr 24, 2015 8:55 AM, <leviw@chromium.org> wrote: > > > https://codereview.chromium.org/1102953002/diff/1/Source/core/layout/LayoutBl... > File Source/core/layout/LayoutBlockFlowLine.cpp (right): > > > https://codereview.chromium.org/1102953002/diff/1/Source/core/layout/LayoutBl... > Source/core/layout/LayoutBlockFlowLine.cpp:799: const bool > needsToCheckEndLine = containsFloats() && lastChild() && > lastChild()->isBR() && lastChild()->style()->clear(); > Driveby: This needs a better name. The addition of "needs" to the > variable doesn't make it clear what the difference between this and the > bool below is. > > Why are you checking lastChild directly? Does this only happen when the > br in question is the last child? That seems suspicious... > > https://codereview.chromium.org/1102953002/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to blink-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/04/24 19:20:21, esprehn wrote: > Are you sure this is specific to br? When I changed it to a span in testing > it still happened. If you changed the last br to something like <span style="clear:both">test<span>, yeah you should see shorter height of yellow than green's. I think that's correct for span. You can see a same result in gecko.
I'm always not good at naming. Welcome your suggestion for an alternative value name :) https://codereview.chromium.org/1102953002/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlockFlowLine.cpp:799: const bool needsToCheckEndLine = containsFloats() && lastChild() && lastChild()->isBR() && lastChild()->style()->clear(); On 2015/04/24 18:55:47, leviw wrote: > Driveby: This needs a better name. The addition of "needs" to the variable > doesn't make it clear what the difference between this and the bool below is. > > Why are you checking lastChild directly? Does this only happen when the br in > question is the last child? That seems suspicious... Yes it does as long as I know. If the br places in middle line of LayoutBlockFlow and something visible like inlineBox or layoutBlock is followed, then we can determine a endLine with the visible one. This bug only happens when br with clear is the last child in LayoutBlockFlow. In this case, the end line is the text node which is just before the last br and the start line is the anchor. We calculate height with only those two lines so the height shrinks after re-layout. However we go over the two line and see the next line, br, we call clearFloats() in the while loop and then we would finally have correct height of layoutBlockFlow.
https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... File LayoutTests/fast/block/float/br-with-clear-3.html (right): https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = document.querySelector('a'); This test is flaky on my computer (without your fix it fails most of the time, but not always). I don't know why a focus / blur requires a relayout anyway. Maybe you should make some explicit style modification on some node to trigger the buggy relayout instead? And put a "document.body.offsetTop;" in front of the style modification code, to make sure that you get separate layout passes?
https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... File LayoutTests/fast/block/float/br-with-clear-3.html (right): https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = document.querySelector('a'); On 2015/04/25 17:31:14, mstensho wrote: > This test is flaky on my computer (without your fix it fails most of the time, > but not always). I don't know why a focus / blur requires a relayout anyway. > Maybe you should make some explicit style modification on some node to trigger > the buggy relayout instead? And put a "document.body.offsetTop;" in front of the > style modification code, to make sure that you get separate layout passes? Hrm. That's weird. I've never failed the test with my fix. :/ Why focus/blur requires a relayout is that it causes drawing/removing a focus ring on anchor node. I don't know what is the problem on your system through, you might block to draw a focus ring? Well O.K. let me update the test as you said. It's just a piece of cake.
Layout tracks overflow, and focus rings are visual overflow so we do a layout. I think all this was done for repaint after layout before we had the overflow update path at the end of recalcStyle. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2015/04/25 17:31:14, mstensho wrote: > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > document.querySelector('a'); > This test is flaky on my computer (without your fix it fails most of the time, > but not always). I don't know why a focus / blur requires a relayout anyway. > Maybe you should make some explicit style modification on some node to trigger > the buggy relayout instead? And put a "document.body.offsetTop;" in front of the > style modification code, to make sure that you get separate layout passes? I could not reproduce the bug with any style changes. Instead, I could see the flakiness on linux system. (ubuntu 15.04) I guess the reason is that we call blur() immediately after focus(). So blur is simply removed from the test. You should see the bug always.
On 2015/04/27 05:06:03, changseok wrote: > On 2015/04/25 17:31:14, mstensho wrote: > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > > document.querySelector('a'); > > This test is flaky on my computer (without your fix it fails most of the time, > > but not always). I don't know why a focus / blur requires a relayout anyway. > > Maybe you should make some explicit style modification on some node to trigger > > the buggy relayout instead? And put a "document.body.offsetTop;" in front of > the > > style modification code, to make sure that you get separate layout passes? > > I could not reproduce the bug with any style changes. It's so easy. Try this: <!DOCTYPE html> <p>There should be a blue <em>square</em> below.</p> <div style="float:right; width:1em; height:20em;"></div> <div style="width:20em; background:blue;"> <span id="elm"></span><br> <br style="clear:both"> </div> <script> onload = function() { document.body.offsetTop; document.getElementById('elm').style.display = 'none'; } </script>
On 2015/04/27 08:16:09, mstensho wrote: > On 2015/04/27 05:06:03, changseok wrote: > > On 2015/04/25 17:31:14, mstensho wrote: > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > > > document.querySelector('a'); > > > This test is flaky on my computer (without your fix it fails most of the > time, > > > but not always). I don't know why a focus / blur requires a relayout anyway. > > > Maybe you should make some explicit style modification on some node to > trigger > > > the buggy relayout instead? And put a "document.body.offsetTop;" in front of > > the > > > style modification code, to make sure that you get separate layout passes? > > > > I could not reproduce the bug with any style changes. > > It's so easy. Try this: > > <!DOCTYPE html> > <p>There should be a blue <em>square</em> below.</p> > <div style="float:right; width:1em; height:20em;"></div> > <div style="width:20em; background:blue;"> > <span id="elm"></span><br> > <br style="clear:both"> > </div> > <script> > onload = function() { > document.body.offsetTop; > document.getElementById('elm').style.display = 'none'; > } > </script> I could always see a blue square on ToT. That's an another issue already fixed I guess?
On 2015/04/27 10:11:35, changseok wrote: > On 2015/04/27 08:16:09, mstensho wrote: > > On 2015/04/27 05:06:03, changseok wrote: > > > On 2015/04/25 17:31:14, mstensho wrote: > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > > > > document.querySelector('a'); > > > > This test is flaky on my computer (without your fix it fails most of the > > time, > > > > but not always). I don't know why a focus / blur requires a relayout > anyway. > > > > Maybe you should make some explicit style modification on some node to > > trigger > > > > the buggy relayout instead? And put a "document.body.offsetTop;" in front > of > > > the > > > > style modification code, to make sure that you get separate layout passes? > > > > > > I could not reproduce the bug with any style changes. > > > > It's so easy. Try this: > > > > <!DOCTYPE html> > > <p>There should be a blue <em>square</em> below.</p> > > <div style="float:right; width:1em; height:20em;"></div> > > <div style="width:20em; background:blue;"> > > <span id="elm"></span><br> > > <br style="clear:both"> > > </div> > > <script> > > onload = function() { > > document.body.offsetTop; > > document.getElementById('elm').style.display = 'none'; > > } > > </script> > > I could always see a blue square on ToT. That's an another issue already fixed I > guess? Indeed, I tested with an older build. How about this, then: <!DOCTYPE html> <p>There should be a blue <em>square</em> below.</p> <div style="float:right; width:1em; height:20em;"></div> <div style="width:20em; background:blue;"> <span id="elm"> </span><br> <br style="clear:both"> </div> <script> onclick = function() { document.body.offsetTop; document.getElementById('elm').style.fontSize = '0.5em'; } </script>
On 2015/04/27 10:38:01, mstensho wrote: > On 2015/04/27 10:11:35, changseok wrote: > > On 2015/04/27 08:16:09, mstensho wrote: > > > On 2015/04/27 05:06:03, changseok wrote: > > > > On 2015/04/25 17:31:14, mstensho wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > > > > > document.querySelector('a'); > > > > > This test is flaky on my computer (without your fix it fails most of the > > > time, > > > > > but not always). I don't know why a focus / blur requires a relayout > > anyway. > > > > > Maybe you should make some explicit style modification on some node to > > > trigger > > > > > the buggy relayout instead? And put a "document.body.offsetTop;" in > front > > of > > > > the > > > > > style modification code, to make sure that you get separate layout > passes? > > > > > > > > I could not reproduce the bug with any style changes. > > > > > > It's so easy. Try this: > > > > > > <!DOCTYPE html> > > > <p>There should be a blue <em>square</em> below.</p> > > > <div style="float:right; width:1em; height:20em;"></div> > > > <div style="width:20em; background:blue;"> > > > <span id="elm"></span><br> > > > <br style="clear:both"> > > > </div> > > > <script> > > > onload = function() { > > > document.body.offsetTop; > > > document.getElementById('elm').style.display = 'none'; > > > } > > > </script> > > > > I could always see a blue square on ToT. That's an another issue already fixed > I > > guess? > > Indeed, I tested with an older build. How about this, then: > > <!DOCTYPE html> > <p>There should be a blue <em>square</em> below.</p> > <div style="float:right; width:1em; height:20em;"></div> > <div style="width:20em; background:blue;"> > <span id="elm"> </span><br> > <br style="clear:both"> > </div> > <script> > onclick = function() { > document.body.offsetTop; > document.getElementById('elm').style.fontSize = '0.5em'; > } > </script> Same. But if I click in somewhere of the test page, the bug happens.
On 2015/04/27 10:47:46, changseok wrote: > On 2015/04/27 10:38:01, mstensho wrote: > > On 2015/04/27 10:11:35, changseok wrote: > > > On 2015/04/27 08:16:09, mstensho wrote: > > > > On 2015/04/27 05:06:03, changseok wrote: > > > > > On 2015/04/25 17:31:14, mstensho wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > > > > > > document.querySelector('a'); > > > > > > This test is flaky on my computer (without your fix it fails most of > the > > > > time, > > > > > > but not always). I don't know why a focus / blur requires a relayout > > > anyway. > > > > > > Maybe you should make some explicit style modification on some node to > > > > trigger > > > > > > the buggy relayout instead? And put a "document.body.offsetTop;" in > > front > > > of > > > > > the > > > > > > style modification code, to make sure that you get separate layout > > passes? > > > > > > > > > > I could not reproduce the bug with any style changes. > > > > > > > > It's so easy. Try this: > > > > > > > > <!DOCTYPE html> > > > > <p>There should be a blue <em>square</em> below.</p> > > > > <div style="float:right; width:1em; height:20em;"></div> > > > > <div style="width:20em; background:blue;"> > > > > <span id="elm"></span><br> > > > > <br style="clear:both"> > > > > </div> > > > > <script> > > > > onload = function() { > > > > document.body.offsetTop; > > > > document.getElementById('elm').style.display = 'none'; > > > > } > > > > </script> > > > > > > I could always see a blue square on ToT. That's an another issue already > fixed > > I > > > guess? > > > > Indeed, I tested with an older build. How about this, then: > > > > <!DOCTYPE html> > > <p>There should be a blue <em>square</em> below.</p> > > <div style="float:right; width:1em; height:20em;"></div> > > <div style="width:20em; background:blue;"> > > <span id="elm"> </span><br> > > <br style="clear:both"> > > </div> > > <script> > > onclick = function() { > > document.body.offsetTop; > > document.getElementById('elm').style.fontSize = '0.5em'; > > } > > </script> > > Same. But if I click in somewhere of the test page, the bug happens. Sorry, it should say onload, not onclick. :)
On 2015/04/27 10:49:37, mstensho wrote: > On 2015/04/27 10:47:46, changseok wrote: > > On 2015/04/27 10:38:01, mstensho wrote: > > > On 2015/04/27 10:11:35, changseok wrote: > > > > On 2015/04/27 08:16:09, mstensho wrote: > > > > > On 2015/04/27 05:06:03, changseok wrote: > > > > > > On 2015/04/25 17:31:14, mstensho wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > > > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > > > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > > > > > > > document.querySelector('a'); > > > > > > > This test is flaky on my computer (without your fix it fails most of > > the > > > > > time, > > > > > > > but not always). I don't know why a focus / blur requires a relayout > > > > anyway. > > > > > > > Maybe you should make some explicit style modification on some node > to > > > > > trigger > > > > > > > the buggy relayout instead? And put a "document.body.offsetTop;" in > > > front > > > > of > > > > > > the > > > > > > > style modification code, to make sure that you get separate layout > > > passes? > > > > > > > > > > > > I could not reproduce the bug with any style changes. > > > > > > > > > > It's so easy. Try this: > > > > > > > > > > <!DOCTYPE html> > > > > > <p>There should be a blue <em>square</em> below.</p> > > > > > <div style="float:right; width:1em; height:20em;"></div> > > > > > <div style="width:20em; background:blue;"> > > > > > <span id="elm"></span><br> > > > > > <br style="clear:both"> > > > > > </div> > > > > > <script> > > > > > onload = function() { > > > > > document.body.offsetTop; > > > > > document.getElementById('elm').style.display = 'none'; > > > > > } > > > > > </script> > > > > > > > > I could always see a blue square on ToT. That's an another issue already > > fixed > > > I > > > > guess? > > > > > > Indeed, I tested with an older build. How about this, then: > > > > > > <!DOCTYPE html> > > > <p>There should be a blue <em>square</em> below.</p> > > > <div style="float:right; width:1em; height:20em;"></div> > > > <div style="width:20em; background:blue;"> > > > <span id="elm"> </span><br> > > > <br style="clear:both"> > > > </div> > > > <script> > > > onclick = function() { > > > document.body.offsetTop; > > > document.getElementById('elm').style.fontSize = '0.5em'; > > > } > > > </script> > > > > Same. But if I click in somewhere of the test page, the bug happens. > > Sorry, it should say onload, not onclick. :) No problem, I guess so. =) BTW, do you want me to swap the test with yours? Or just want to see if the bug is fixed in your test as well?
On 2015/04/27 10:58:55, changseok wrote: > On 2015/04/27 10:49:37, mstensho wrote: > > On 2015/04/27 10:47:46, changseok wrote: > > > On 2015/04/27 10:38:01, mstensho wrote: > > > > On 2015/04/27 10:11:35, changseok wrote: > > > > > On 2015/04/27 08:16:09, mstensho wrote: > > > > > > On 2015/04/27 05:06:03, changseok wrote: > > > > > > > On 2015/04/25 17:31:14, mstensho wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > > > > File LayoutTests/fast/block/float/br-with-clear-3.html (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1102953002/diff/1/LayoutTests/fast/block/floa... > > > > > > > > LayoutTests/fast/block/float/br-with-clear-3.html:22: var anchor = > > > > > > > > document.querySelector('a'); > > > > > > > > This test is flaky on my computer (without your fix it fails most > of > > > the > > > > > > time, > > > > > > > > but not always). I don't know why a focus / blur requires a > relayout > > > > > anyway. > > > > > > > > Maybe you should make some explicit style modification on some > node > > to > > > > > > trigger > > > > > > > > the buggy relayout instead? And put a "document.body.offsetTop;" > in > > > > front > > > > > of > > > > > > > the > > > > > > > > style modification code, to make sure that you get separate layout > > > > passes? > > > > > > > > > > > > > > I could not reproduce the bug with any style changes. > > > > > > > > > > > > It's so easy. Try this: > > > > > > > > > > > > <!DOCTYPE html> > > > > > > <p>There should be a blue <em>square</em> below.</p> > > > > > > <div style="float:right; width:1em; height:20em;"></div> > > > > > > <div style="width:20em; background:blue;"> > > > > > > <span id="elm"></span><br> > > > > > > <br style="clear:both"> > > > > > > </div> > > > > > > <script> > > > > > > onload = function() { > > > > > > document.body.offsetTop; > > > > > > document.getElementById('elm').style.display = 'none'; > > > > > > } > > > > > > </script> > > > > > > > > > > I could always see a blue square on ToT. That's an another issue already > > > fixed > > > > I > > > > > guess? > > > > > > > > Indeed, I tested with an older build. How about this, then: > > > > > > > > <!DOCTYPE html> > > > > <p>There should be a blue <em>square</em> below.</p> > > > > <div style="float:right; width:1em; height:20em;"></div> > > > > <div style="width:20em; background:blue;"> > > > > <span id="elm"> </span><br> > > > > <br style="clear:both"> > > > > </div> > > > > <script> > > > > onclick = function() { > > > > document.body.offsetTop; > > > > document.getElementById('elm').style.fontSize = '0.5em'; > > > > } > > > > </script> > > > > > > Same. But if I click in somewhere of the test page, the bug happens. > > > > Sorry, it should say onload, not onclick. :) > > No problem, I guess so. =) > BTW, do you want me to swap the test with yours? Or just want to see if the bug > is fixed in your test as well? I'd like you to write a test that modifies the style explicitly, rather than depending on focus()/blur() triggering relayout. You could modify your current test to do that, or steal my test. Whatever you prefer. Keeping your current test in addition to a new one that modifies the style explicitly is also an option.
On 2015/04/27 11:04:05, mstensho wrote: > I'd like you to write a test that modifies the style explicitly, rather than > depending on focus()/blur() triggering relayout. You could modify your current > test to do that, or steal my test. Whatever you prefer. Keeping your current > test in addition to a new one that modifies the style explicitly is also an > option. O.K Updated.
On 2015/04/27 11:26:33, changseok wrote: > On 2015/04/27 11:04:05, mstensho wrote: > > I'd like you to write a test that modifies the style explicitly, rather than > > depending on focus()/blur() triggering relayout. You could modify your current > > test to do that, or steal my test. Whatever you prefer. Keeping your current > > test in addition to a new one that modifies the style explicitly is also an > > option. > > O.K Updated. Test looks fine now. I don't feel like reviewing the actual code changes, though, as I'm not super-familiar with this code, and also because leviw / esprehn have already commented. Please continue the discussion with them to get a green light. :)
This still isn't quite right. If I change your yellow box to have these contents: <p class='yellow'> <a href='#'>Anchor</a><br> Text line. <br style="clear:both"><span></span> </p> It will continue to fail with your patch.
On 2015/04/27 18:41:54, leviw wrote: > This still isn't quite right. > > If I change your yellow box to have these contents: > > <p class='yellow'> > <a href='#'>Anchor</a><br> > Text line. > <br style="clear:both"><span></span> > </p> > > It will continue to fail with your patch. For that case, we should not limit the check of br with clear to only last child? How about this? When layouting a block flow, we check if the block contains a br with clear style among children, and keep the info in LineLayoutState. This info could be used when deciding checkForEndLineMatch. In any case, when a block ends with an invisible line and contains float, we need to look lines to the end of block.
comment please?
On 2015/05/04 at 12:30:21, shivamidow wrote: > comment please? Seems reasonable.
On 2015/05/04 20:14:15, leviw wrote: > On 2015/05/04 at 12:30:21, shivamidow wrote: > > comment please? > > Seems reasonable. Then.. can we land this if no other concern? ''a
https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1568: if (o->isBR() && (o->style()->clear() != CNONE)) remove parens https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1864: if ((resolver.status() != endLineStatus) || (containsFloats() && layoutState.containsClearBR())) remove extra parens on the first expr.
https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... File LayoutTests/fast/block/float/br-with-clear-3.html (right): https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... LayoutTests/fast/block/float/br-with-clear-3.html:21: window.onload = function() { I don't suspect the onload handler is needed here since your script is at the end. https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... File LayoutTests/fast/block/float/br-with-clear-4.html (right): https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... LayoutTests/fast/block/float/br-with-clear-4.html:18: <br style="clear:both"><span></span> I think it'd be nice if you combined these test cases into one that used check-layout.js instead of ref tests, but I won't block it on that. https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:748: Remove unnecessary empty line. https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/lin... File Source/core/layout/line/LineLayoutState.h (right): https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/lin... Source/core/layout/line/LineLayoutState.h:80: bool containsClearBR() const { return m_containsClearBR; } Nit: I'd call this "containsBRWithClear" or "containsBRWithClearStyle"
Thanks for all comments! https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... File LayoutTests/fast/block/float/br-with-clear-3.html (right): https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... LayoutTests/fast/block/float/br-with-clear-3.html:21: window.onload = function() { On 2015/05/11 00:25:29, leviw wrote: > I don't suspect the onload handler is needed here since your script is at the > end. You're right. Removed onload. https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... File LayoutTests/fast/block/float/br-with-clear-4.html (right): https://codereview.chromium.org/1102953002/diff/100001/LayoutTests/fast/block... LayoutTests/fast/block/float/br-with-clear-4.html:18: <br style="clear:both"><span></span> On 2015/05/11 00:25:29, leviw wrote: > I think it'd be nice if you combined these test cases into one that used > check-layout.js instead of ref tests, but I won't block it on that. I didn't know the check-layout.js. Let me try to use it next time. thanks ;) https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:748: On 2015/05/11 00:25:29, leviw wrote: > Remove unnecessary empty line. Done. https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1568: if (o->isBR() && (o->style()->clear() != CNONE)) On 2015/05/10 18:23:35, esprehn wrote: > remove parens Done. https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1864: if ((resolver.status() != endLineStatus) || (containsFloats() && layoutState.containsClearBR())) On 2015/05/10 18:23:35, esprehn wrote: > remove extra parens on the first expr. Done. https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/lin... File Source/core/layout/line/LineLayoutState.h (right): https://codereview.chromium.org/1102953002/diff/100001/Source/core/layout/lin... Source/core/layout/line/LineLayoutState.h:80: bool containsClearBR() const { return m_containsClearBR; } On 2015/05/11 00:25:29, leviw wrote: > Nit: I'd call this "containsBRWithClear" or "containsBRWithClearStyle" containsBRWithClear sounds good to me. =)
More comments?
https://codereview.chromium.org/1102953002/diff/120001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/120001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1566: if (o->isBR() && o->style()->clear() != CNONE) It's sad that we're doing this whenever there's any br with a clear, not only if they're in the end of the line. That said, I think this patch is unintrusive enough as to be okay. https://codereview.chromium.org/1102953002/diff/120001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1862: if (resolver.status() != endLineStatus || (containsFloats() && layoutState.containsBRWithClear())) I think I'd prefer the following: if (resolver.status() != endLineStatus) return false; // Comment about why we're doing this if (layoutState.containsBRWithClear() && containsFloats()) return false; You'll notice I reversed the order of these because I suspect containing br's with clear is less common than floats.
https://codereview.chromium.org/1102953002/diff/120001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/120001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1566: if (o->isBR() && o->style()->clear() != CNONE) On 2015/05/14 02:22:19, leviw wrote: > It's sad that we're doing this whenever there's any br with a clear, not only if > they're in the end of the line. > > That said, I think this patch is unintrusive enough as to be okay. Yeah.. Do you have any better idea to reduce iteration? I always welcome a neat idea. =) https://codereview.chromium.org/1102953002/diff/120001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1862: if (resolver.status() != endLineStatus || (containsFloats() && layoutState.containsBRWithClear())) On 2015/05/14 02:22:19, leviw wrote: > I think I'd prefer the following: > > if (resolver.status() != endLineStatus) > return false; > > // Comment about why we're doing this > if (layoutState.containsBRWithClear() && containsFloats()) > return false; > > You'll notice I reversed the order of these because I suspect containing br's > with clear is less common than floats. Make sense.
Still not looks good to you? I'd like to make this go forward. Feel free to leave concerns, suggestion or whatever if any. That would be much helpful to polish the cl.
https://codereview.chromium.org/1102953002/diff/160001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/160001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1866: // contains floats. It might mislead height of the block into being shorter than expected. Can we word this better? Specifically mention the bug instead of saying "might" and "be careful"
https://codereview.chromium.org/1102953002/diff/160001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/160001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1866: // contains floats. It might mislead height of the block into being shorter than expected. On 2015/05/19 21:29:52, leviw wrote: > Can we word this better? > > Specifically mention the bug instead of saying "might" and "be careful" Updated.
https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1866: // It misleads shorter height of the block than expected. We should check until the end line "It misleads shorter height of the block than expected" doesn't make sense :-/
https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1866: // It misleads shorter height of the block than expected. We should check until the end line On 2015/05/20 21:22:29, leviw wrote: > "It misleads shorter height of the block than expected" doesn't make sense :-/ Hrm.. what does not make sense to you? If you could point it out a little bit more in depth, it would be much helpful. Not sure of your thought though.. then how about this? "For partial layout where the block contains floats, the end line here is the last visible one. Due to calculating only these visible lines for the block height, invisible br having clear style is ignored so that it causes shorter height of the block than expected. We should check until the end line of the block for the case."
https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1866: // It misleads shorter height of the block than expected. We should check until the end line On 2015/05/21 at 09:32:40, changseok wrote: > On 2015/05/20 21:22:29, leviw wrote: > > "It misleads shorter height of the block than expected" doesn't make sense :-/ > > Hrm.. what does not make sense to you? If you could point it out a little bit more in depth, it would be much helpful. It's not proper English... "shorter height of the block" is not a sensible direct object for misleads... > > Not sure of your thought though.. then how about this? > "For partial layout where the block contains floats, the end line here is the last visible one." Last visible float? > ..."Due to calculating only these visible lines for the block height, invisible br having clear style is ignored so that it causes shorter height of the block than expected. We should check until the end line of the block for the case." What do you mean "visible" here? I think what you're trying to say is that a trailing BR can be collapsed as it usually doesn't contribute to the height, but if it has a clear style we need to force it to layout in the context of floats to honor that style.
https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... File Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... Source/core/layout/LayoutBlockFlowLine.cpp:1866: // It misleads shorter height of the block than expected. We should check until the end line On 2015/05/21 17:49:50, leviw wrote: > On 2015/05/21 at 09:32:40, changseok wrote: > > On 2015/05/20 21:22:29, leviw wrote: > > > "It misleads shorter height of the block than expected" doesn't make sense > :-/ > > > > Hrm.. what does not make sense to you? If you could point it out a little bit > more in depth, it would be much helpful. > > It's not proper English... "shorter height of the block" is not a sensible > direct object for misleads... > Oops, English is not my mother language so I used to make this kind of errors. Sorry about that. :P > > Not sure of your thought though.. then how about this? > > "For partial layout where the block contains floats, the end line here is the > last visible one." > Last visible float? > I meant last visible line. > > ..."Due to calculating only these visible lines for the block height, > invisible br having clear style is ignored so that it causes shorter height of > the block than expected. We should check until the end line of the block for the > case." > > What do you mean "visible" here? > What I meant for visible was something to have actual contents so it takes a visual region. For example, empty span, <span></span> is not visible, but <span>text</span> is visible. > I think what you're trying to say is that a trailing BR can be collapsed as it > usually doesn't contribute to the height, but if it has a clear style we need to > force it to layout in the context of floats to honor that style. Yeah. same context, but your summary is neater. May I just use yours?
On 2015/05/22 at 08:47:36, shivamidow wrote: > https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... > File Source/core/layout/LayoutBlockFlowLine.cpp (right): > > https://codereview.chromium.org/1102953002/diff/180001/Source/core/layout/Lay... > Source/core/layout/LayoutBlockFlowLine.cpp:1866: // It misleads shorter height of the block than expected. We should check until the end line > On 2015/05/21 17:49:50, leviw wrote: > > On 2015/05/21 at 09:32:40, changseok wrote: > > > On 2015/05/20 21:22:29, leviw wrote: > > > > "It misleads shorter height of the block than expected" doesn't make sense > > :-/ > > > > > > Hrm.. what does not make sense to you? If you could point it out a little bit > > more in depth, it would be much helpful. > > > > It's not proper English... "shorter height of the block" is not a sensible > > direct object for misleads... > > > Oops, English is not my mother language so I used to make this kind of errors. Sorry about that. :P No worries. > > > > Not sure of your thought though.. then how about this? > > > "For partial layout where the block contains floats, the end line here is the > > last visible one." > > Last visible float? > > > I meant last visible line. Visible is an overloaded term in CSS. > > > ..."Due to calculating only these visible lines for the block height, > > invisible br having clear style is ignored so that it causes shorter height of > > the block than expected. We should check until the end line of the block for the > > case." > > > > What do you mean "visible" here? > > > What I meant for visible was something to have actual contents so it takes a visual region. For example, empty span, <span></span> is not visible, but <span>text</span> is visible. See above comment. > > > I think what you're trying to say is that a trailing BR can be collapsed as it > > usually doesn't contribute to the height, but if it has a clear style we need to > > force it to layout in the context of floats to honor that style. > > Yeah. same context, but your summary is neater. May I just use yours? Of course :)
On 2015/05/22 20:16:44, leviw wrote: > > Yeah. same context, but your summary is neater. May I just use yours? > > Of course :) Thanks! Updated.
Description nits: "If br with clear places as an end line, it is ignored while relayouting." If _a_ br with _a_ clear _style is placed on the end of a line it is collapsed and subsequently not taken into account when matching the ends of lines_ "target only something visible" Again, visible is an overloaded term and doesn't usually mean what you want it to mean.
On 2015/05/22 21:05:14, leviw wrote: > Description nits: > > "If br with clear places as an end line, it is ignored while relayouting." > If _a_ br with _a_ clear _style is placed on the end of a line it is collapsed > and subsequently not taken into account when matching the ends of lines_ > > "target only something visible" > Again, visible is an overloaded term and doesn't usually mean what you want it > to mean. I see. Thanks again for your comments. =) I just updated the description.
Would you see this again? Does this not look good to you, owners?
Okay. LGTM.
The CQ bit was checked by shivamidow@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1102953002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195993 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
