Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2955843002:
[LayoutNG] Make floats push line boxes down if no break opportunities can fit
|
Created:
3 years, 5 months ago by kojii Modified:
3 years, 5 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, lchoi+reviews_chromium.org, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Make floats push line boxes down if no break opportunities can fit
When no break opportunities can fit in a line that has floats and/or
exclusions, such line box should be pushed down to avoid them.
A simple example is "float: left; width: 100%". This patch supports both
100% case and other cases in slightly different code paths since
NGLayoutOpportunityIterator handles 100% case differently.
Rewind() has been used for next lines, but this is the first case where
NGLineBreaker rewinds and process the current line again. Initializing
states is moved to NextLine() from BreakLine() to make this possible.
This patch also includes fix to avoid processing the same floats twice.
This should have been an issue when Rewind() is used, but it was not
handled properly before. The fix isn't very clean though, this should be
revisited.
Refactoring suggested in Ian's review[1] is included. This groups states
related to current line to a struct.
[1] https://codereview.chromium.org/2931363002#msg30
BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Minor fix #Patch Set 4 : Rebase #Patch Set 5 : Rebase, cleanup #Patch Set 6 : Fixes and more cleanup #
Total comments: 6
Patch Set 7 : ikilpatrick review (add STACK_ALLOCATED) #Patch Set 8 : Rebase #
Total comments: 6
Messages
Total messages: 43 (30 generated)
Description was changed from ========== belowfloat BUG= ========== to ========== belowfloat BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== belowfloat BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [LayoutNG] Make floats push line boxes down if no break opportunities can fit When no break opportunities can fit in a line that has floats and/or exclusions, such line box should be pushed down to avoid them. A simple example is "float: left; width: 100%". This patch supports both 100% case and other cases in slightly different code paths since NGLayoutOpportunityIterator handles 100% case differently. Rewind() has been used for next lines, but this is the first case where NGLineBreaker rewinds and process the current line again. Initializing states is moved to NextLine() from BreakLine() to make this possible. This patch also includes fix to avoid processing the same floats twice. This should have been an issue when Rewind() is used, but it was not handled properly before. The fix isn't very clean though, this should be revisited. BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
kojii@chromium.org changed reviewers: + ikilpatrick@chromium.org
PTAL.
Patchset #3 (id:60001) has been deleted
NG bot test failures are ok, shape-outside tests pass today because of magic, and this patch fixes to fail. ...does this conflict with what you're working on? I can postpone if so.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/27 02:38:21, kojii wrote: > NG bot test failures are ok, shape-outside tests pass today because of magic, > and this patch fixes to fail. > > ...does this conflict with what you're working on? I can postpone if so. Yeah, do you mind if we hold off on this at the moment? I'll try and get most of it done within 2-3 weeks, (as long as you don't feel like this will hold you up?)
On 2017/06/27 at 20:33:22, ikilpatrick wrote: > On 2017/06/27 02:38:21, kojii wrote: > > NG bot test failures are ok, shape-outside tests pass today because of magic, > > and this patch fixes to fail. > > > > ...does this conflict with what you're working on? I can postpone if so. > > Yeah, do you mind if we hold off on this at the moment? I'll try and get most of it done within 2-3 weeks, (as long as you don't feel like this will hold you up?) np, thank you for working on floats refactoring, looking forward to it!!
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Description was changed from ========== [LayoutNG] Make floats push line boxes down if no break opportunities can fit When no break opportunities can fit in a line that has floats and/or exclusions, such line box should be pushed down to avoid them. A simple example is "float: left; width: 100%". This patch supports both 100% case and other cases in slightly different code paths since NGLayoutOpportunityIterator handles 100% case differently. Rewind() has been used for next lines, but this is the first case where NGLineBreaker rewinds and process the current line again. Initializing states is moved to NextLine() from BreakLine() to make this possible. This patch also includes fix to avoid processing the same floats twice. This should have been an issue when Rewind() is used, but it was not handled properly before. The fix isn't very clean though, this should be revisited. BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [LayoutNG] Make floats push line boxes down if no break opportunities can fit When no break opportunities can fit in a line that has floats and/or exclusions, such line box should be pushed down to avoid them. A simple example is "float: left; width: 100%". This patch supports both 100% case and other cases in slightly different code paths since NGLayoutOpportunityIterator handles 100% case differently. Rewind() has been used for next lines, but this is the first case where NGLineBreaker rewinds and process the current line again. Initializing states is moved to NextLine() from BreakLine() to make this possible. This patch also includes fix to avoid processing the same floats twice. This should have been an issue when Rewind() is used, but it was not handled properly before. The fix isn't very clean though, this should be revisited. Refactoring suggested in Ian's review[1] is included. This groups states related to current line to a struct. [1] https://codereview.chromium.org/2931363002#msg30 BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
Ian, is this now ready to review? Your last commit did not conflict at all, and this change fixes 4 more tests. ng_bot has some failures in shapes, but these tests should not pass without shapes support.
ianjkilpatrick@gmail.com changed reviewers: + ianjkilpatrick@gmail.com
https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc (right): https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:123: PrepareNextLine(line_info); Ah i see, I prefer stack allocating this things and passing them to each function that needs it, but up to you. I.e. LineData line_data(line_info); // Most of PrepareNextLine BreakLine(line_info, &line_data); Up to you, I just like being explicit about lifetimes, and what uses specific data. https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:642: if (line_.has_floats_or_exclusions) { What happens in the case of: <span id=line> <div id=float1></div>XXX<div id=float2></div>XXXXXXX </span> Where "XXXXXXX" will trigger overflow? https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.h (right): https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.h:49: struct LineData { STACK_ALLOCATED()?
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, CL updated and replies inline: https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc (right): https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:123: PrepareNextLine(line_info); Understood the point. For me, NGLineBreaker is a stateful context object already, in that case, I mildly prefer members. If the class is not stateful context object, I agree with you. Also in this case, we need (part of) previous LineData to setup a new line, so some of them need to stay as members. Let me go this way if you're ok. https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:642: if (line_.has_floats_or_exclusions) { Floats create a break opportunity, so only the 2nd "XXXXXXX" goes to the next line. If you don't have first "XXX"; i.e., <div id=float1></div><div id=float2></div>XXXXXXX Break opportunities created by floats are at the beginning of line, which are suppressed, so there's no break opportunities in this line. Test: http://jsbin.com/zinuzax/edit?html,output BTW, we don't handle break opportunities floats/OOF create correctly yet. I thought I once fixed this but my tests at that point were not enough, I'll re-work on it. https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.h (right): https://codereview.chromium.org/2955843002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.h:49: struct LineData { Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
word-break-all-wrap-with-floats.html may also be interesting. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css3... When with "word-break: break-all", break opportunities change. This test ensures we break or push correctly in that case too, using the correct break opportunities.
A gentle reminder, just in case you no longer watch Rietveld.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
So I'm more than fine with the "line_" changes if you just want to submit that. If you like a can have a play in the next couple of days for the float thing, I think we want to allow a linebox which just has a single float in it, instead of this behaviour? https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc (right): https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:204: line_.has_floats_or_exclusions = !iter.IsAtEnd(); this variable name is a little misleading for me, its really - has_floats_that_are_larger_than_available_inline_size ? https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:209: // such floats/exclusions. Set the line box location to it. so is this approaching it the wrong way? Wouldn't we want to create a linebox - just containing the single float, then enter into the line breaker again for the second line? Is that difficult to do? This matches what the first-line selector would do, e.g. <!DOCTYPE html> <style> ::first-line { color: blue; } body { width: 100px; } #float { width: 100%; float: left; height: 10px; background: red;} </style> <p><div id=f></div>This isn't blue.</p> https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:452: // When rewind occurs, an item may be handled multiple times. so, i don't think we want to be able to rewind past a float? I think the logic we want is: a) A float will only get placed on the current line if there is room on it. We should modify HandleFloat to position a float if it exceeds the inline size, if its the only thing on that line. ^ Should fix this bug? b) Therefore we should never enter into HandleOverflow after we've placed a float. (There are always break opportunities between floats right?) ^ We should add a DCHECK inside of HandleOverflow that we never go back past a float. c) This means that we can have a linebox with a single float inside of it (which seems ok, and matches first-line behaviour?)
Thank you for having a look, and creative ideas to solve things simpler. Replies inline: https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc (right): https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:204: line_.has_floats_or_exclusions = !iter.IsAtEnd(); On 2017/07/21 at 18:18:43, ianjkilpatrick wrote: > this variable name is a little misleading for me, its really - > > has_floats_that_are_larger_than_available_inline_size ? No, this flag is about whether it has at least one float/exclusion or not. It may be larger than available size or smaller. https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:209: // such floats/exclusions. Set the line box location to it. On 2017/07/21 at 18:18:43, ianjkilpatrick wrote: > so is this approaching it the wrong way? > > Wouldn't we want to create a linebox - just containing the single float, then enter into the line breaker again for the second line? > > Is that difficult to do? I'm fine either way, I don't see much differences, but doing so runs a bit of additional code. Any reasons to prefer that? > This matches what the first-line selector would do, e.g. > <!DOCTYPE html> > <style> > ::first-line { color: blue; } > body { width: 100px; } > #float { width: 100%; float: left; height: 10px; background: red;} > </style> > <p><div id=f></div>This isn't blue.</p> First-line is an interesting point, but it can probably be done independently from whether we create an empty line box or not. It looks like impls don't agree. http://jsbin.com/bozuhec/edit?html,output * Gecko doesn't consider the pushed line is first-line * Edge/Blink/WebKit consider the pushed line is fist-line I mildly prefer matching to Edge/Blink/WebKit since it's consistent with the other case in the tests above, but ok to match to Gecko if it's more preferrable. Filed https://github.com/w3c/csswg-drafts/issues/1644 https://codereview.chromium.org/2955843002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:452: // When rewind occurs, an item may be handled multiple times. On 2017/07/21 at 18:18:43, ianjkilpatrick wrote: > so, i don't think we want to be able to rewind past a float? I think the logic we want is: > > a) A float will only get placed on the current line if there is room on it. > We should modify HandleFloat to position a float if it exceeds the inline size, if its the only thing on that line. > ^ Should fix this bug? I'm not following this...we already don't position floats if the float doesn't fit, no? The case this CL is trying to fix is when floats fit, but the next word doesn't. > b) Therefore we should never enter into HandleOverflow after we've placed a float. > (There are always break opportunities between floats right?) > ^ We should add a DCHECK inside of HandleOverflow that we never go back past a float. Not common but logically it's still possible. Some tests: http://jsbin.com/fipovos/edit?html,output For the flexibility to improve the line breaker quality as we go, I'd like not to have "unrewindable objects", but this is probably a bit academic wish we may not appreciate in future, we can ignore this. > c) This means that we can have a linebox with a single float inside of it (which seems ok, and matches first-line behaviour?) As above, I'm ok to have such a linebox, but it doesn't solve all cases (and first-line behavior is questionable.)
p.s. About first-line, I found better way to explain; we have IsFirstFormattedLine in NGLineBreaker, we need to determine this before we line break because it can affect font choice and thus line break point. So whether to create a line box or not shouldn't affect IsFirstFormattedLine. Does this sound reasonable?
> So I'm more than fine with the "line_" changes if you just want to submit that. Let's split this part to avoid reviewing it multiple times then, and due to deprecation of Reitveld, let me move to Gerrit by splitting. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
