Bug 111176

Created attachment 190957 [details] Ref-test highlighting the problem

Created attachment 204339 [details] WIP 1 First attempt with fixed elements not positioned by flow thread but by render view. The fixed elements are inserted into the list of flow thread content nodes.

Created attachment 206094 [details] Patch

Comment on attachment 206094 [details] Patch Attachment 206094 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/992154 New failing tests: fast/regions/element-in-named-flow-fixed-from-static.html

Created attachment 206101 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

I have applied the patch on my ML machine and i was unable to reproduce the test failure.

Comment on attachment 206094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206094&action=review Looks good, I have a couple of questions below. Is there any other case that we need to patch in order to avoid the CSS Region rules for the content of the fixed elements. For example flowThreadState() == RenderObject::InsideFlowThread will be true even if the element is inside the fixed elements that are now collected by the RenderView. In that case you don't have any flow thread layout state on the stack and it might have undefined behavior. > Source/WebCore/rendering/RenderBlock.cpp:7758 > + LayoutState* layoutState = view()->layoutState(); nit: this variable seems to be unused. > Source/WebCore/rendering/RenderBox.cpp:4602 > if ((layoutState && !layoutState->isPaginated()) || (!layoutState && !flowThreadContainingBlock())) > + return false; > + return true; not: there's no need for the if statement. write just one return statement. > Source/WebCore/rendering/RenderLayer.cpp:1359 > + layer = layer->parent(); This is based on the fact that the RenderView is the parent of the RenderFlowThread. I think we need some asserts to catch these subtle assumptions. We may need to change that in the future and asserts will help finding such assumptions. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1787 > + && (layer->renderer()->flowThreadState() == RenderObject::NotInsideFlowThread || layer->renderer()->fixedPositionInNamedFlowWithViewContainingBlock()); What happens for the 3D elements nested in the "positioned: fixed" elements. I think that those layers should have compositing enabled too. > Source/WebCore/rendering/RenderLayerModelObject.cpp:136 > + layer()->dirtyStackingContainerZOrderLists(); Should we also dirty the receiving stacking container? Where is that done? Can you add a comment to clarify it? > Source/WebCore/rendering/RenderView.cpp:145 > flowThreadController()->layoutRenderNamedFlowThreads(); > + layoutPositionedObjects(false, true); This doesn't work with nested flows. For example a fixed region inside a flow will stay without a layout.

Created attachment 206627 [details] Patch 2

Attachment 206627 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/TestExpectations', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed.html', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute.html', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static-expected.txt', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static.html', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed-expected.txt', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed.html', u'LayoutTests/fast/regions/fixed-element-transformed-parent-expected.txt', u'LayoutTests/fast/regions/fixed-element-transformed-parent.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region.html', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow-expected.txt', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/NodeRenderingContext.cpp', u'Source/WebCore/rendering/FlowThreadController.cpp', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderBox.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerModelObject.cpp', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/RenderView.cpp', u'Source/WebCore/rendering/RenderView.h']" exit_code: 1 Source/WebCore/rendering/RenderObject.cpp:658: Missing space before ( in while( [whitespace/parens] [5] Source/WebCore/rendering/RenderBlock.h:511: The parameter name "namedFlow" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #7) > (From update of attachment 206094 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206094&action=review > > Looks good, I have a couple of questions below. > > Is there any other case that we need to patch in order to avoid the CSS Region rules for the content of the fixed elements. For example flowThreadState() == RenderObject::InsideFlowThread will be true even if the element is inside the fixed elements that are now collected by the RenderView. In that case you don't have any flow thread layout state on the stack and it might have undefined behavior. > I took a look at the code and i was not able to spot such situation. The fixed positioned elements in this situation are always laid out after the flow thread has been laid out and they do not depend on flow thread state on the stack. > > Source/WebCore/rendering/RenderBlock.cpp:7758 > > + LayoutState* layoutState = view()->layoutState(); > > nit: this variable seems to be unused. > I moved it closer to the place where it is used. > > Source/WebCore/rendering/RenderBox.cpp:4602 > > if ((layoutState && !layoutState->isPaginated()) || (!layoutState && !flowThreadContainingBlock())) > > + return false; > > + return true; > > not: there's no need for the if statement. write just one return statement. > I simplified the function with this occasion. > > Source/WebCore/rendering/RenderLayer.cpp:1359 > > + layer = layer->parent(); > > This is based on the fact that the RenderView is the parent of the RenderFlowThread. I think we need some asserts to catch these subtle assumptions. We may need to change that in the future and asserts will help finding such assumptions. > Added an assert to check that the parent layer for the flow thread layer is the view layer. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1787 > > + && (layer->renderer()->flowThreadState() == RenderObject::NotInsideFlowThread || layer->renderer()->fixedPositionInNamedFlowWithViewContainingBlock()); > > What happens for the 3D elements nested in the "positioned: fixed" elements. I think that those layers should have compositing enabled too. > I lifted the restriction here for all elements inside a flow thread, except for the flow thread itself. I skipped the test fast/repaint/region-painting-composited-element.html with this occasion. > > Source/WebCore/rendering/RenderLayerModelObject.cpp:136 > > + layer()->dirtyStackingContainerZOrderLists(); > > Should we also dirty the receiving stacking container? Where is that done? Can you add a comment to clarify it? > Yes, i have done that and added a test for it. > > Source/WebCore/rendering/RenderView.cpp:145 > > flowThreadController()->layoutRenderNamedFlowThreads(); > > + layoutPositionedObjects(false, true); > > This doesn't work with nested flows. For example a fixed region inside a flow will stay without a layout. The fixed elements inside a flow thread that are to be laid out by view are laid out after their flow thread ancestor has been laid out, not in a bulk call after all the flow threads have been laid out.

