Bug 74635 – getComputedStyle for border-width is not implemented.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

getComputedStyle for border-width is not implemented.

[Alexis Menard (darktears)]

Created attachment 119469 [details]
Patch

[Early Warning System Bot]

Comment on attachment 119469 [details]
Patch

Attachment 119469 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10903321

[WebKit Review Bot]

Comment on attachment 119469 [details]
Patch

Attachment 119469 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10904348

[Tony Chang]

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.

[Alexis Menard (darktears)]

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

[Alexis Menard (darktears)]

Created attachment 119493 [details]
Patch

[Tony Chang]

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?

[Alexis Menard (darktears)]

Created attachment 119624 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119624 [details]
Patch

Clearing flags on attachment: 119624

Committed r103096: <http://trac.webkit.org/changeset/103096>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.