|
|
Created:
3 years, 11 months ago by atotic Modified:
3 years, 11 months ago Reviewers:
cbiesinger CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix wrong percent_resolution_size inside ConstraintSpace::CreateFromLayoutObject.
This caused RunOldLayout to use wrong PercentageResolutionSize
<main style="width:500px;height:150px;position:relative;border: solid black">
<div style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div>
</main>
This used to display at 1/2 width in NGLayout.
BUG=635619
[ng_constraint_width]
Review-Url: https://codereview.chromium.org/2616093004
Cr-Commit-Position: refs/heads/master@{#442730}
Committed: https://chromium.googlesource.com/chromium/src/+/67e0a1510f8a824b0e07c61484bc55c28c2f6c86
Patch Set 1 #
Total comments: 2
Patch Set 2 : Better fix #
Total comments: 2
Patch Set 3 : size->logical_size #
Total comments: 3
Messages
Total messages: 17 (6 generated)
Description was changed from ========== Fix wrong percentage on RunOldLayout BUG=635619 ========== to ========== Fix wrong percent_resolution_size inside ConstraintSpace::CreateFromLayoutObject. This caused RunOldLayout to use wrong PercentageResolutionSize <main style="width:500px;height:150px;position:relative;border: solid black"> <div style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div> </main> This used to display at 1/2 width in NGLayout. BUG=635619 [ng_constraint_width] ==========
atotic@chromium.org changed reviewers: + cbiesinger@chromium.org
ptal
https://codereview.chromium.org/2616093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2616093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:75: box.containingBlockLogicalHeightForContent(IncludeMarginBorderPadding)}; OK, a few questions: - Here you explicitly choose containingBlockLogicalHeightForContent instead of availableLogicalHeightForPercentageComputation for percentage resolution. Why? The latter is supposed to be the correct one and if it isn't we should fix that - Why is the available space supposed to be different from the percentage size here? - If this is in fact correct you should use ExcludeMarginBorderPadding right? I assume this is for the !fixed_block case.
ptal https://codereview.chromium.org/2616093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2616093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:75: box.containingBlockLogicalHeightForContent(IncludeMarginBorderPadding)}; On 2017/01/09 at 18:17:10, cbiesinger wrote: > OK, a few questions: > - Here you explicitly choose containingBlockLogicalHeightForContent instead of availableLogicalHeightForPercentageComputation for percentage resolution. Why? The latter is supposed to be the correct one and if it isn't we should fix that You are right. The real problem was that available_sizes were overridden by hasOverrideLogicalContentWidth afterwards. New fix just saves percentage_size before we override available_size with OverrideLogicalContentSize.
lgtm but see comments and nit below. I am not completely sure why this fixes your bug though. The width calculation for the block should just take the fixed inline size and never do actual percentage calculation: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... Can you investigate a bit more why this is necessary? (This is OK to submit anyway, because it will fix other bugs -- things like percentage padding calculations would be wrong) https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:56: available_logical_height}; While you're touching this, I'd also make this other change: NGLogicalSize available_size = percentage_size; then in the ifs change available_size.{inline,block}_size. I think that would be clear. It would also rename the ambiguously-named "size" variable, which is another plus :)
On 2017/01/10 at 19:45:03, cbiesinger wrote: > lgtm but see comments and nit below. > > I am not completely sure why this fixes your bug though. The width calculation for the block should just take the fixed inline size and never do actual percentage calculation: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... > > Can you investigate a bit more why this is necessary? > > (This is OK to submit anyway, because it will fix other bugs -- things like percentage padding calculations would be wrong) This is because NGBlockNode is RunOldLayout, and old code does take container size into account. Still investingating how this happens, there are other bugs.
https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2616093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:56: available_logical_height}; On 2017/01/10 at 19:45:03, cbiesinger wrote: > While you're touching this, I'd also make this other change: > > NGLogicalSize available_size = percentage_size; > then in the ifs change available_size.{inline,block}_size. I think that would be clear. It would also rename the ambiguously-named "size" variable, which is another plus :) done
The CQ bit was checked by atotic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2616093004/#ps40001 (title: "size->logical_size")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:74: NGLogicalSize logical_size = {available_logical_width, available_size is better. But I also meant if you could move this line above, right under percentage_size. Ah... I think I figured it out wrt the old codepath... two reasons: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... gets called before we check for overrideLogicalContentWidth. But also: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... We ignore the override width unless we're a flex item. Should probably change that.
just replying to a comment https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:74: NGLogicalSize logical_size = {available_logical_width, On 2017/01/10 at 22:00:50, cbiesinger wrote: > available_size is better. But I also meant if you could move this line above, right under percentage_size. logical_size can't go right under percentage_size, because available_logical_height gets recomputed in between those two statements. > Ah... I think I figured it out wrt the old codepath... two reasons: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > gets called before we check for overrideLogicalContentWidth. But also: > We ignore the override width unless we're a flex item. Should probably change that. I am shy when it comes to editing old code, unless I am explicitly fixing an existing bug. My knowledge of existing code paths is limitied, and I fear unexpected dependencies. Do you want to do old code work, or should I just follow your advice?
https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2616093004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:74: NGLogicalSize logical_size = {available_logical_width, On 2017/01/10 23:24:15, atotic wrote: > On 2017/01/10 at 22:00:50, cbiesinger wrote: > > available_size is better. But I also meant if you could move this line above, > right under percentage_size. > > logical_size can't go right under percentage_size, because > available_logical_height gets recomputed in between those two statements. I'll send a followup patch once this lands, I think that'll help in explaining what I mean and then we can discuss whether it's a good idea :-) > > Ah... I think I figured it out wrt the old codepath... two reasons: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > gets called before we check for overrideLogicalContentWidth. But also: > > We ignore the override width unless we're a flex item. Should probably change > that. > > I am shy when it comes to editing old code, unless I am explicitly fixing > an existing bug. My knowledge of existing code paths is limitied, > and I fear unexpected dependencies. > > Do you want to do old code work, or should I just follow your advice? I can do that tomorrow.
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484085201655740, "parent_rev": "9422f34b57066a40cea5b40c1e022adeb4712859", "commit_rev": "67e0a1510f8a824b0e07c61484bc55c28c2f6c86"}
Message was sent while issue was closed.
Description was changed from ========== Fix wrong percent_resolution_size inside ConstraintSpace::CreateFromLayoutObject. This caused RunOldLayout to use wrong PercentageResolutionSize <main style="width:500px;height:150px;position:relative;border: solid black"> <div style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div> </main> This used to display at 1/2 width in NGLayout. BUG=635619 [ng_constraint_width] ========== to ========== Fix wrong percent_resolution_size inside ConstraintSpace::CreateFromLayoutObject. This caused RunOldLayout to use wrong PercentageResolutionSize <main style="width:500px;height:150px;position:relative;border: solid black"> <div style="position:absolute;top:25%;width:50%;background-color:yellow;">hi</div> </main> This used to display at 1/2 width in NGLayout. BUG=635619 [ng_constraint_width] Review-Url: https://codereview.chromium.org/2616093004 Cr-Commit-Position: refs/heads/master@{#442730} Committed: https://chromium.googlesource.com/chromium/src/+/67e0a1510f8a824b0e07c61484bc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/67e0a1510f8a824b0e07c61484bc... |