Comment on attachment 206627 [details] Patch 2 Attachment 206627 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1066522 New failing tests: compositing/regions/webkit-flow-renderer-layer-compositing.html

Created attachment 206630 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.3

Comment on attachment 206627 [details] Patch 2 Attachment 206627 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1063671 New failing tests: fast/multicol/mixed-positioning-stacking-order.html fast/regions/element-in-named-flow-fixed-from-absolute.html fullscreen/full-screen-iframe-with-max-width-height.html fast/regions/element-outflow-static-from-inflow-fixed.html fast/regions/element-inflow-fixed-from-outflow-static.html fast/multicol/mixed-opacity-fixed-test.html compositing/regions/webkit-flow-renderer-layer-compositing.html fast/regions/element-in-named-flow-absolute-from-fixed.html

Created attachment 206634 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

Created attachment 206646 [details] Patch 3 Correct style and skip another regions compositing test.

Comment on attachment 206646 [details] Patch 3 Attachment 206646 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1068483 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html

Created attachment 206650 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.3

Comment on attachment 206646 [details] Patch 3 Attachment 206646 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1077085 New failing tests: fast/multicol/mixed-positioning-stacking-order.html fast/regions/element-in-named-flow-fixed-from-absolute.html fast/regions/element-outflow-static-from-inflow-fixed.html fast/regions/element-inflow-fixed-from-outflow-static.html fast/multicol/mixed-opacity-fixed-test.html http/tests/security/cross-origin-plugin-private-browsing-toggled.html fast/regions/element-in-named-flow-absolute-from-fixed.html

Created attachment 206663 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

