Bug 100075 - [New Multicolumn][css-fixed] Fixed elements do not show across all pages in horizontal pagination mode
Summary: [New Multicolumn][css-fixed] Fixed elements do not show across all pages in h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-22 22:41 PDT by KyungTae Kim
Modified: 2017-08-29 03:18 PDT (History)
26 users (show)

See Also:


Attachments
Patch (12.70 KB, patch)
2012-10-24 23:25 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (12.74 KB, patch)
2012-10-25 02:32 PDT, KyungTae Kim
no flags Details | Formatted Diff | Diff
Page1(normal) (63.93 KB, image/png)
2012-11-05 03:48 PST, KyungTae Kim
no flags Details
BetweenPages(abnormal) (64.38 KB, image/png)
2012-11-05 03:50 PST, KyungTae Kim
no flags Details
Patch (18.97 KB, patch)
2012-11-05 22:42 PST, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (11.00 KB, patch)
2012-12-03 02:13 PST, KyungTae Kim
no flags Details | Formatted Diff | Diff
Patch (11.49 KB, patch)
2013-04-22 17:03 PDT, KyungTae Kim
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KyungTae Kim 2012-10-22 22:41:36 PDT
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?
Comment 1 KyungTae Kim 2012-10-24 17:38:30 PDT
I'm fixing it. 
I think it needs API changes for RenderLayer::paintLayer, RenderLayer::paintLayerContents and so on.
Comment 2 KyungTae Kim 2012-10-24 23:25:37 PDT
Created attachment 170558 [details]
Patch
Comment 3 WebKit Review Bot 2012-10-24 23:29:28 PDT
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.
Comment 4 Early Warning System Bot 2012-10-24 23:29:59 PDT
Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14546525
Comment 5 Early Warning System Bot 2012-10-24 23:30:38 PDT
Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14566493
Comment 6 Peter Beverloo (cr-android ews) 2012-10-24 23:35:27 PDT
Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14546526
Comment 7 WebKit Review Bot 2012-10-24 23:54:57 PDT
Comment on attachment 170558 [details]
Patch

Attachment 170558 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14553552
Comment 8 KyungTae Kim 2012-10-25 02:32:25 PDT
Created attachment 170595 [details]
Patch
Comment 9 KyungTae Kim 2012-11-05 03:48:46 PST
Created attachment 172309 [details]
Page1(normal)

The fixed-positioned element is showed on 1st page - normal operation.
Comment 10 KyungTae Kim 2012-11-05 03:50:02 PST
Created attachment 172310 [details]
BetweenPages(abnormal)

The fixed-positioned element need to be showed both on page1 & page2, but only showed on page1 now.
Comment 11 Alexandru Chiculita 2012-11-05 11:41:59 PST
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
Comment 12 KyungTae Kim 2012-11-05 18:37:59 PST
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.
Comment 13 KyungTae Kim 2012-11-05 22:42:11 PST
Created attachment 172492 [details]
Patch
Comment 14 WebKit Review Bot 2012-11-06 01:35:40 PST
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
Comment 15 KyungTae Kim 2012-12-03 02:13:45 PST
Created attachment 177212 [details]
Patch
Comment 16 Build Bot 2012-12-03 02:46:28 PST
Comment on attachment 177212 [details]
Patch

Attachment 177212 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15104433
Comment 17 WebKit Review Bot 2012-12-03 03:05:07 PST
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
Comment 18 Build Bot 2013-01-16 15:05:26 PST
Comment on attachment 177212 [details]
Patch

Attachment 177212 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15903605
Comment 19 Markus Moormann 2013-02-18 03:00:50 PST
Is somebody still working on this? We have great problems with this issue. Is there a known workaround?
Comment 20 KyungTae Kim 2013-02-19 02:07:33 PST
(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?
Comment 21 Markus Moormann 2013-02-21 02:49:35 PST
(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?
Comment 22 Dave Hyatt 2013-03-03 14:48:44 PST
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.
Comment 23 Dave Hyatt 2013-03-03 14:56:34 PST
Adding [New Multicolumn] so I can track this when I enable the new code for view-level pagination, but it should handle it correctly.
Comment 24 KyungTae Kim 2013-04-22 17:03:42 PDT
Created attachment 199131 [details]
Patch
Comment 25 KyungTae Kim 2013-04-22 17:11:01 PDT
Comment on attachment 199131 [details]
Patch

I just add this patch to sync with the newest code
Comment 26 Build Bot 2013-04-22 17:14:42 PDT
Comment on attachment 199131 [details]
Patch

Attachment 199131 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/16555
Comment 27 Build Bot 2013-04-22 18:58:41 PDT
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
Comment 28 Radar WebKit Bug Importer 2014-04-17 23:09:59 PDT
<rdar://problem/16656270>
Comment 29 Dave Hyatt 2014-04-21 13:44:46 PDT
This will be fixed when the new columns turn on.
Comment 30 Dave Hyatt 2014-04-30 14:56:51 PDT
Fixed by new columns turning on in r168046.