Bug 121828 – [CSS Regions] Activate all regions to have layers, as CSS Regions create a new stacking context

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mihai Maerean]

This work was part of bug #118665 but it would have increased the size of the patch as it changes a lot of test expectations.

[Mihai Maerean]

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.

[Mihai Maerean]

Created attachment 212999 [details]
patch

[Mihnea Ovidenie]

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

[Mihai Maerean]

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

[Alexandru Chiculita]

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.

[Mihai Maerean]

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.

[Mihai Maerean]

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

[Mihai Maerean]

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

[Darin Adler]

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.

[Mihai Maerean]

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

[WebKit Commit Bot]

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

[Mihai Maerean]

Created attachment 213163 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 213163 [details]
patch for landing

Clearing flags on attachment: 213163

Committed r156767: <http://trac.webkit.org/changeset/156767>

[Darin Adler]

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.

[Mihai Maerean]

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.