Comment on attachment 206646 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=206646&action=review r- In general, I don't like what's happening here, but it's a spec disagreement. I would rather fixed positioned objects just not be placed inside the named flow thread at all. I don't see any value whatsoever in allowing them to be inside the flow thread. If instead they were just outside the named flow thread completely, then all of this code would get way simpler. Is there any compelling use case for fixed positioned objects being inside the flow threads? Anyway, the rest of the comments are assuming the spec stays the way it is, so here goes: Don't you need to patch RenderLayer::accumulateOffsetTowardsAncestor also? > Source/WebCore/rendering/FlowThreadController.cpp:105 > flowRenderer->layoutIfNeeded(); > + flowRenderer->view()->layoutNamedFlowFixedPositionedObjects(flowRenderer); I would make a member function on RenderNamedFlowThread that handles doing both of these operations. layoutNormalFlowAndFixedPositionedObjects. > Source/WebCore/rendering/RenderBlock.cpp:2878 > + if (namedFlow) { > + ASSERT(this->isRenderView()); > + if (namedFlow != r->flowThreadAncestorForFixedPositionedObjects()) > + continue; > + } I don't like this code in RenderBlock when it's view-specific. Also, you put it in front of comment that describes code that comes after the code you put in, so you just disconnected the comment from some code it applied to. I would suggest doing something differently here. I think you should refactor layoutPositionedObjects so that the work it does for each object inside the loop is in a helper function, e.g., layoutPositionedObject(). Then you could just have a complete implementation of laying out positioned objects over in layoutNamedFlowPositionedObjects that has your named flow filtering check and that then just calls layoutPositionedObject() to do the rest of the work. That keeps you from having to touch this RenderBlock code. > Source/WebCore/rendering/RenderBlock.h:511 > - void layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly = false); > + void layoutPositionedObjects(bool relayoutChildren, bool fixedPositionObjectsOnly = false, const RenderNamedFlowThread* = 0); Shouldn't have to add this, as per my comment above. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1787 > + && !layer->isOutOfFlowRenderFlowThread(); These aren't equivalent. You just changed behavior for in-flow RenderFlowThreads. > Source/WebCore/rendering/RenderLayerModelObject.cpp:136 > + if (flowThreadState() == RenderObject::InsideOutOfFlowThread) { > + bool wasFixedPositionedInFlowThread = oldStyle->position() == FixedPosition; > + bool isFixedPositionedInFlowThread = newStyle->position() == FixedPosition; > + if (wasFixedPositionedInFlowThread != isFixedPositionedInFlowThread) > + layer()->dirtyStackingContainerZOrderLists(); Helper function please. > Source/WebCore/rendering/RenderLayerModelObject.cpp:182 > + if (flowThreadState() == RenderObject::InsideOutOfFlowThread) { > + bool wasFixedPositionedInFlowThread = oldStyle && oldStyle->position() == FixedPosition; > + bool isFixedPositionedInFlowThread = style()->position() == FixedPosition; > + if (wasFixedPositionedInFlowThread != isFixedPositionedInFlowThread) > + layer()->dirtyStackingContainerZOrderLists(); Helper function please, since this is identical to the code above. > Source/WebCore/rendering/RenderObject.cpp:631 > +bool RenderObject::fixedPositionInNamedFlowWithViewContainingBlock() const > +{ > + return (flowThreadState() == RenderObject::InsideOutOfFlowThread) > + && (style()->position() == FixedPosition) > + && (containingBlock() == view()); > +} This is only used by RenderLayer. I'd just put it over there rather than adding new code to RenderObject. > Source/WebCore/rendering/RenderObject.cpp:665 > +RenderFlowThread* RenderObject::flowThreadAncestorForFixedPositionedObjects() const > +{ > + if ((flowThreadState() != InsideOutOfFlowThread) || (style()->position() != FixedPosition)) > + return 0; > + > + RenderObject* curr = const_cast<RenderObject*>(this); > + while (curr) { > + if (curr->isRenderFlowThread()) > + return toRenderFlowThread(curr); > + curr = curr->parent(); > + } > + > + return 0; > +} I would factor this a little differently. I would have a RenderObject function that does only the while loop. Make flowThreadContainingBlock() also use it for the slow case. Then RenderNamedFlowThread could do the flowThreadState() and fixedPosition condition checks in its own loop, and then all it has to do is call the crawl function if those pass. > Source/WebCore/rendering/RenderObject.cpp:820 > + || (isOutOfFlowRenderFlowThread() && !toRenderFlowThread(this)->hasRegions()); This check confuses me. I don't quite understand it. Why is hasRegions() supposed to be false here? > Source/WebCore/rendering/RenderView.cpp:141 > +void RenderView::layoutNamedFlowFixedPositionedObjects(const RenderNamedFlowThread* namedFlow) > +{ > + layoutPositionedObjects(false, true, namedFlow); > +} I would implement the loop yourself and do the filtering yourself.

How about instead of this patch you just hack the layout of the objects to size/place relative to the viewport while still leaving them in the flow threads? This would make the condition easily detected during RenderNamedFlowThread's layoutPositionedObjects...

Created attachment 208623 [details] WIP1 with alternate approach Alternate approach while leaving the fixed positioned elements inside the flow thread.

Created attachment 209993 [details] WIP2 with alternate approach. Rebased and with tests.

Created attachment 210123 [details] Patch

