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.
Created attachment 130372 [details] Patch
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.
Created attachment 130427 [details] Patch
(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.
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.
Created attachment 130446 [details] Patch
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.
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.
Created attachment 130453 [details] Patch
Comment on attachment 130453 [details] Patch Clearing flags on attachment: 130453 Committed r109968: <http://trac.webkit.org/changeset/109968>
All reviewed patches have been landed. Closing bug.