getComputedStyle for border-width is not implemented.
Created attachment 119469 [details] Patch
Comment on attachment 119469 [details] Patch Attachment 119469 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10903321
Comment on attachment 119469 [details] Patch Attachment 119469 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10904348
Comment on attachment 119469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119469&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2166 > + RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); > + list->append(zoomAdjustedPixelValue(style->borderTop().width(), style.get(), cssValuePool)); When I added this for flex-flow, I didn't realize that getComputedStyle wasn't implemented for other short hand properties. I think adding this is a good idea, but we should make some helper functions for this rather than duplicating code (e.g., the code for margin width would be almost identical). > LayoutTests/ChangeLog:11 > + * fast/css/getComputedStyle/getComputedStyle-border-width-expected.txt: Added. > + * fast/css/getComputedStyle/getComputedStyle-border-width.html: Added. Do you need to also fix up fast/css/getComputedStyle/computed-style* and svg/css/getComputedStyle-basic? > LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width.html:16 > +testContainer.innerHTML = '<div id="test" style="border-width: 10px 5px 4px 3px; border-style: solid solid;">hello</div>'; Can we have some other units in the test case? It would also be interesting to see the setting of border-top and making sure the value makes it out when calling getComputedStyle on border-width.
(In reply to comment #4) > (From update of attachment 119469 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119469&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2166 > > + RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); > > + list->append(zoomAdjustedPixelValue(style->borderTop().width(), style.get(), cssValuePool)); > > When I added this for flex-flow, I didn't realize that getComputedStyle wasn't implemented for other short hand properties. I think adding this is a good idea, but we should make some helper functions for this rather than duplicating code (e.g., the code for margin width would be almost identical). Yep, I was thinking of going step by step and implement each one after each one, group in helper function when the needs come. > > > LayoutTests/ChangeLog:11 > > + * fast/css/getComputedStyle/getComputedStyle-border-width-expected.txt: Added. > > + * fast/css/getComputedStyle/getComputedStyle-border-width.html: Added. > > Do you need to also fix up fast/css/getComputedStyle/computed-style* and svg/css/getComputedStyle-basic? Unless I'm mistaken it doesn't seem so. > > > LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width.html:16 > > +testContainer.innerHTML = '<div id="test" style="border-width: 10px 5px 4px 3px; border-style: solid solid;">hello</div>'; > > Can we have some other units in the test case? It would also be interesting to see the setting of border-top and making sure the value makes it out when calling getComputedStyle on border-width. Will fix.
Created attachment 119493 [details] Patch
Comment on attachment 119493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119493&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2163 > + if (style->borderTopStyle() == BNONE || style->borderTopStyle() == BHIDDEN) > + return cssValuePool->createValue(0, CSSPrimitiveValue::CSS_PX); Why does this only check the top? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2169 > + list->append(zoomAdjustedPixelValue(style->borderTop().width(), style.get(), cssValuePool)); > + list->append(zoomAdjustedPixelValue(style->borderRight().width(), style.get(), cssValuePool)); > + list->append(zoomAdjustedPixelValue(style->borderBottom().width(), style.get(), cssValuePool)); > + list->append(zoomAdjustedPixelValue(style->borderLeft().width(), style.get(), cssValuePool)); Can you just use style->borderTopWidth() like the above code for CSSPropertyBorderTopWidth? I think that avoids the need to check border*Style() above? > LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width.html:42 > +e.style.borderWidth="" > +e.style.borderTop = "10px solid" > +shouldBe("computedStyle.getPropertyValue('border-width')", "'10px 3px 3px 3px'"); Shouldn't right, bottom, left all be 0px here?
Created attachment 119624 [details] Patch
Comment on attachment 119624 [details] Patch Clearing flags on attachment: 119624 Committed r103096: <http://trac.webkit.org/changeset/103096>
All reviewed patches have been landed. Closing bug.