Attachment 210123 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-absolute-from-fixed.html', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute-expected.txt', u'LayoutTests/fast/regions/element-in-named-flow-fixed-from-absolute.html', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static-expected.txt', u'LayoutTests/fast/regions/element-inflow-fixed-from-outflow-static.html', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed-expected.txt', u'LayoutTests/fast/regions/element-outflow-static-from-inflow-fixed.html', u'LayoutTests/fast/regions/fixed-element-transformed-parent-expected.txt', u'LayoutTests/fast/regions/fixed-element-transformed-parent.html', u'LayoutTests/fast/regions/fixed-in-named-flow-scroll-expected.txt', u'LayoutTests/fast/regions/fixed-in-named-flow-scroll.html', u'LayoutTests/fast/regions/fixed-inside-fixed-in-named-flow-expected.html', u'LayoutTests/fast/regions/fixed-inside-fixed-in-named-flow.html', u'LayoutTests/fast/regions/fixed-inside-named-flow-zIndex-expected.html', u'LayoutTests/fast/regions/fixed-inside-named-flow-zIndex.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-namedflow-noregions.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region-expected.html', u'LayoutTests/fast/regions/fixed-pos-elem-in-region.html', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow-expected.html', u'LayoutTests/fast/regions/fixed-pos-region-in-nested-flow.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/FlowThreadController.cpp', u'Source/WebCore/rendering/FlowThreadController.h', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h']" exit_code: 1 Source/WebCore/rendering/RenderLayer.h:947: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:947: The parameter name "paintingInfo" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:989: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:990: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:991: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:992: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:993: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.h:989: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:989: The parameter name "result" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:990: The parameter name "hitTestLocation" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:991: The parameter name "transformState" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/RenderLayer.h:994: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderLayer.cpp:4547: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 13 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 210123 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210123&action=review r=me, but fix all the style queue errors. I also have a few suggestions below. > Source/WebCore/rendering/RenderBox.cpp:1847 > + // For fixed positioned elements inside out-of-flow named flows, we do not want to > + // map their position further to regions based on their coordinates inside the named flows. > + if (o->isOutOfFlowRenderFlowThread() && fixedPositionedWithNamedFlowContainingBlock()) > + o->mapLocalToContainer(toRenderLayerModelObject(o), transformState, mode, wasFixed); > + else > + o->mapLocalToContainer(repaintContainer, transformState, mode, wasFixed); I would invert this to give precedence to the non-flow-thread code path. if (!o->isOutOfFlowRenderFlowThread() || !fixedPositionedWithNamedFlowContainingBlock()) // Do the common thing. :) > Source/WebCore/rendering/RenderBox.cpp:2975 > + if (containingBlock->isRenderNamedFlowThread() && (style()->position() == FixedPosition)) No need for parentheses around (style()->position() == FixedPosition()) > Source/WebCore/rendering/RenderBox.cpp:3033 > + if (containingBlock->isRenderNamedFlowThread() && (style()->position() == FixedPosition)) Ditto. >> Source/WebCore/rendering/RenderLayer.cpp:4547 >> + RenderLayer* hitLayer = fixedLayer->hitTestLayer(fixedLayer->renderer().flowThreadContainingBlock()->layer(), 0, request, tempResult, >> + hitTestRect, hitTestLocation, false, transformState, zOffsetForDescendants); > > When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Fix the funny indentation here.

Comment on attachment 210123 [details] Patch Attachment 210123 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1582636 New failing tests: fast/regions/fixed-in-named-flow-scroll.html

Created attachment 210156 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Created attachment 210331 [details] Patch for landing

Comment on attachment 210331 [details] Patch for landing Clearing flags on attachment: 210331 Committed r154973: <http://trac.webkit.org/changeset/154973>

All reviewed patches have been landed. Closing bug.

It looks like the bots MountainLion Debug WebKit2 are red since this patch landed. The following two tests fail: fast/regions/fixed-inside-named-flow-zIndex.html fast/regions/fixed-pos-region-in-nested-flow.html Can you please have a look?

(In reply to comment #32) > It looks like the bots MountainLion Debug WebKit2 are red since this patch landed. > > The following two tests fail: > fast/regions/fixed-inside-named-flow-zIndex.html > fast/regions/fixed-pos-region-in-nested-flow.html > > Can you please have a look? Absolutely, sorry for this, taking a look.