Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388723004:
[SPInvalidation] Fix PrePaintTreeWalk for multicol spanner (Closed)
Description[SPInvalidation] Fix PrePaintTreeWalk for multicol spanner
We should not do anything for the spanner placeholder but just walk the
spanner directly.
Out-of-flow positioned descendants of a multicol spanner need to be
specially handled because their container may be not their ancestor in
the order of PrePaintTreeWalk.
This fixes the following layout tests for slimmingPaintInvalidation:
fast/multicol/dynamic/*spanner*.html
fast/multicol/span/*.html
(except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html
which is still failing because of some other reason.)
BUG=646176
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/ea4d26d0200991d105b7ae8b43a7040c5d1e3f5a
Cr-Commit-Position: refs/heads/master@{#423713}
Patch Set 1 #Patch Set 2 : Positioned descendants of spanner; Unit test #
Total comments: 16
Patch Set 3 : fixes #
Total comments: 6
Patch Set 4 : Revert ContainingBlockContext changes #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== [SPInvalidation] Fix multicol span tree walk We should not do anything for the spanner placeholder but just walk the spanner directly. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 ========== to ========== [SPInvalidation] Fix multicol span tree walk We should not do anything for the spanner placeholder but just walk the spanner directly. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== [SPInvalidation] Fix multicol span tree walk We should not do anything for the spanner placeholder but just walk the spanner directly. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Description was changed from ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
Hold on. Just found that fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html failure is also related to tree walk. Will upload a new patch to fix them all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/04 17:49:49, Xianzhu wrote: > Hold on. Just found that > fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html failure > is also related to tree walk. Will upload a new patch to fix them all. Done. Ptal. The new patch fixed part of fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html. It still fails because of bug 648274. The added code in PaintPropertyTreeBuilder doesn't have counterpart in old spv1 because slow path is forced for multicol spanners.
Description was changed from ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. Out-of-flow positioned descendants of a multicol spanner need to be specially handled because their container may be not their ancestor in the order of PrePaintTreeWalk. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chrishtr@chromium.org changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:669: container.objectPaintProperties()->localBorderBoxProperties(); DCHECK(properties && container.isBox()) https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2748: TEST_P(PaintPropertyTreeBuilderTest, PaintOffsetUnderMultiColumn) { @mstensho could you please review this test and verify Xianzhu's calculations as well as his comments about some of them being incorrect?
https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2760: " <div id=normal style='height: 50px'>" Missing </div> ? https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2764: " <div id=space3 class=space></div>" I don't think you intended for this to be inside the spanner? At least that's what the indentation suggests. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2772: // Column 2. TODO(crbug.com/648274): This is incorrect. Should be (50, 0). What should it be relative to? LayoutPoint(0, 30) suggests that it's relative to the flow thread. As far as I can read from the documentation, it should be relative to "the propertyTreeState's transform space". Does that mean the viewport in this case, since we have no transforms? If it should be something else, I don't think (50, 0) is right. Perhaps (100, 0). Column width is 100px, after all. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2786: // Column 2. TODO(crbug.com/648274): This is incorrect. Should be (50, 80). (100, 80), perhaps? https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2793: EXPECT_EQ(LayoutPoint(0, 30), Shouldn't this be (0,0) then? https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2797: // TODO(crbug.com/648274): This is incorrect. Should be (80, 90). Probably (180, 90) then. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:63: localContext.treeBuilderContext.isUnderMultiColumnSpanner = true; <div style="columns:3;"> <div id="rel" style="position:relative;"> <div id="spanner" style="columns-span:all;"> <div id="abs" style="position:absolute;"></div> </div> </div> </div> The containing block of #abs is #rel. As such, #abs escapes #spanner and becomes out-of-flow multicol content, and not part of the spanner. So if isUnderMultiColumnSpanner is true at #abs, I guess it's a problem. Here, on the other hand: <div style="columns:3;"> <div id="relspanner" style="columns-span:all;"> <div id="abs" style="position:absolute;"></div> </div> </div> The containing block of #abs is #relspanner, so here #abs lives inside the spanner.
https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:669: container.objectPaintProperties()->localBorderBoxProperties(); On 2016/10/05 00:48:17, chrishtr wrote: > DCHECK(properties && container.isBox()) Done. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2760: " <div id=normal style='height: 50px'>" On 2016/10/05 08:44:09, mstensho wrote: > Missing </div> ? Done. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2764: " <div id=space3 class=space></div>" On 2016/10/05 08:44:09, mstensho wrote: > I don't think you intended for this to be inside the spanner? At least that's > what the indentation suggests. Right. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2772: // Column 2. TODO(crbug.com/648274): This is incorrect. Should be (50, 0). On 2016/10/05 08:44:09, mstensho wrote: > What should it be relative to? LayoutPoint(0, 30) suggests that it's relative to > the flow thread. As far as I can read from the documentation, it should be > relative to "the propertyTreeState's transform space". Does that mean the > viewport in this case, since we have no transforms? > > If it should be something else, I don't think (50, 0) is right. Perhaps (100, > 0). Column width is 100px, after all. Changed the comment to (100, 0). Made the mistake (as well as other should-be's) using 50px column width. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2786: // Column 2. TODO(crbug.com/648274): This is incorrect. Should be (50, 80). On 2016/10/05 08:44:09, mstensho wrote: > (100, 80), perhaps? Done. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2797: // TODO(crbug.com/648274): This is incorrect. Should be (80, 90). On 2016/10/05 08:44:09, mstensho wrote: > Probably (180, 90) then. Done. https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): https://codereview.chromium.org/2388723004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:63: localContext.treeBuilderContext.isUnderMultiColumnSpanner = true; On 2016/10/05 08:44:09, mstensho wrote: > <div style="columns:3;"> > <div id="rel" style="position:relative;"> > <div id="spanner" style="columns-span:all;"> > <div id="abs" style="position:absolute;"></div> > </div> > </div> > </div> > > The containing block of #abs is #rel. As such, #abs escapes #spanner and becomes > out-of-flow multicol content, and not part of the spanner. > > So if isUnderMultiColumnSpanner is true at #abs, I guess it's a problem. > > Here, on the other hand: > > <div style="columns:3;"> > <div id="relspanner" style="columns-span:all;"> > <div id="abs" style="position:absolute;"></div> > </div> > </div> > > The containing block of #abs is #relspanner, so here #abs lives inside the > spanner. Yes. Updated comments here and PaintPropertyTreeBuilder to state that the special case may or may not happen and the special handling applies is for the special case only.
I agree with the comments in the test now. https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2787: EXPECT_EQ(LayoutPoint(0, 90), Yeah.. looks like you're getting these values in the flow thread coordinate space.
https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2787: EXPECT_EQ(LayoutPoint(0, 90), On 2016/10/05 18:17:54, mstensho wrote: > Yeah.. looks like you're getting these values in the flow thread coordinate > space. Yes. We need to fix bug 648274 by mapping the paint offsets into the correct coordinate space.
OK, back to chrishtr for the magic word, then. :) I don't think I should stamp this, as I'm unfamiliar with the code.
https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:25: // State that propagates on the container chain (and so is adjusted when an What do mean by container? Calling container()? parent()?
https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:25: // State that propagates on the container chain (and so is adjusted when an On 2016/10/06 16:22:32, chrishtr wrote: > What do mean by container? Calling container()? parent()? container(). Previous "containing block" was not accurate because it might be also a relative-position LayoutInline (as the container of an absolute-position object).
https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:25: // State that propagates on the container chain (and so is adjusted when an On 2016/10/06 at 16:47:48, Xianzhu wrote: > On 2016/10/06 16:22:32, chrishtr wrote: > > What do mean by container? Calling container()? parent()? > > container(). > > Previous "containing block" was not accurate because it might be also a relative-position LayoutInline (as the container of an absolute-position object). container() is actually containing block, as defined by CSS. https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... (see reference to container()) Walter plans to rename those methods soon to be less confusing. In the meantime I think it would be better to change this back to ContainingBlockContext.
https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h (right): https://codereview.chromium.org/2388723004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h:25: // State that propagates on the container chain (and so is adjusted when an On 2016/10/06 16:54:21, chrishtr wrote: > On 2016/10/06 at 16:47:48, Xianzhu wrote: > > On 2016/10/06 16:22:32, chrishtr wrote: > > > What do mean by container? Calling container()? parent()? > > > > container(). > > > > Previous "containing block" was not accurate because it might be also a > relative-position LayoutInline (as the container of an absolute-position > object). > > container() is actually containing block, as defined by CSS. > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > (see reference to container()) > > Walter plans to rename those methods soon to be less confusing. In the meantime > I think it would be better to change this back to ContainingBlockContext. I should have read Walter's doc more thoroughly. It's great to make clear of the concepts. Reverted the changes about ContainingBlockContext.
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. Out-of-flow positioned descendants of a multicol spanner need to be specially handled because their container may be not their ancestor in the order of PrePaintTreeWalk. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. Out-of-flow positioned descendants of a multicol spanner need to be specially handled because their container may be not their ancestor in the order of PrePaintTreeWalk. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. Out-of-flow positioned descendants of a multicol spanner need to be specially handled because their container may be not their ancestor in the order of PrePaintTreeWalk. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Fix PrePaintTreeWalk for multicol spanner We should not do anything for the spanner placeholder but just walk the spanner directly. Out-of-flow positioned descendants of a multicol spanner need to be specially handled because their container may be not their ancestor in the order of PrePaintTreeWalk. This fixes the following layout tests for slimmingPaintInvalidation: fast/multicol/dynamic/*spanner*.html fast/multicol/span/*.html (except for fast/multicol/dynamic/abspos-multicol-with-spanner-becomes-spanner.html which is still failing because of some other reason.) BUG=646176 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/ea4d26d0200991d105b7ae8b43a7040c5d1e3f5a Cr-Commit-Position: refs/heads/master@{#423713} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ea4d26d0200991d105b7ae8b43a7040c5d1e3f5a Cr-Commit-Position: refs/heads/master@{#423713}
Message was sent while issue was closed.
I was looking at this again to see if we could simplify the multi-column
special-cases in PrePaintTreeWalk and PaintPropertyTreeBuilder. We currently
modify the paint and hit testing treewalks to special-case colspan:all by
skipping spanners and later traversing them through a placeholder. The current
pre-paint tree walk (and associated paint property tree building) has a similar
modification. Could we let paint properties get built in layout tree order
instead? My reasoning is that the paint properties follow the unmodified layout
tree order:
<!doctype html>
<div id="columns" style="columns: 1; width: 100px;">
a b c
<div id="relative" style="position: relative; background: green;
filter:blur(1px);">
relative
<div id="spanner" style="column-span: all">
<div id="absolute" style="position: absolute; background: blue; top: 5px; left:
0;">abs</div>
</div>
</div>
d e f
<div>
*LayoutView 0x268adf604010 #document
LayoutBlockFlow 0x268adf61c010 HTML
LayoutBlockFlow 0x268adf61c138 BODY
LayoutBlockFlow 0x268adf61c260 DIV id="columns" style="columns: 1;
width: 100px;"
LayoutMultiColumnFlowThread (anonymous) 0x268adf628010
LayoutBlockFlow (anonymous) 0x268adf61ca78
LayoutText 0x268adf63c1a0 #text "\na b c\n"
LayoutBlockFlow (relative positioned) 0x268adf61c4b0 DIV id="relative"
style="position: relative; background: green; filter:blur(1px);"
LayoutBlockFlow (anonymous) 0x268adf61c388
LayoutText 0x268adf63c268 #text "\nrelative\n"
LayoutBlockFlow (column spanner) 0x268adf61c828 DIV id="spanner"
style="column-span: all"
LayoutBlockFlow (positioned) 0x268adf61cba0 DIV id="absolute"
style="position: absolute; background: blue; top: 5px; left: 0;"
LayoutText 0x268adf63c330 #text "abs"
LayoutBlockFlow (anonymous) 0x268adf61c950
LayoutText 0x268adf63c010 #text "\nd e f\n"
LayoutBlockFlow 0x268adf61c5d8 DIV
LayoutMultiColumnSet (anonymous) 0x268adf64c300
LayoutMultiColumnSpannerPlaceholder (anonymous) 0x268adf638010
LayoutMultiColumnSet (anonymous) 0x268adf64c010
In layout tree order: "a b c", "relative", "spanner", "abs", "d e f"
In paint order (also, current prepaint tree walk order): "a b c", "relative", "d
e f", "spanner", "abs"
The spanner paint offset would need some finessing, but I think we could adjust
the paint offset when the spanner is traversed, rather than adjusting the
traversal order for everything else.
Message was sent while issue was closed.
On 2017/01/04 06:15:04, pdr. wrote: > I was looking at this again to see if we could simplify the multi-column > special-cases in PrePaintTreeWalk and PaintPropertyTreeBuilder. We currently > modify the paint and hit testing treewalks to special-case colspan:all by > skipping spanners and later traversing them through a placeholder. The current > pre-paint tree walk (and associated paint property tree building) has a similar > modification. Could we let paint properties get built in layout tree order > instead? My reasoning is that the paint properties follow the unmodified layout > tree order: > > <!doctype html> > <div id="columns" style="columns: 1; width: 100px;"> > a b c > <div id="relative" style="position: relative; background: green; > filter:blur(1px);"> > relative > <div id="spanner" style="column-span: all"> > <div id="absolute" style="position: absolute; background: blue; top: 5px; left: > 0;">abs</div> > </div> > </div> > d e f > <div> > > *LayoutView 0x268adf604010 #document > LayoutBlockFlow 0x268adf61c010 HTML > LayoutBlockFlow 0x268adf61c138 BODY > LayoutBlockFlow 0x268adf61c260 DIV id="columns" style="columns: 1; > width: 100px;" > LayoutMultiColumnFlowThread (anonymous) 0x268adf628010 > LayoutBlockFlow (anonymous) 0x268adf61ca78 > LayoutText 0x268adf63c1a0 #text "\na b c\n" > LayoutBlockFlow (relative positioned) 0x268adf61c4b0 DIV id="relative" > style="position: relative; background: green; filter:blur(1px);" > LayoutBlockFlow (anonymous) 0x268adf61c388 > LayoutText 0x268adf63c268 #text "\nrelative\n" > LayoutBlockFlow (column spanner) 0x268adf61c828 DIV id="spanner" > style="column-span: all" > LayoutBlockFlow (positioned) 0x268adf61cba0 DIV id="absolute" > style="position: absolute; background: blue; top: 5px; left: 0;" > LayoutText 0x268adf63c330 #text "abs" > LayoutBlockFlow (anonymous) 0x268adf61c950 > LayoutText 0x268adf63c010 #text "\nd e f\n" > LayoutBlockFlow 0x268adf61c5d8 DIV > LayoutMultiColumnSet (anonymous) 0x268adf64c300 > LayoutMultiColumnSpannerPlaceholder (anonymous) 0x268adf638010 > LayoutMultiColumnSet (anonymous) 0x268adf64c010 > > In layout tree order: "a b c", "relative", "spanner", "abs", "d e f" > In paint order (also, current prepaint tree walk order): "a b c", "relative", "d > e f", "spanner", "abs" > > The spanner paint offset would need some finessing, but I think we could adjust > the paint offset when the spanner is traversed, rather than adjusting the > traversal order for everything else. The proposal sounds great. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
