Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925273007:
[LayoutNG] A little bit of documentation on margin collapsing. (Closed)
|
Created:
3 years, 6 months ago by ikilpatrick Modified:
3 years, 6 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, 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] A little bit of documentation on margin collapsing.
BUG=635619
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Review-Url: https://codereview.chromium.org/2925273007
Cr-Commit-Position: refs/heads/master@{#479547}
Committed: https://chromium.googlesource.com/chromium/src/+/d0b8f4909e983ea6c5dd60c25216e6fdf3ccb39b
Patch Set 1 #
Total comments: 18
Patch Set 2 : address comments. #
Total comments: 9
Patch Set 3 : 80-col #Messages
Total messages: 20 (8 generated)
Description was changed from ========== [LayoutNG] A little bit of documentation on margin collapsing. BUG=635619 ========== to ========== [LayoutNG] A little bit of documentation on margin collapsing. BUG=635619 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
ianjkilpatrick@gmail.com changed reviewers: + cbiesinger@chromium.org, ianjkilpatrick@gmail.com, kojii@chromium.org
This is just a first pass, let me know where more details would add value.
atotic@chromium.org changed reviewers: + atotic@chromium.org
lgtm Nice work. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/BlockLayout.md (right): https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:3: This document can be viewed in formatted form [here](https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/layout/ng/FlowLayout.md). s/FlowLayout/BlockLayout https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:37: <!-- The top-level divs below are 10px apart --> It'd be clearer if there was only 1 way to get math correct. Right now, looking at it, I can't tell if top level divs are 10px apart because of: 1st margin-bottom: 10px 4th margin-bottom - 2nd margin bottom or something else. In examples like this, I like to use primes, it is harder to stumble upon incorrect interpretation that migth work. ie: <div style="margin-bottom: 3px"> <div style="margin-bottom: -5px"> <div style="margin-bottom: 7px">Hi</div> </div> </div> <div style="margin-top: 11px"> <div style="margin-top: -13px">there</div> </div> Now, I really need to know that the solution is: max (3, 7 ,11) + min (-5, -13) At least, I think that's what the solution is :)
lgtm, thank you for doing this! This detailed, written document helps me to think about where I need to understand better very much! A few questions inline. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/BlockLayout.md (right): https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:97: Fragment* Layout(LogicalOffset bfc_estimate, MarginStrut input_strut) { Is "bfc_estimate" the "offset to BFC without the last end margin strut which may need further collapsing", correct? https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:110: IIUC, up until here is common to all algorithms, and the "for" loop below is about laying out the content area, which may vary by algorithms, correct? And the content area algorithms can use curr_bfc_estimate only if it is no longer an estimate, it needs to figure out how to handle positioning children if otherwise? I understood this "otherwise" case is what we're trying to solve for inline, does this look correct? I think what I'm trying to understand is, why we have troubles to place floats while we can place line boxes. Is it because we try to place floats before we place line boxes? Maybe we stop putting floats into FragmentBuilder in line breaker but only keep as temporary, then do it in PlaceItems(), when we know more about line boxes? I'll play a bit with your CL. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:113: const auto* fragment = child.Layout(curr_strut); child.Layout(curr_bfc_estimate, curr_strut) ?
dgrogan@chromium.org changed reviewers: + dgrogan@chromium.org
https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/BlockLayout.md (right): https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:48: In the above example as there isn't **anything** separating the edges of two fragments the margins Could you add a parenthetical after **anything** that gives an example or two of what might be separating the edges? I think you mean just borders and padding but am not sure. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:72: A naïve algorithm for the above case would be to _bubble_ up margins. For example each fragment Which "above case" do you mean? I suspect the adjoining margins example. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:74: **adjoining** to its parent, you simply keep track of the margins by calling `Append` on the margin Could you say which of the 4 margin struts (block-start/block-end * parent/child) you call Append on and which margin strut you pass to that call? Perhaps it depends on the situation? https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:87: _child_ will position _itself_ within the BFC. If we did margin collapsing this way way we'd create "way way"
https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/BlockLayout.md (right): https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:3: This document can be viewed in formatted form [here](https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/layout/ng/FlowLayout.md). On 2017/06/10 00:26:51, atotic wrote: > s/FlowLayout/BlockLayout Done. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:37: <!-- The top-level divs below are 10px apart --> On 2017/06/10 00:26:51, atotic wrote: > It'd be clearer if there was only 1 way to get math correct. Right now, looking > at it, I can't tell if top level divs are 10px apart because of: > 1st margin-bottom: 10px > 4th margin-bottom - 2nd margin bottom > or something else. > In examples like this, I like to use primes, it is harder to stumble upon > incorrect interpretation that migth work. ie: > > <div style="margin-bottom: 3px"> > <div style="margin-bottom: -5px"> > <div style="margin-bottom: 7px">Hi</div> > </div> > </div> > <div style="margin-top: 11px"> > <div style="margin-top: -13px">there</div> > </div> > > Now, I really need to know that the solution is: > max (3, 7 ,11) + min (-5, -13) > > At least, I think that's what the solution is :) Done. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:48: In the above example as there isn't **anything** separating the edges of two fragments the margins On 2017/06/12 18:54:40, dgrogan wrote: > Could you add a parenthetical after **anything** that gives an example or two of > what might be separating the edges? I think you mean just borders and padding > but am not sure. Done. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:72: A naïve algorithm for the above case would be to _bubble_ up margins. For example each fragment On 2017/06/12 18:54:40, dgrogan wrote: > Which "above case" do you mean? I suspect the adjoining margins example. Done. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:74: **adjoining** to its parent, you simply keep track of the margins by calling `Append` on the margin On 2017/06/12 18:54:40, dgrogan wrote: > Could you say which of the 4 margin struts (block-start/block-end * > parent/child) you call Append on and which margin strut you pass to that call? > Perhaps it depends on the situation? Done? - added example code for bubbling. PTAL https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:87: _child_ will position _itself_ within the BFC. If we did margin collapsing this way way we'd create On 2017/06/12 18:54:40, dgrogan wrote: > "way way" Done. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:97: Fragment* Layout(LogicalOffset bfc_estimate, MarginStrut input_strut) { On 2017/06/10 05:12:13, kojii wrote: > Is "bfc_estimate" the "offset to BFC without the last end margin strut which may > need further collapsing", correct? Thats correct, another way of thinking about this is the position of the incoming margin strut. https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:110: On 2017/06/10 05:12:13, kojii wrote: > IIUC, up until here is common to all algorithms, and the "for" loop below is > about laying out the content area, which may vary by algorithms, correct? This is only for block and inline, but yeah. > And the content area algorithms can use curr_bfc_estimate only if it is no > longer an estimate, it needs to figure out how to handle positioning children if > otherwise? I understood this "otherwise" case is what we're trying to solve for > inline, does this look correct? As soon as you "collapse" the margin strut it is no longer an estimate. > I think what I'm trying to understand is, why we have troubles to place floats > while we can place line boxes. Is it because we try to place floats before we > place line boxes? Maybe we stop putting floats into FragmentBuilder in line > breaker but only keep as temporary, then do it in PlaceItems(), when we know > more about line boxes? I'll play a bit with your CL. ok https://codereview.chromium.org/2925273007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:113: const auto* fragment = child.Layout(curr_strut); On 2017/06/10 05:12:13, kojii wrote: > child.Layout(curr_bfc_estimate, curr_strut) ? Done.
Thanks for writing this up! Couple of comments: https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/BlockLayout.md (right): https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:11: A simple way to think about margin collapsing is that it takes the maximum margin between two Can we stay with 80 character lines? https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:32: The rule here is: `max(pos_margins) + min(neg_margins)`. This rule we'll refer to as the _margin Maybe also link to the CSS spec here (or, at the beginning of the section), since this is just a recap of the spec https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:100: This would be pretty simple - however it doesn't work. As we discussed in the floats section a The floats section that just consists of "TODO"? :-) https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:107: strut becomes an input as well as where the margin strut is currently positioned within the BFC. I think adding some more details about the bfc offset would be helpful. E.g. something like the following, though I'm sure you can do a better job than me: We can always calculate the position of a child within a parent (Fragment::offset), but we sometimes have to defer the calculation of the bfc offset -- we can only compute the bfc offset once we have a child with nonzero height, at which point we resolve all our pending bfc offsets.
All-around comment: I think this is a great start. Because of the missing bfc/floating parts, I still do not fully understand what is going on in block layout, so I am holding off on more meaningful comments.
https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/BlockLayout.md (right): https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:11: A simple way to think about margin collapsing is that it takes the maximum margin between two On 2017/06/13 16:45:25, cbiesinger wrote: > Can we stay with 80 character lines? Done. https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:32: The rule here is: `max(pos_margins) + min(neg_margins)`. This rule we'll refer to as the _margin On 2017/06/13 16:45:25, cbiesinger wrote: > Maybe also link to the CSS spec here (or, at the beginning of the section), > since this is just a recap of the spec Done. https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:100: This would be pretty simple - however it doesn't work. As we discussed in the floats section a On 2017/06/13 16:45:24, cbiesinger wrote: > The floats section that just consists of "TODO"? :-) Yup, was going to do this in the next patch. https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:107: strut becomes an input as well as where the margin strut is currently positioned within the BFC. On 2017/06/13 16:45:24, cbiesinger wrote: > I think adding some more details about the bfc offset would be helpful. E.g. > something like the following, though I'm sure you can do a better job than me: > We can always calculate the position of a child within a parent > (Fragment::offset), but we sometimes have to defer the calculation of the bfc > offset -- we can only compute the bfc offset once we have a child with nonzero > height, at which point we resolve all our pending bfc offsets. Yeah I was going to introduce the BFC offset concept in the floats section.
https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/BlockLayout.md (right): https://codereview.chromium.org/2925273007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/BlockLayout.md:107: strut becomes an input as well as where the margin strut is currently positioned within the BFC. On 2017/06/13 20:42:40, ikilpatrick wrote: > On 2017/06/13 16:45:24, cbiesinger wrote: > > I think adding some more details about the bfc offset would be helpful. E.g. > > something like the following, though I'm sure you can do a better job than me: > > We can always calculate the position of a child within a parent > > (Fragment::offset), but we sometimes have to defer the calculation of the bfc > > offset -- we can only compute the bfc offset once we have a child with nonzero > > height, at which point we resolve all our pending bfc offsets. > > Yeah I was going to introduce the BFC offset concept in the floats section. OK, I only mentioned it because your code below makes use of it; but that's fine.
lgtm
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atotic@chromium.org, kojii@chromium.org Link to the patchset: https://codereview.chromium.org/2925273007/#ps40001 (title: "80-col")
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": 1497479487512150,
"parent_rev": "e3873dfc2212de0b9b0784315b30b38ce0f46fbc", "commit_rev":
"d0b8f4909e983ea6c5dd60c25216e6fdf3ccb39b"}
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] A little bit of documentation on margin collapsing. BUG=635619 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [LayoutNG] A little bit of documentation on margin collapsing. BUG=635619 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng Review-Url: https://codereview.chromium.org/2925273007 Cr-Commit-Position: refs/heads/master@{#479547} Committed: https://chromium.googlesource.com/chromium/src/+/d0b8f4909e983ea6c5dd60c25216... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d0b8f4909e983ea6c5dd60c25216... |
