Bug 121828

The spec at http://dev.w3.org/csswg/css-regions/ says CSS Regions create a new stacking context. This change is also needed for the visual overflow patch.

Created attachment 212999 [details] patch

Don't you need to return true in requiresLayer() when AC is disabled?

Created attachment 213002 [details] patch. stacking contexts are not related to having backing.

Comment on attachment 213002 [details] patch. stacking contexts are not related to having backing. View in context: https://bugs.webkit.org/attachment.cgi?id=213002&action=review > LayoutTests/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Please explain why you had to change the tests below. > LayoutTests/fast/repaint/region-painting-composited-element-expected.html:13 > + position: relative; z-index: 1; /*create Stacking Context*/ nit: use two lines. > Source/WebCore/ChangeLog:8 > + Test: fast/regions/stacking-context-paint-order.html Please add more details on why the change is actually needed as we already created layers in some cases before. > Source/WebCore/rendering/RenderFlowThread.cpp:323 > + region->setRequiresLayerForCompositing(); It is not required to call setRequiresLayerForCompositing anymore as requiresLayer is always going to be true. > Source/WebCore/rendering/RenderRegion.cpp:722 > +void RenderRegion::setRequiresLayerForCompositing() Please remove this function and all its friends: RenderBox::updateLayerIfNeeded RenderBox::updatePaintingContainerForFloatingObject RenderBlock::updateFloatingObjectsPaintingContainer RenderBlock::updateLocalFloatingObjectsForPaintingContainer RenderBlock::updateAllDescendantsFloatingObjectsPaintingContainer > Source/WebCore/rendering/RenderRegion.h:145 > + virtual bool requiresLayer() const { return true; } // Because CSS Regions create Stacking Contexts. I would rewrite this to handle the isRenderRegionSet() instead of overwriting it again below.

Comment on attachment 213002 [details] patch. stacking contexts are not related to having backing. View in context: https://bugs.webkit.org/attachment.cgi?id=213002&action=review >> LayoutTests/fast/repaint/region-painting-composited-element-expected.html:13 >> + position: relative; z-index: 1; /*create Stacking Context*/ > > nit: use two lines. A div needs to be both positioned and have a z-index in order to become a Stacking Context. That's why they're on the same line (and so is the comment). >> Source/WebCore/rendering/RenderRegion.h:145 >> + virtual bool requiresLayer() const { return true; } // Because CSS Regions create Stacking Contexts. > > I would rewrite this to handle the isRenderRegionSet() instead of overwriting it again below. The OOP principle here is that the behaviour for each class can be found in that class's files. So the CSS Regions implementation is inside RenderRegion files and RegionSet keeps the old behaviour in its own file. You don't need to search in the CSS Regions file to find details about the RegionSet implementation. In the future, there will be a new class that both CSS Regions and RegionSet derive from in order to make this awkward situation go away.

Created attachment 213084 [details] patch. incorporates the feedback.

Created attachment 213091 [details] patch. incorporates the feedback.

Comment on attachment 213091 [details] patch. incorporates the feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=213091&action=review > Source/WebCore/rendering/RenderRegion.h:142 > + virtual bool requiresLayer() const { return true; } // Because CSS Regions create Stacking Contexts. Should be marking this OVERRIDE. Also, I think the comment is not clear. I’d rather you make this non-inline so you could have a clearer comment inside the .cpp file. We won’t lose any inlining from that. I would write this comment: // All regions create stacking contexts, as specified in the CSS standard. Do that by allocating a separate RenderLayer for each. bool RenderRegion::requiresLayer() const { return true; } The comment may not seem all that different to you, but I find it more specific and easier to comprehend. > Source/WebCore/rendering/RenderRegionSet.h:64 > + virtual bool requiresLayer() const { return RenderBlockFlow::requiresLayer(); } Should be marking this OVERRIDE and FINAL. But also, this needs a why comment. The change log simply says what the code is doing, and not why. A brief comment explaining why is what we are looking for. Why don’t all region sets require a layer? And given that, why do they derive from region? It is possibly evidence that the class hierarchy is flawed. The comment can be brief, but the subject is “why”, not what.

Created attachment 213157 [details] patch for landing. incorporates the feedback.

Comment on attachment 213157 [details] patch for landing. incorporates the feedback. Rejecting attachment 213157 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 213157, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/2905421

Created attachment 213163 [details] patch for landing

Comment on attachment 213163 [details] patch for landing Clearing flags on attachment: 213163 Committed r156767: <http://trac.webkit.org/changeset/156767>

Comment on attachment 213163 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=213163&action=review > Source/WebCore/rendering/RenderMultiColumnSet.h:123 > + // RenderMultiColumnSet derives from RenderRegion, but unlike the CSS Regions specification, the Multi-Columns CSS > + // specification states that the column boxes do not establish new Stacking Contexts. > + virtual bool requiresLayer() const OVERRIDE FINAL { return RenderBlockFlow::requiresLayer(); } As a follow-up thought, to repeat something I said in my initial review: I would much prefer to have this comment in a .cpp file rather than here in the header. Making the function non-inline would give us a nice place to put the comment that is not in the header. Another thought: Can we mark this entire class FINAL rather than marking this one function FINAL? Maybe it was already marked FINAL in which case I don’t think we have to mark the function too. Sorry if I misled you before. > Source/WebCore/rendering/RenderRegion.h:143 > + // All regions create stacking contexts, as specified in the CSS standard. Do that by allocating a separate RenderLayer for each. > + virtual bool requiresLayer() const OVERRIDE { return true; } As a follow-up thought, to repeat something I said in my initial review: I would much prefer to have this comment in a .cpp file rather than here in the header. Making the function non-inline would give us a nice place to put the comment that is not in the header.

Comment on attachment 213163 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=213163&action=review >> Source/WebCore/rendering/RenderMultiColumnSet.h:123 >> + virtual bool requiresLayer() const OVERRIDE FINAL { return RenderBlockFlow::requiresLayer(); } > > As a follow-up thought, to repeat something I said in my initial review: I would much prefer to have this comment in a .cpp file rather than here in the header. Making the function non-inline would give us a nice place to put the comment that is not in the header. > > Another thought: Can we mark this entire class FINAL rather than marking this one function FINAL? Maybe it was already marked FINAL in which case I don’t think we have to mark the function too. Sorry if I misled you before. Yes, the whole RenderMultiColumnSet class is marked as FINAL. There's no need to add it to this particular member function. I'm implementing these changes as part of bug #122265.