Bug 91405 – z-index should work without position on flexitems

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Tony Chang]

This was recently added to the spec: http://dev.w3.org/csswg/css3-flexbox/#painting

[Ojan Vafai]

Created attachment 157308 [details]
Work in progress

[Ojan Vafai]

Comment on attachment 157308 [details]
Work in progress

In theory, this should be all we need to do, but requiresLayer is called before the flex item is appended to the RenderFlexibleBox, so parent() is always null.

[Ojan Vafai]

I looked into calling addChild before we setStyle on the RenderBlock. addChild has a few things that depend on the style, so getting that to work would be both non-trivial and not clearly the correct change.

The alternative is to have RenderObject::createObject return RenderFlexibleBoxItems instead of RenderBlocks if the parent node is a flexbox. Then RenderFlexibleBoxItem would only override requiresLayer to return true if it has a z-index. Anyone opposed to this change?

[Tony Chang]

(In reply to comment #3)
> The alternative is to have RenderObject::createObject return RenderFlexibleBoxItems instead of RenderBlocks if the parent node is a flexbox. Then RenderFlexibleBoxItem would only override requiresLayer to return true if it has a z-index. Anyone opposed to this change?

I suspect this would cause problems if we need to change the render tree (e.g., changing the display from -webkit-flex to block in JS).

[Tony Chang]

Don't we create layers dynamically? E.g., when adding scrollbars or when dynamically making a block position:absolute?

[Ojan Vafai]

(In reply to comment #4)
> (In reply to comment #3)
> > The alternative is to have RenderObject::createObject return RenderFlexibleBoxItems instead of RenderBlocks if the parent node is a flexbox. Then RenderFlexibleBoxItem would only override requiresLayer to return true if it has a z-index. Anyone opposed to this change?
> 
> I suspect this would cause problems if we need to change the render tree (e.g., changing the display from -webkit-flex to block in JS).

What sorts of problems? We need to recreate the tree regardless, no? I suppose it would be slower because we need to recreate all the children instead of reparent them?

(In reply to comment #5)
> Don't we create layers dynamically? E.g., when adding scrollbars or when dynamically making a block position:absolute?

We create them under styleDidChange, which ends up getting called when we first create the RenderObject. There may be cases where we know the parent, but there are cases where we don't, so it doesn't really help.

[Ojan Vafai]

Actually, there's no way we can change the type of the flex-items because they are not necessarily RenderBlocks. They could be tables, flexboxes, etc.

So, some way or another, requiresLayer needs to know that this object's parent will be a flexbox.

[Simon Fraser (smfr)]

If this is too gross we can push back on the spec change.

[Ojan Vafai]

Actually, I think this is easy. I didn't realize adjustRenderStyle already has the following: 
    if (style->position() == StaticPosition)
        style->setHasAutoZIndex();

We just need to not do that if the parent is a flexbox. Then in RenderBlock, we can always require a layer if it has a non-auto z-index.

[Ojan Vafai]

Created attachment 157539 [details]
Patch

[Build Bot]

Comment on attachment 157539 [details]
Patch

Attachment 157539 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13470365

[Julien Chaffraix]

Comment on attachment 157539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157539&action=review

I think the approach is sound.

> Source/WebCore/css/StyleResolver.cpp:2071
> +static bool isFlexBox(EDisplay display)

Nit: maybe isFlexBoxContainer would make more sense?

> Source/WebCore/rendering/RenderBlock.h:81
> +    virtual bool requiresLayer() const OVERRIDE { return !style()->hasAutoZIndex() || RenderBox::requiresLayer(); }

I don't think this is totally right. Some renderers that are flex-items per the spec are not RenderBlocks: mostly all the RenderReplaced renderers (<img>, <applet>, <object>, <embed>, <iframe> ...).

All in all, we are better off moving the hasAutoZIndex() check in RenderBox which would cover all the above cases (assuming svg elements cannot be flex-items which I didn't see mentioned in the spec).

> LayoutTests/css3/flexbox/z-index-expected.html:7
> +    <div style="position: absolute; top: 25px; left: 25px; height: 25px; width: 50px; z-index: 50; background-color: purple"></div>

I would hoist the common style into a style sheet for readability (like the width / height / position part)

[Tony Chang]

Comment on attachment 157539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157539&action=review

> Source/WebCore/css/StyleResolver.cpp:2161
> +        if (isFlexBox(parentStyle->display())) {

I like this check a lot better than the old one.

Is it possible for parentStyle to be NULL (e.g., when detached from the document)?  Should the table writing mode check above also check for a NULL parentStyle?

[Ojan Vafai]

Comment on attachment 157539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157539&action=review

>> Source/WebCore/css/StyleResolver.cpp:2071
>> +static bool isFlexBox(EDisplay display)
> 
> Nit: maybe isFlexBoxContainer would make more sense?

I went isDisplayFlexibleBox.

>> Source/WebCore/css/StyleResolver.cpp:2161
>> +        if (isFlexBox(parentStyle->display())) {
> 
> I like this check a lot better than the old one.
> 
> Is it possible for parentStyle to be NULL (e.g., when detached from the document)?  Should the table writing mode check above also check for a NULL parentStyle?

Good point. Looking at the calling code, it can be null in theory. Fix this case and the table writing mode case.

>> Source/WebCore/rendering/RenderBlock.h:81
>> +    virtual bool requiresLayer() const OVERRIDE { return !style()->hasAutoZIndex() || RenderBox::requiresLayer(); }
> 
> I don't think this is totally right. Some renderers that are flex-items per the spec are not RenderBlocks: mostly all the RenderReplaced renderers (<img>, <applet>, <object>, <embed>, <iframe> ...).
> 
> All in all, we are better off moving the hasAutoZIndex() check in RenderBox which would cover all the above cases (assuming svg elements cannot be flex-items which I didn't see mentioned in the spec).

Good call. Moved to RenderBox.

>> LayoutTests/css3/flexbox/z-index-expected.html:7
>> +    <div style="position: absolute; top: 25px; left: 25px; height: 25px; width: 50px; z-index: 50; background-color: purple"></div>
> 
> I would hoist the common style into a style sheet for readability (like the width / height / position part)

Will do.

[Ojan Vafai]

Created attachment 157625 [details]
Patch

[Build Bot]

Comment on attachment 157625 [details]
Patch

Attachment 157625 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13470463

[Tony Chang]

(In reply to comment #14)
> > Is it possible for parentStyle to be NULL (e.g., when detached from the document)?  Should the table writing mode check above also check for a NULL parentStyle?
> 
> Good point. Looking at the calling code, it can be null in theory. Fix this case and the table writing mode case.

I tried to make a repro case for you where parentStyle is NULL, but I don't think it's possible.

adjustRenderStyle has 2 callers.
- styleForElement sets m_parentStyle to a clone of style()
- pseudoStyleForElement seems to check for a null parentStyle, but it only has a single caller in RenderObject.cpp.
  - in getUncachedPseudoStyle, if parentStyle is NULL, it assigns style() to parentStyle.

We could just have a follow up patch that cleans up pseudoStyleForElement.  It's confusing that parentStyle has a default arg of 0.

[Ojan Vafai]

(In reply to comment #17)
> adjustRenderStyle has 2 callers.
> - styleForElement sets m_parentStyle to a clone of style()
> - pseudoStyleForElement seems to check for a null parentStyle, but it only has a single caller in RenderObject.cpp.
>   - in getUncachedPseudoStyle, if parentStyle is NULL, it assigns style() to parentStyle.

Good digging. I didn't look at the caller of pseudoStyleForElement. https://bugs.webkit.org/show_bug.cgi?id=93730

[Ojan Vafai]

Created attachment 157776 [details]
Patch

[Tony Chang]

Comment on attachment 157776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157776&action=review

> Source/WebCore/rendering/RenderBox.h:47
> +    virtual bool requiresLayer() const OVERRIDE { return isRoot() || isOutOfFlowPositioned() || isRelPositioned() || isTransparent() || hasOverflowClip() || hasTransform() || hasHiddenBackface() || hasMask() || hasReflection() || hasFilter() || style()->specifiesColumns() || !style()->hasAutoZIndex(); }

Nit: Do we need the isOutOfFlowPositioned() || isRelPositioned() part of the check now?

[Tony Chang]

(In reply to comment #20)
> (From update of attachment 157776 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157776&action=review
> 
> > Source/WebCore/rendering/RenderBox.h:47
> > +    virtual bool requiresLayer() const OVERRIDE { return isRoot() || isOutOfFlowPositioned() || isRelPositioned() || isTransparent() || hasOverflowClip() || hasTransform() || hasHiddenBackface() || hasMask() || hasReflection() || hasFilter() || style()->specifiesColumns() || !style()->hasAutoZIndex(); }
> 
> Nit: Do we need the isOutOfFlowPositioned() || isRelPositioned() part of the check now?

Oh, I forgot about StickyPosition.  I guess StickyPosition does not require a layer.

[Julien Chaffraix]

Comment on attachment 157776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157776&action=review

>>> Source/WebCore/rendering/RenderBox.h:47
>>> +    virtual bool requiresLayer() const OVERRIDE { return isRoot() || isOutOfFlowPositioned() || isRelPositioned() || isTransparent() || hasOverflowClip() || hasTransform() || hasHiddenBackface() || hasMask() || hasReflection() || hasFilter() || style()->specifiesColumns() || !style()->hasAutoZIndex(); }
>> 
>> Nit: Do we need the isOutOfFlowPositioned() || isRelPositioned() part of the check now?
> 
> Oh, I forgot about StickyPosition.  I guess StickyPosition does not require a layer.

Yes, we need the isOutOfFlowPositoned() || isRelPositioned() part. See the placing / shifting through RenderLayer::setStatic{Block|Inline}Position and REnderLayer::relativePositionOffset.

For position: sticky, that part of the change hasn't landed. See bug 90046.

> LayoutTests/css3/flexbox/z-index-expected.html:22
> +    <div class="small" style="z-index: 50; background-color: purple; top: 25px;"></div>

Could you add some testing for the replaced elements that you had missed in your first patch?

[Ojan Vafai]

(In reply to comment #22)
> > LayoutTests/css3/flexbox/z-index-expected.html:22
> > +    <div class="small" style="z-index: 50; background-color: purple; top: 25px;"></div>
> 
> Could you add some testing for the replaced elements that you had missed in your first patch?

Sigh. Good call. img elements work, but iframe elements don't for some reason. I wonder if we're wrapping them in an anonymous node.

[Ojan Vafai]

(In reply to comment #23)
> (In reply to comment #22)
> > > LayoutTests/css3/flexbox/z-index-expected.html:22
> > > +    <div class="small" style="z-index: 50; background-color: purple; top: 25px;"></div>
> > 
> > Could you add some testing for the replaced elements that you had missed in your first patch?
> 
> Sigh. Good call. img elements work, but iframe elements don't for some reason. I wonder if we're wrapping them in an anonymous node.

Turns out iframes don't work at all as flex items. Will file a separate bug and commit this with just the img test case.

[Ojan Vafai]

Committed r125693: <http://trac.webkit.org/changeset/125693>

[Simon Fraser (smfr)]

Comment on attachment 157776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157776&action=review

>>>> Source/WebCore/rendering/RenderBox.h:47
>>>> +    virtual bool requiresLayer() const OVERRIDE { return isRoot() || isOutOfFlowPositioned() || isRelPositioned() || isTransparent() || hasOverflowClip() || hasTransform() || hasHiddenBackface() || hasMask() || hasReflection() || hasFilter() || style()->specifiesColumns() || !style()->hasAutoZIndex(); }
>>> 
>>> Nit: Do we need the isOutOfFlowPositioned() || isRelPositioned() part of the check now?
>> 
>> Oh, I forgot about StickyPosition.  I guess StickyPosition does not require a layer.
> 
> Yes, we need the isOutOfFlowPositoned() || isRelPositioned() part. See the placing / shifting through RenderLayer::setStatic{Block|Inline}Position and REnderLayer::relativePositionOffset.
> 
> For position: sticky, that part of the change hasn't landed. See bug 90046.

Adding !style()->hasAutoZIndex() was a terrible change to make. It interacts badly with -webkit-overflow-scrolling: touch, which makes all elements with non-hidden overflow have non-auto z-index.

Also, why isn't this code in a RenderFlex* subclass?

[Tony Chang]

(In reply to comment #26)
> Adding !style()->hasAutoZIndex() was a terrible change to make. It interacts badly with -webkit-overflow-scrolling: touch, which makes all elements with non-hidden overflow have non-auto z-index.
> 

Can you provide a html snippet that demonstrates the problem?  I think you're saying that this makes all -webkit-overflow-scrolling: touch boxes become layers even if they don't need to?

> Also, why isn't this code in a RenderFlex* subclass?

It applies to children of the flexbox, not the flexbox itself.

[Simon Fraser (smfr)]

(In reply to comment #27)
> (In reply to comment #26)
> > Adding !style()->hasAutoZIndex() was a terrible change to make. It interacts badly with -webkit-overflow-scrolling: touch, which makes all elements with non-hidden overflow have non-auto z-index.
> > 
> 
> Can you provide a html snippet that demonstrates the problem?  I think you're saying that this makes all -webkit-overflow-scrolling: touch boxes become layers even if they don't need to?

Yes, but I'm fixing that via bug 118337.

> > Also, why isn't this code in a RenderFlex* subclass?
> 
> It applies to children of the flexbox, not the flexbox itself.

Julien explained this to me; we think the current code is correct (but possibly surprising).