Bug 15562

Summary: getComputedStyle returns "auto" for zIndex property even after it has been set, on non-positioned elements
Product: WebKit Reporter: Marcus Better <marcus>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: arv, bdakin, cdumez, commit-queue, dbates, dino, dstockwell, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, ian, jfernandez, joepeck, koivisto, kondapallykalyan, macpherson, menard, m.goleb+bugzilla, michaelyagudaev, mifenton, mitz, mrowe, pdr, rego, sam, simon.fraser, svillar, tkent, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 523.x (Safari 3)   
Hardware: All   
OS: OS X 10.4   
Attachments:
Description Flags
Test case
none
Patch
koivisto: review+
Patch
none
Archive of layout-test-results from ews212 for win-future none

Description Marcus Better 2007-10-19 07:27:10 PDT
If a style attribute is set in script, that should become the computed value for the attribute. The getComputedStyle function should therefore return the new value. However this is not the case with WebKit.
Comment 1 Marcus Better 2007-10-19 07:29:14 PDT
Created attachment 16730 [details]
Test case

This checks the computed and inline zIndex style attributes for an element without an explicit style. Then the attribute is given a value with JavaScript. We then check if the computed style has been changed to reflect this.
Comment 2 Marcus Better 2007-10-19 07:29:47 PDT
Test result on Firefox 2.0.0.7:

Before:
computed: auto
inline:
Setting z-index to 10...
computed: 10
inline: 10
SUCCESS!
Comment 3 Marcus Better 2007-10-19 07:31:11 PDT
Test result on WebKit r26570 nightly:

Before:
computed: auto
inline: 
Setting z-index to 10...
computed: auto
inline: 10
FAILURE! Computed and inline style differ
Comment 4 Mark Rowe (bdash) 2007-10-20 09:08:46 PDT
This is related to the fact that the div on which the z-index is set is unpositioned.  Adding "            e.style.position = "relative";" before retrieving the final computed style results in the expected value being returned.
Comment 5 Mark Rowe (bdash) 2007-10-20 09:19:30 PDT
See http://bugs.webkit.org/show_bug.cgi?id=14321#c4 for why it returns auto when the element is not positioned.  Mitz, should we consider matching Firefox's behaviour here?
Comment 6 Dave Hyatt 2007-10-20 15:32:29 PDT
I don't think we should.  This is a bug in Firefox I believe.  The computed result of z-index for an unpositioned element is auto, not the value specified in the stylesheet (since that value is going to be completely ignored when rendering).

