Bug 91405

Created attachment 157308 [details] Work in progress

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.

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?

(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).

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

(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.

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.

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

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.

Created attachment 157539 [details] Patch

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

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 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 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.

Created attachment 157625 [details] Patch

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

(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.

(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

Created attachment 157776 [details] Patch

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?

(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 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?

(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.

(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.

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

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?

(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.

(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).