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

Issue 1415623002: Need to reposition an out-of-flow object *before* re-paginating it. (Closed)

Created:
5 years, 2 months ago by mstensho (USE GERRIT)
Modified:
5 years, 2 months ago
CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, mstensho (USE GERRIT), pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Need to reposition an out-of-flow object *before* re-paginating it. https://codereview.chromium.org/1343163005 removed code that was actually needed. Instead of simply reverting the necessary parts, I rewrote them. The old code called updateLogicalHeight() before layout, when all it wanted to do was set the logical top. This made me a bit uneasy, because that could in theory prevent the height change from being detected during layout() (and LayoutBlockFlow::layoutBlockFlow() would for instance fail to set |relayoutChildren| before calling layoutPositionedObjects()). Furthermore, it used to check if the child establishes a writing mode root, and update the logical *width* instead of height in that case. That's unnecessary and potentially harmful (preventing the width change from being detected during layout()). If the child establishes a new writing mode, it becomes opaque as far as pagination is concerned, so no need to handle this. BUG=544783 R=leviw@chromium.org Committed: https://crrev.com/cd0c3a5c30540ae92f017e3c9f5309c450bf65ec Cr-Commit-Position: refs/heads/master@{#354815}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -0 lines) Patch
A third_party/WebKit/LayoutTests/fast/multicol/dynamic/bottom-aligned-abspos-change-column-height.html View 1 chunk +17 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/multicol/dynamic/bottom-aligned-abspos-change-column-height-expected.txt View 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlock.cpp View 3 chunks +20 lines, -0 lines 0 comments Download

Messages

Total messages: 6 (1 generated)
mstensho (USE GERRIT)
5 years, 2 months ago (2015-10-19 15:42:21 UTC) #1
leviw_travelin_and_unemployed
lgtm Makes sense. Thanks!
5 years, 2 months ago (2015-10-19 17:05:48 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415623002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415623002/1
5 years, 2 months ago (2015-10-19 17:06:27 UTC) #4
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 2 months ago (2015-10-19 19:08:35 UTC) #5
commit-bot: I haz the power
5 years, 2 months ago (2015-10-19 19:09:21 UTC) #6
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/cd0c3a5c30540ae92f017e3c9f5309c450bf65ec
Cr-Commit-Position: refs/heads/master@{#354815}

Powered by Google App Engine
This is Rietveld 408576698