Bug 91405

Summary: z-index should work without position on flexitems
Product: WebKit Reporter: Tony Chang <tony>
Component: Layout and RenderingAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, eoconnor, eric, hyatt, jamesr, jchaffraix, macpherson, menard, ojan, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62048    
Attachments:
Description Flags
Work in progress
none
Patch
none
Patch
none
Patch tony: review+, jchaffraix: commit-queue-

Description Tony Chang 2012-07-16 11:07:10 PDT
This was recently added to the spec: http://dev.w3.org/csswg/css3-flexbox/#painting
Comment 1 Ojan Vafai 2012-08-08 15:22:06 PDT
Created attachment 157308 [details]
Work in progress
Comment 2 Ojan Vafai 2012-08-08 15:24:14 PDT
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.
Comment 3 Ojan Vafai 2012-08-08 15:47:30 PDT
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?
Comment 4 Tony Chang 2012-08-08 18:41:08 PDT
(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).
Comment 5 Tony Chang 2012-08-08 18:42:47 PDT
Don't we create layers dynamically? E.g., when adding scrollbars or when dynamically making a block position:absolute?
Comment 6 Ojan Vafai 2012-08-08 19:08:28 PDT
(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.
Comment 7 Ojan Vafai 2012-08-09 13:30:25 PDT
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.
Comment 8 Simon Fraser (smfr) 2012-08-09 13:32:28 PDT
If this is too gross we can push back on the spec change.
Comment 9 Ojan Vafai 2012-08-09 13:35:20 PDT
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.
Comment 10 Ojan Vafai 2012-08-09 13:52:24 PDT
Created attachment 157539 [details]
Patch
Comment 11 Build Bot 2012-08-09 14:29:48 PDT
Comment on attachment 157539 [details]
Patch

Attachment 157539 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13470365
Comment 12 Julien Chaffraix 2012-08-09 16:17:41 PDT
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)
Comment 13 Tony Chang 2012-08-09 16:49:35 PDT
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?
Comment 14 Ojan Vafai 2012-08-09 20:00:48 PDT
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.
Comment 15 Ojan Vafai 2012-08-09 20:30:26 PDT
Created attachment 157625 [details]
Patch
Comment 16 Build Bot 2012-08-09 21:08:13 PDT
Comment on attachment 157625 [details]
Patch

Attachment 157625 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13470463
Comment 17 Tony Chang 2012-08-09 23:17:48 PDT
(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.
Comment 18 Ojan Vafai 2012-08-10 11:36:00 PDT
(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
Comment 19 Ojan Vafai 2012-08-10 11:40:49 PDT
Created attachment 157776 [details]
Patch
Comment 20 Tony Chang 2012-08-12 16:39:57 PDT
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?
Comment 21 Tony Chang 2012-08-12 17:16:38 PDT
(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.
Comment 22 Julien Chaffraix 2012-08-13 07:31:34 PDT
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?
Comment 23 Ojan Vafai 2012-08-15 12:15:57 PDT
(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.
Comment 24 Ojan Vafai 2012-08-15 12:23:38 PDT
(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.
Comment 25 Ojan Vafai 2012-08-15 12:28:30 PDT
Committed r125693: <http://trac.webkit.org/changeset/125693>
Comment 26 Simon Fraser (smfr) 2013-07-02 15:17:02 PDT
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?
Comment 27 Tony Chang 2013-07-02 15:48:41 PDT
(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.
Comment 28 Simon Fraser (smfr) 2013-07-02 20:19:12 PDT
(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).