getComputedStyle returns incorrect values for the width and height of pseudo-elements

• Component: CSS
• Status: RESOLVED FIXED

[Jessie Berlin]

Created attachment 53754 [details]
Test case showing the width and height for the pseudo-element being reported as those of the element it is defined on.

getComputedStyle incorrectly returns the width and height values of the element on which the pseudo-element is defined instead of the values for the pseudo element. This is the same incorrect behavior as exhibited in Firefox 3.6.

[Alexis Menard (darktears)]

Created attachment 130372 [details]
Patch

[Tony Chang]

Comment on attachment 130372 [details]
Patch

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

This doesn't look right. E.g., I think you would get the wrong value if width/height is auto or if min/max width changes the size.  We need to get the right RenderObject and ask it for the right size.

Does this work properly in IE?

> Source/WebCore/ChangeLog:12
> +        No new tests : Extend the existing one for pseudo-elements.

Nit: It's nice to name the test in the ChangeLog.

[Alexis Menard (darktears)]

Created attachment 130427 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #2)
> (From update of attachment 130372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130372&action=review
> 
> This doesn't look right. E.g., I think you would get the wrong value if width/height is auto or if min/max width changes the size.  We need to get the right RenderObject and ask it for the right size.
> 
> Does this work properly in IE?

I don't have IE but it works as it should in FF and Opera.

> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests : Extend the existing one for pseudo-elements.
> 
> Nit: It's nice to name the test in the ChangeLog.

[Tony Chang]

Comment on attachment 130427 [details]
Patch

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

We need more tests for this.  Changing the renderer pointer impacts more than just width and height, we should test as many values as possible.

Also, you should add tests without display:block and with auto sizing.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1344
> +    if (m_pseudoElementSpecifier == AFTER)
> +        renderer = renderer->afterPseudoElementRenderer();
> +
> +    if (m_pseudoElementSpecifier == BEFORE)

else if since we can't be both AFTER and BEFORE.

[Alexis Menard (darktears)]

Created attachment 130446 [details]
Patch

[Tony Chang]

Comment on attachment 130446 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1344
> +    if (m_pseudoElementSpecifier == AFTER)
> +        renderer = renderer->afterPseudoElementRenderer();
> +    else if (m_pseudoElementSpecifier == BEFORE)
> +        renderer = renderer->beforePseudoElementRenderer();

Is it possible for renderer to be NULL?  E.g., if the node is display: none.  Can you add a test for this case?

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html:55
> +      #testBeforeAfterTable::before {

Did you mean to have 2 #testBeforeAfterTable::before sections?

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html:64
> +      #testBeforeAfterTable::after {

Did you mean to have 2 #testBeforeAfterTable::after sections?

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html:163
> +        { 'elementId' : 'testBeforeAfterInline', 'pseudoElement' : ':before', 'property' : 'height', 'expectedValue' : 'auto' },
> +        { 'elementId' : 'testBeforeAfterInline', 'pseudoElement' : ':before', 'property' : 'width', 'expectedValue' : 'auto' },
> +        { 'elementId' : 'testBeforeAfterInline', 'pseudoElement' : ':after', 'property' : 'height', 'expectedValue' : 'auto' },
> +        { 'elementId' : 'testBeforeAfterInline', 'pseudoElement' : ':after', 'property' : 'width', 'expectedValue' : 'auto' },

These should return the computed value, right?  Maybe just add a FIXME here so we know that we don't actually want to return auto.  It would also be OK for the test to output FAIL.

[Tony Chang]

Comment on attachment 130446 [details]
Patch

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

>> LayoutTests/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html:163
>> +        { 'elementId' : 'testBeforeAfterInline', 'pseudoElement' : ':after', 'property' : 'width', 'expectedValue' : 'auto' },
> 
> These should return the computed value, right?  Maybe just add a FIXME here so we know that we don't actually want to return auto.  It would also be OK for the test to output FAIL.

Oh, nevermind, we always return auto on inline blocks.  This looks correct.

[Alexis Menard (darktears)]

Created attachment 130453 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130453 [details]
Patch

Clearing flags on attachment: 130453

Committed r109968: <http://trac.webkit.org/changeset/109968>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.