cc'ing Hixie to see what he thinks.
Comment 7 Ian 'Hixie' Hickson 2007-10-23 01:06:17 PDT
Actually the computed value of 'z-index' is, according to CSS2.1, always the specified value. The _used value_ is the one that depends on the other properties
in this case.
Comment 8 Eric Seidel (no email) 2007-12-31 21:18:13 PST
This is required by Acid3.  In fact, Hixie seems to have used this for 8 acid 3 tests. :(
Comment 9 Eric Seidel (no email) 2008-01-01 01:39:28 PST
Test 32: FAIL (underlying problems prevent this test from running properly)
Test 33: FAIL (underlying problems prevent this test from running properly)
Test 34: FAIL (underlying problems prevent this test from running properly)
Test 35: FAIL (underlying problems prevent this test from running properly)
Test 36: FAIL (underlying problems prevent this test from running properly)
Test 37: FAIL (underlying problems prevent this test from running properly)
Test 38: FAIL (underlying problems prevent this test from running properly)

I lied, it's only 7 Acid3 tests which depend on this bug.  It's kinda lame that 7 tests would exploit this bug, but oh well.
Comment 10 Ian 'Hixie' Hickson 2008-01-01 05:51:03 PST
Wow, that was totally accidental. I only used z-index because it was the only CSS2 property I could find that only took keywords and integers, which sidesteps many of the issues with getting stringified computed values. If you have any other suggestions for how to write that part of the test harness in a way that is justifiable using 2004-or-earlier CRs or RECs, I'm happy to oblige.
Comment 11 Ian 'Hixie' Hickson 2008-01-01 14:24:48 PST
Note that it's going to be 16 tests by the time I'm doing with this series of tests.
Comment 12 Eric Seidel (no email) 2008-01-30 01:10:52 PST
Acid3 no longer depends on this
Comment 13 Michael Yagudaev 2011-08-18 08:12:30 PDT
Ok I believe I figured out the reason for this bug. 

If you read the css3 specification for z-index (http://www.w3.org/TR/CSS2/visuren.html#z-index) it says that the default value for a z-index is auto. Now, if an element is not applied with a position type that is non-static (i.e. position is relative, absolute or fixed), static is default for position so the z-index will have no effect on the element. So it seems webkit does not bother calculating this z-index and just returns the default, which is auto.

The work around to fix this therefore is
[selector]
{
   position: relative;
}

If you want to get the value of the container then use:
[selector]
{
   position: relative;
   z-index: inherit;
}

The above is really useful when implementing loading spinners for your content (e.g. a spinner that appears on top of a dialog, simply get the z-index and increment by one).
Comment 14 Michael Yagudaev 2011-08-20 18:20:43 PDT
Posted a short article on how to get the z-index cross browsers, which also includes a jsfiddle example you can play around with:
http://www.yagudaev.com/programming/javascript/20-getting-reliable-z-index-cross-browser

(In reply to comment #13)
> Ok I believe I figured out the reason for this bug. 
> 
> If you read the css3 specification for z-index (http://www.w3.org/TR/CSS2/visuren.html#z-index) it says that the default value for a z-index is auto. Now, if an element is not applied with a position type that is non-static (i.e. position is relative, absolute or fixed), static is default for position so the z-index will have no effect on the element. So it seems webkit does not bother calculating this z-index and just returns the default, which is auto.
> 
> The work around to fix this therefore is
> [selector]
> {
>    position: relative;
> }
> 
> If you want to get the value of the container then use:
> [selector]
> {
>    position: relative;
>    z-index: inherit;
> }
> 
> The above is really useful when implementing loading spinners for your content (e.g. a spinner that appears on top of a dialog, simply get the z-index and increment by one).
Comment 15 Simon Fraser (smfr) 2019-11-19 20:49:53 PST
Created attachment 383942 [details]
Patch
Comment 16 Antti Koivisto 2019-11-20 01:32:57 PST
Comment on attachment 383942 [details]
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.
Comment 17 Simon Fraser (smfr) 2019-11-20 11:44:33 PST
(In reply to Antti Koivisto from comment #16)
> Comment on attachment 383942 [details]
> 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?

There probably will be more. When that happens we should figure out how to handle it.

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

I'll use "specified" and "used" to match CSS terminology.

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

OK

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

I considered that, but it's going to be slower, and in several places we set the used value to 0 to get stacking side-effects.
Comment 18 Simon Fraser (smfr) 2019-11-20 13:05:29 PST
Created attachment 383979 [details]
Patch
Comment 19 EWS Watchlist 2019-11-20 15:26:26 PST
Comment on attachment 383979 [details]
Patch

Attachment 383979 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13269917

New failing tests:
js/dom/customAccessor-defineProperty.html
Comment 20 EWS Watchlist 2019-11-20 15:26:28 PST
Created attachment 383994 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 21 WebKit Commit Bot 2019-11-20 17:47:38 PST
The commit-queue encountered the following flaky tests while processing attachment 383979 [details]:

media/modern-media-controls/compact-media-controls/compact-media-controls-constructor.html bug 193587 (author: graouts@apple.com)
The commit-queue is continuing to process your patch.
Comment 22 WebKit Commit Bot 2019-11-20 17:48:34 PST
Comment on attachment 383979 [details]
Patch

Clearing flags on attachment: 383979

Committed r252724: <https://trac.webkit.org/changeset/252724>
Comment 23 Simon Fraser (smfr) 2019-12-03 09:27:37 PST
*** Bug 203390 has been marked as a duplicate of this bug. ***
Comment 24 Radar WebKit Bug Importer 2019-12-20 14:30:47 PST
<rdar://problem/58125677>