[webkit-reviews] review canceled: [Bug 91405] z-index should work without position on flexitems : [Attachment 157539] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Ojan Vafai
<ojan at chromium.org>'s request for review:
Bug 91405: z-index should work without position on flexitems
https://bugs.webkit.org/show_bug.cgi?id=91405

Attachment 157539: Patch
https://bugs.webkit.org/attachment.cgi?id=157539&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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)