Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798203002:
NGInlineLayoutAlgorithm should take margins of inline floats into account (Closed)
|
Created:
3 years, 8 months ago by Gleb Lanbin Modified:
3 years, 8 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, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNGInlineLayoutAlgorithm should take margins of inline floats into account
1) Moved ComputeMarginsForChild to NGLayoutAlgorithm
2) Changed InlineLayoutAlgorithmTest to call ComputeMarginsForChild
and pass them to NGFloatingObject
BUG=635619
TEST=NGInlineLayoutAlgorithmTest::PositionFloatsWithMargins
Review-Url: https://codereview.chromium.org/2798203002
Cr-Commit-Position: refs/heads/master@{#462644}
Committed: https://chromium.googlesource.com/chromium/src/+/d3920ccc81711676a41982bcae3a29b4142c6838
Patch Set 1 #
Total comments: 8
Patch Set 2 : fix comments #
Total comments: 1
Patch Set 3 : use parent's writing mode, direction while calculating margins #
Messages
Total messages: 30 (19 generated)
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
glebl@chromium.org changed reviewers: + ikilpatrick@chromium.org
Description was changed from ========== Make InlineLayoutAlgorithmTest calculate margins of inline floats 1) Move ComputeMarginsForChild to NGLayoutAlgorithm 2) Change InlineLayoutAlgorithmTest to call ComputeMarginsForChild and set pass them to NGFloatingObject BUG=635619 TEST=NGInlineLayoutAlgorithmTest::PositionFloatsWithMargins ========== to ========== Make InlineLayoutAlgorithmTest calculate margins of inline floats 1) Move ComputeMarginsForChild to NGLayoutAlgorithm 2) Change InlineLayoutAlgorithmTest to call ComputeMarginsForChild and set pass them to NGFloatingObject BUG=635619 TEST=NGInlineLayoutAlgorithmTest::PositionFloatsWithMargins ==========
glebl@chromium.org changed reviewers: + kojii@chromium.org
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.
Description was changed from ========== Make InlineLayoutAlgorithmTest calculate margins of inline floats 1) Move ComputeMarginsForChild to NGLayoutAlgorithm 2) Change InlineLayoutAlgorithmTest to call ComputeMarginsForChild and set pass them to NGFloatingObject BUG=635619 TEST=NGInlineLayoutAlgorithmTest::PositionFloatsWithMargins ========== to ========== NGInlineLayoutAlgorithm should take margins of inline floats into account 1) Moved ComputeMarginsForChild to NGLayoutAlgorithm 2) Changed InlineLayoutAlgorithmTest to call ComputeMarginsForChild and pass them to NGFloatingObject BUG=635619 TEST=NGInlineLayoutAlgorithmTest::PositionFloatsWithMargins ==========
Thank you, sharing more code between inline and block is great. https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc (right): https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc:15: template <typename NGInputNodeType, typename NGBreakTokenType> I'm probably missing something but can you explain why we use template, and where NGBreakTokenType is used? https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc:21: const ComputedStyle& child_style = child->Style(); This doesn't look right to me...NGInlineNode::Style() is not a style for the wrapper box it produces. I think you really want is: child->IsBlock() ? child->Style() : child->ContainerBlockNode()->Style(); (though we don't have ContainerBlockNode() today) AFAIU, adding parent link in NGLayoutInputNode is fine, but I'd like to ask that to Ian.
https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc:334: NGBoxStrut margins = ComputeMarginsForChild(*float_space.get(), node); So I think this should actually be just: NGBoxStrut margins = ComputeMargins(*float_space.get(), node->Style(), wm, dir); We never need to apply auto margins for floats, so the above is fine. And we don't want the rest of the patch I believe, see other comment. https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc (right): https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc:17: NGLayoutAlgorithm<NGInputNodeType, NGBreakTokenType>::ComputeMarginsForChild( So I don't think we want to pull this down into the base class - The only special thing this does is applying auto margins, which is only applicable to block-level block flow layout, (i.e. BlockLayoutAlgorithm). If we pull this down here, I worry that as soon as someone implements grid using this they'll place a: if (isGrid()) { // do something else with margins } And we'll be back to the confusing legacy codebase :). Better to rely on composition here for the moment.
thanks, PTAL. https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc:334: NGBoxStrut margins = ComputeMarginsForChild(*float_space.get(), node); On 2017/04/06 16:24:31, ikilpatrick wrote: > So I think this should actually be just: > > NGBoxStrut margins = ComputeMargins(*float_space.get(), node->Style(), wm, dir); > > We never need to apply auto margins for floats, so the above is fine. > > And we don't want the rest of the patch I believe, see other comment. Done. https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc (right): https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc:15: template <typename NGInputNodeType, typename NGBreakTokenType> On 2017/04/06 13:44:52, kojii wrote: > I'm probably missing something but can you explain why we use template, and > where NGBreakTokenType is used? Acknowledged. https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc:17: NGLayoutAlgorithm<NGInputNodeType, NGBreakTokenType>::ComputeMarginsForChild( On 2017/04/06 16:24:31, ikilpatrick wrote: > So I don't think we want to pull this down into the base class - The only > special thing this does is applying auto margins, which is only applicable to > block-level block flow layout, (i.e. BlockLayoutAlgorithm). > > If we pull this down here, I worry that as soon as someone implements grid using > this they'll place a: > if (isGrid()) { > // do something else with margins > } > > And we'll be back to the confusing legacy codebase :). > > Better to rely on composition here for the moment. Acknowledged. https://codereview.chromium.org/2798203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_layout_algorithm.cc:21: const ComputedStyle& child_style = child->Style(); On 2017/04/06 13:44:52, kojii wrote: > This doesn't look right to me...NGInlineNode::Style() is not a style for the > wrapper box it produces. I think you really want is: > child->IsBlock() ? child->Style() : child->ContainerBlockNode()->Style(); > (though we don't have ContainerBlockNode() today) > > AFAIU, adding parent link in NGLayoutInputNode is fine, but I'd like to ask that > to Ian. Acknowledged.
The CQ bit was checked by glebl@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...
https://codereview.chromium.org/2798203002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc (right): https://codereview.chromium.org/2798203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc:336: ComputeMargins(*float_space, float_style, float_space->WritingMode(), .nit (can fix later) I'm not sure why we passed writing-mode, and direction into computemargins as it always should use the parents writing mode and direction; it probably makes sense to remove these arguments. (there is a subtle bug here as if the writing modes are orthogonal, you'll end up with bad margins). You'll need to give it a space which is in the parents writing mode to resolve against the correct inline size.
lgtm if you want to fix this later...
On 2017/04/06 17:44:21, ikilpatrick wrote: > https://codereview.chromium.org/2798203002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc > (right): > > https://codereview.chromium.org/2798203002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc:336: > ComputeMargins(*float_space, float_style, float_space->WritingMode(), > .nit (can fix later) > > I'm not sure why we passed writing-mode, and direction into computemargins as it > always should use the parents writing mode and direction; it probably makes > sense to remove these arguments. > > (there is a subtle bug here as if the writing modes are orthogonal, you'll end > up with bad margins). You'll need to give it a space which is in the parents > writing mode to resolve against the correct inline size. ack, I will send a follow-up patch to remove them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
On 2017/04/06 17:51:59, Gleb Lanbin wrote: > On 2017/04/06 17:44:21, ikilpatrick wrote: > > > https://codereview.chromium.org/2798203002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc > > (right): > > > > > https://codereview.chromium.org/2798203002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/ng/ng_inline_layout_algorithm.cc:336: > > ComputeMargins(*float_space, float_style, float_space->WritingMode(), > > .nit (can fix later) > > > > I'm not sure why we passed writing-mode, and direction into computemargins as > it > > always should use the parents writing mode and direction; it probably makes > > sense to remove these arguments. > > > > (there is a subtle bug here as if the writing modes are orthogonal, you'll end > > up with bad margins). You'll need to give it a space which is in the parents > > writing mode to resolve against the correct inline size. > > ack, I will send a follow-up patch to remove them. switched to use parent's writing_mode, direction
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by glebl@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491515418993120,
"parent_rev": "5a2389fc0abfbdcb4045acd27d62b7d43270da43", "commit_rev":
"d3920ccc81711676a41982bcae3a29b4142c6838"}
Message was sent while issue was closed.
Description was changed from ========== NGInlineLayoutAlgorithm should take margins of inline floats into account 1) Moved ComputeMarginsForChild to NGLayoutAlgorithm 2) Changed InlineLayoutAlgorithmTest to call ComputeMarginsForChild and pass them to NGFloatingObject BUG=635619 TEST=NGInlineLayoutAlgorithmTest::PositionFloatsWithMargins ========== to ========== NGInlineLayoutAlgorithm should take margins of inline floats into account 1) Moved ComputeMarginsForChild to NGLayoutAlgorithm 2) Changed InlineLayoutAlgorithmTest to call ComputeMarginsForChild and pass them to NGFloatingObject BUG=635619 TEST=NGInlineLayoutAlgorithmTest::PositionFloatsWithMargins Review-Url: https://codereview.chromium.org/2798203002 Cr-Commit-Position: refs/heads/master@{#462644} Committed: https://chromium.googlesource.com/chromium/src/+/d3920ccc81711676a41982bcae3a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d3920ccc81711676a41982bcae3a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
