[webkit-reviews] review granted: [Bug 15562] getComputedStyle returns "auto" for zIndex property even after it has been set, on non-positioned elements : [Attachment 383942] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 15562: getComputedStyle returns "auto" for zIndex property even after it
has been set, on non-positioned elements
https://bugs.webkit.org/show_bug.cgi?id=15562

Attachment 383942: Patch

https://bugs.webkit.org/attachment.cgi?id=383942&action=review




--- Comment #16 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 383942
  --> https://bugs.webkit.org/attachment.cgi?id=383942
Patch

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

> Source/WebCore/ChangeLog:12
> +	   Fix by storing the specified value in RenderStyle as "zIndex", and
the used value as "effectiveZIndex", and
> +	   converting all rendering code to use the "effective" variants.
getComputedStyle reads "zIndex".

Are we going to end up with specified/effective values for many properties?
Should we come up with some sort of generic solution to this problem, like
generating a separate RenderStyle for getComputedStyle to read?

> Source/WebCore/rendering/style/StyleBoxData.h:57
>      int zIndex() const { return m_zIndex; }
>      bool hasAutoZIndex() const { return m_hasAutoZIndex; }

It might be good to rename these to 'original' (or 'specified'), similar to
display properties.

> Source/WebCore/rendering/style/StyleBoxData.h:60
> +    bool effectiveZIndexIsAuto() const { return m_effectiveZIndexIsAuto; }

The usual naming for boolean properties is to put the verb first,
'hasEffectiveZIndexAuto'. In any case both these should follow the same naming
pattern.

> Source/WebCore/style/StyleAdjuster.cpp:300
> +    if (style.hasAutoZIndex() || (style.position() == PositionType::Static
&& !m_parentBoxStyle.isDisplayFlexibleOrGridBox()))
> +	   style.setEffectiveZIndexIsAuto();
> +    else
> +	   style.setEffectiveZIndex(style.zIndex());

I suppose an alternative strategy would be to compute the effective value
dynamically in render tree.