Bug 100075 – [New Multicolumn][css-fixed] Fixed elements do not show across all pages in horizontal pagination mode

• Component: CSS
• Status: RESOLVED FIXED

[KyungTae Kim]

By CSS fixed-positioning spec, for paged media, boxes with fixed positions must be repeated on every page.
http://www.w3.org/TR/CSS2/visuren.html#fixed-positioning

But on horizontal pagination mode, when page changed fixed-position elements are removed.

I found this on Windows port, but I guess it's platform independent issue.

Could anybody fix it or give me hint about it, please?

[KyungTae Kim]

I'm fixing it. 
I think it needs API changes for RenderLayer::paintLayer, RenderLayer::paintLayerContents and so on.

[KyungTae Kim]

Created attachment 170558 [details]
Patch

[WebKit Review Bot]

Attachment 170558 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderLayer.h:749:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14546525

[Early Warning System Bot]

Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14566493

[Peter Beverloo (cr-android ews)]

Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14546526

[WebKit Review Bot]

Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14553552

[KyungTae Kim]

Created attachment 170595 [details]
Patch

[KyungTae Kim]

Created attachment 172309 [details]
Page1(normal)

The fixed-positioned element is showed on 1st page - normal operation.

[KyungTae Kim]

Created attachment 172310 [details]
BetweenPages(abnormal)

The fixed-positioned element need to be showed both on page1 & page2, but only showed on page1 now.

[Alexandru Chiculita]

Comment on attachment 170595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170595&action=review

Thanks for taking a look at this! Fixed positioned elements are a problem that CSS Regions have, too. Also, composited layers inside multi-column and regions are not really supported. However, I don't think this is the best approach to fix the issue.

> Source/WebCore/ChangeLog:13
> +        No new tests (OOPS!).

You should have some tests for this patch. It also helps understanding what you are trying to fix.

> Source/WebCore/rendering/RenderLayer.cpp:3058
> +    if ((paintsWithTransform(paintBehavior) && !(paintFlags & PaintLayerAppliedTransform)) || (paintFlags & PaintLayerPaintingPaginatedChildLayers)) {
>          TransformationMatrix layerTransform = renderableTransform(paintBehavior);

I'm not sure this is going to work for nested multi-column elements. Anyway, it looks like a little hack to fix the position at paint-time. Are the mouse/touch events propagated correctly? Is the bounding box of the element computed correctly?
I would try to fix that model instead (ie. collect the RenderLayers in the right parent offset layer). That might also fix the composting case.

> Source/WebCore/rendering/RenderLayer.cpp:3062
> +    // Revert the translated offset for pagination in case of fixed positioned objects. 
> +    if (PaintLayerPaintingPaginatedChildLayers && renderer()->style()->position() == FixedPosition)
> +        layerTransform.translateRight(-offsetX, -offsetY);

nit: It looks like the indentation is not right here.

> Source/WebCore/rendering/RenderLayer.h:739
> +                    PaintLayerFlags = 0, int = 0, int = 0);

nit: We usually omit the the parameter name only when it is a duplicate of the parameter type. In this case it's not, so I suppose you need it.
Is integer of good enough precision? Don't you need to use LayoutUnit instead? "offset" used to be a LayoutSize. Maybe you could pass just one LayoutSize instead of the two <int>s

[KyungTae Kim]

Comment on attachment 170595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170595&action=review

>> Source/WebCore/ChangeLog:13
>> +        No new tests (OOPS!).
> 
> You should have some tests for this patch. It also helps understanding what you are trying to fix.

I'm making the tests. I'll upload it.

>> Source/WebCore/rendering/RenderLayer.cpp:3058
>>          TransformationMatrix layerTransform = renderableTransform(paintBehavior);
> 
> I'm not sure this is going to work for nested multi-column elements. Anyway, it looks like a little hack to fix the position at paint-time. Are the mouse/touch events propagated correctly? Is the bounding box of the element computed correctly?
> I would try to fix that model instead (ie. collect the RenderLayers in the right parent offset layer). That might also fix the composting case.

