Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(60)

Issue 2640153002: [nglayout] Fixed position with left:auto (Closed)

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.

Description

Fixed position with left:auto was broken because element's static position was not propagated to old layout. computeLogicalWidth(computedValues) eventually uses layer()->getStaticInlinePosition for positioning if left or top are not set. We were not saving this value. I fixed it by saving the value inside the builder. The reason for using builder is code reuse among different algorithms. The fix makes 3 more tests pass. Example: <p>Yo</p> <div id="div1" style="position:fixed:width:9px;height:9px;background-color:yellow" ></div> BUG=635619 [ng_save_static] Review-Url: https://codereview.chromium.org/2640153002 Cr-Commit-Position: refs/heads/master@{#444577} Committed: https://chromium.googlesource.com/chromium/src/+/846a1b9762d89d23f293b0492df433e2239f71f5

Patch Set 1 #

Total comments: 2

Patch Set 2 : LegacySaveStaticOffset -> SaveStaticOffsetForLegacy #

Total comments: 3

Patch Set 3 : conflict rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -5 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 2 chunks +2 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_node.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_block_node.cc View 1 2 2 chunks +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 22 (12 generated)
atotic
ptal
3 years, 11 months ago (2017-01-18 21:02:48 UTC) #3
cbiesinger
lgtm https://codereview.chromium.org/2640153002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_node.h File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right): https://codereview.chromium.org/2640153002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_node.h#newcode70 third_party/WebKit/Source/core/layout/ng/ng_block_node.h:70: void LegacySaveStaticOffset(const NGLogicalOffset&); I think this would read ...
3 years, 11 months ago (2017-01-18 21:09:24 UTC) #5
ikilpatrick
https://codereview.chromium.org/2640153002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2640153002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode79 third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:79: child->SaveStaticOffsetForLegacy(child_offset); can this go inside the algorithm instead? I'd ...
3 years, 11 months ago (2017-01-18 21:36:12 UTC) #7
atotic
https://codereview.chromium.org/2640153002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_node.h File third_party/WebKit/Source/core/layout/ng/ng_block_node.h (right): https://codereview.chromium.org/2640153002/diff/1/third_party/WebKit/Source/core/layout/ng/ng_block_node.h#newcode70 third_party/WebKit/Source/core/layout/ng/ng_block_node.h:70: void LegacySaveStaticOffset(const NGLogicalOffset&); On 2017/01/18 at 21:09:24, cbiesinger wrote: ...
3 years, 11 months ago (2017-01-18 21:38:48 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2640153002/20001
3 years, 11 months ago (2017-01-18 21:39:42 UTC) #12
atotic
https://codereview.chromium.org/2640153002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right): https://codereview.chromium.org/2640153002/diff/20001/third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc#newcode79 third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:79: child->SaveStaticOffsetForLegacy(child_offset); On 2017/01/18 at 21:36:12, ikilpatrick wrote: > can ...
3 years, 11 months ago (2017-01-18 21:42:29 UTC) #13
commit-bot: I haz the power
Failed to apply patch for third_party/WebKit/Source/core/layout/ng/ng_block_node.h: While running git apply --index -p1; error: patch failed: ...
3 years, 11 months ago (2017-01-18 23:13:18 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2640153002/40001
3 years, 11 months ago (2017-01-18 23:31:28 UTC) #18
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/846a1b9762d89d23f293b0492df433e2239f71f5
3 years, 11 months ago (2017-01-19 00:59:22 UTC) #21
atotic
3 years, 11 months ago (2017-01-20 18:09:27 UTC) #22
Message was sent while issue was closed.
https://codereview.chromium.org/2640153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc (right):

https://codereview.chromium.org/2640153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_fragment_builder.cc:79:
child->SaveStaticOffsetForLegacy(child_offset);
On 2017/01/18 at 21:42:28, atotic wrote:
> On 2017/01/18 at 21:36:12, ikilpatrick wrote:
> > can this go inside the algorithm instead? I'd prefer we keep all of the
NG->Legacy copying inside that file.
> 
> cons of doing it in the algorithm is that all algorithms would have to do it,
and it is an
> easy thing to overlook. I could add a comment to docs for
AddOutOfFlowChildCandidate
> to "always call SaveStaticOffsetForLegacy before this", but to me it is just a
bug waiting to
> happen.

Talked to Ian about this. Decided to investigate pulling lots of OOF logic out
of the builder. It will be more feasible once the state machines are out.

It looks like new Float and Margin code is following the same pattern
of using the builder for some logic.

This code is inside the builder because we'd like to reuse it in different
algorithms. A good time to take another look for best way to refactor would be
when we start implementation of the next layout algorithm (flex?)

Powered by Google App Engine
This is Rietveld 408576698