Summary: | [New Multicolumn][css-fixed] Fixed elements do not show across all pages in horizontal pagination mode | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | KyungTae Kim <ktf.kim> | ||||||||||||||||
Component: | CSS | Assignee: | Dave Hyatt <hyatt> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | anilsson, buildbot, commit-queue, dglazkov, donggwan.kim, dw.im, eric, esprehn+autocc, hyatt, jonlee, khodych, laszlo.gombos, leviw, lmcliste, mitz, moormann, ojan, peter+ews, prakashviswanathan18, rniwa, s.choi, simon.fraser, tonikitoo, vincent, webkit-bug-importer, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
KyungTae Kim
2012-10-22 22:41:36 PDT
I'm fixing it. I think it needs API changes for RenderLayer::paintLayer, RenderLayer::paintLayerContents and so on. Created attachment 170558 [details]
Patch
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 on attachment 170558 [details] Patch Attachment 170558 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14546525 Comment on attachment 170558 [details] Patch Attachment 170558 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14566493 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 on attachment 170558 [details] Patch Attachment 170558 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14553552 Created attachment 170595 [details]
Patch
Created attachment 172309 [details]
Page1(normal)
The fixed-positioned element is showed on 1st page - normal operation.
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 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 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. Created attachment 172492 [details]
Patch
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 Created attachment 177212 [details]
Patch
Comment on attachment 177212 [details] Patch Attachment 177212 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15104433 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 on attachment 177212 [details] Patch Attachment 177212 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15903605 Is somebody still working on this? We have great problems with this issue. Is there a known workaround? (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? (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 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.
Adding [New Multicolumn] so I can track this when I enable the new code for view-level pagination, but it should handle it correctly. Created attachment 199131 [details]
Patch
Comment on attachment 199131 [details]
Patch
I just add this patch to sync with the newest code
Comment on attachment 199131 [details] Patch Attachment 199131 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/16555 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 This will be fixed when the new columns turn on. Fixed by new columns turning on in r168046. |