The patch submitted as part of https://bugs.webkit.org/show_bug.cgi?id=38402 has an incorrect pixel test baseline for fast/multicol/column-rules-stacking.html. The image has text above which says "You should see no red below" but the image below shows red.
Normally I'd think reverting the patch would be the best course of action, but given the size of this patch and the nature of Dave's work, I suggested to Satish we go this route instead. Dave, can you please suggest a course of action?
To add more info on the baseline: http://trac.webkit.org/log/trunk/LayoutTests/platform/mac/fast/multicol/column-rules-stacking-expected.png shows the mac baseline image before and after this change. The original baseline image (r41153) looks as expected with no red bars, whereas the new baseline image (r67660) has the red bars and contradicts the text instructions at the top of the image.
I tested with a latest build of Webkit with Safari on mac and I see this layout test html is indeed displaying red columns. Looks like the "negative z-index child" part being tested here is broken.
Dave, any word? If not, I guess we should probably just revert this?
I just found out about this bug. Next time, send me an email. I'm cc'd on every single Bugzilla bug, so I didn't even know this had been filed. I'll look into it now.
(In reply to comment #5) > I just found out about this bug. Next time, send me an email. I'm cc'd on every single Bugzilla bug, so I didn't even know this had been filed. > > I'll look into it now. Sorry about that Dave. Will keep it in mind from now on!
Created attachment 68409 [details] Patch
Comment on attachment 68409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68409&action=review Which return clause in checkContainingBlockChainForPagination() does this layout test exercise? If necessary, we should have tests for both. > WebCore/rendering/RenderLayer.cpp:471 > +static bool checkContainingBlockChainForPagination(RenderBoxModelObject* renderer, RenderBox* columnsRenderer) It's hard to wrap my brain around which of these is a descendant of the other. A comment would help. > WebCore/rendering/RenderLayer.cpp:486 > + // If the previous block is absolutely positioned, then we can't be paginated by the columns block. > + if (prevBlock->isPositioned()) Just absolutely positioned, or relatively positioned too?
(In reply to comment #8) > (From update of attachment 68409 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68409&action=review > > Which return clause in checkContainingBlockChainForPagination() does this layout test exercise? If necessary, we should have tests for both. > fast/multicol/positioned-split.html exercises the other clause. Basically the original code worked with column-rules-stacking.html but not with positioned-split.html. When I landed the new pagination model I made positioned-split.html work but broke column-rules-stacking.html. This new code makes both work. > > WebCore/rendering/RenderLayer.cpp:471 > > +static bool checkContainingBlockChainForPagination(RenderBoxModelObject* renderer, RenderBox* columnsRenderer) > > It's hard to wrap my brain around which of these is a descendant of the other. A comment would help. > I'll rename columnsRenderer to ancestorColumnsRenderer. > > WebCore/rendering/RenderLayer.cpp:486 > > + // If the previous block is absolutely positioned, then we can't be paginated by the columns block. > > + if (prevBlock->isPositioned()) > > Just absolutely positioned, or relatively positioned too? This is an excellent question. My own take on relative positioning is that it is in the flow of the columns and therefore it should be paginated by them. This is the sensible behavior in my opinion. Firefox disagrees with me though and refuses to paginate relative positioned objects. Instead it just applies the offset to all the column boxes and pushes them out of the columns container. It looks awful. I don't know if Firefox's behavior is intentional given that they can't paginate many kinds of objects in columns (tables, floats, etc.). I need to raise this with the CSS WG.
Fixed in r68069.