Actually, current horizontal pagination implementation fix the position at paint-time, 
and the mouse/touch events are not propagated correctly.
After apply this patch, only the events are propagated correctly for fixed position elements because the (-offsetX, -offsetY) translation move back them to the correct position.

>> Source/WebCore/rendering/RenderLayer.cpp:3062
>> +        layerTransform.translateRight(-offsetX, -offsetY);
> 
> nit: It looks like the indentation is not right here.

I'll fix. Thanks.

>> Source/WebCore/rendering/RenderLayer.h:739
>> +                    PaintLayerFlags = 0, int = 0, int = 0);
> 
> nit: We usually omit the the parameter name only when it is a duplicate of the parameter type. In this case it's not, so I suppose you need it.
> Is integer of good enough precision? Don't you need to use LayoutUnit instead? "offset" used to be a LayoutSize. Maybe you could pass just one LayoutSize instead of the two <int>s

You're right but I thought passing LayoutSize object could be burden because these parameters are rarely used.
Anyway, I'm searching methods not to add these hacky parameters.

[KyungTae Kim]

Created attachment 172492 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172492 [details]
Patch

Attachment 172492 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14728787

New failing tests:
fast/multicol/pagination-lr-fixed-object.html
fast/multicol/pagination-rl-fixed-object.html

[KyungTae Kim]

Created attachment 177212 [details]
Patch

[Build Bot]

Comment on attachment 177212 [details]
Patch

Attachment 177212 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15104433

[WebKit Review Bot]

Comment on attachment 177212 [details]
Patch

Attachment 177212 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15121203

New failing tests:
fast/multicol/pagination-lr-fixed-object.html
fast/multicol/pagination-rl-fixed-object.html

[Build Bot]

Comment on attachment 177212 [details]
Patch

Attachment 177212 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15903605

[Markus Moormann]

Is somebody still working on this? We have great problems with this issue. Is there a known workaround?

[KyungTae Kim]

(In reply to comment #19)
> Is somebody still working on this? We have great problems with this issue. Is there a known workaround?

Because nobody reviewed my patch, I didn't upload additional patch.
Actually, the patch is very tricky, not a correct solution.
I guess the pagination should not occur on the paint phase, but the layout phase, but it needs many changes.

As I know, the code for multi-column used this 'paint phase' code, then moved to the 'layout phase'.

Do you have any idea for solving this problem?

[Markus Moormann]

(In reply to comment #20)
> (In reply to comment #19)
> > Is somebody still working on this? We have great problems with this issue. Is there a known workaround?
> 
> Because nobody reviewed my patch, I didn't upload additional patch.
> Actually, the patch is very tricky, not a correct solution.
> I guess the pagination should not occur on the paint phase, but the layout phase, but it needs many changes.
> 
> As I know, the code for multi-column used this 'paint phase' code, then moved to the 'layout phase'.
> 
> Do you have any idea for solving this problem?

I'm afraid I don't have any idea. I've also read quite a lot of posts that try to accomplish a footer when printing and no one had an answer so one really needs to fix this. Who can review the patch?

[Dave Hyatt]

Comment on attachment 177212 [details]
Patch

The new column code has already fixed this, and I don't want to add more stuff that just has to be removed later. Feel free to keep the bug open though, but let's just wait for the new multicolumn code for this.

[Dave Hyatt]

Adding [New Multicolumn] so I can track this when I enable the new code for view-level pagination, but it should handle it correctly.

[KyungTae Kim]

Created attachment 199131 [details]
Patch

[KyungTae Kim]

Comment on attachment 199131 [details]
Patch

I just add this patch to sync with the newest code

[Build Bot]

Comment on attachment 199131 [details]
Patch

Attachment 199131 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/16555

[Build Bot]

Comment on attachment 199131 [details]
Patch

Attachment 199131 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/181433

[Radar WebKit Bug Importer]

<rdar://problem/16656270>

[Dave Hyatt]

This will be fixed when the new columns turn on.

[Dave Hyatt]

Fixed by new columns turning on in r168046.