Bug 18294

Summary:

Strange Result for getComputedStyle on borderWidth set in em

Product:

WebKit

Reporter:

Garrett Smith <xk1t>

Component:

DOM

Assignee:

Alexis Menard (darktears) <menard>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, darin, macpherson, menard, tony, webkit.review.bot

Priority:

Version:

523.x (Safari 3)

Hardware:

OS:

OS X 10.5

URL:

http://dhtmlkitchen.com/ape/test/tests/dom/style-f-test.html

Attachments:

Description	Flags
First attempt	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none

Garrett Smith

Reported 2008-04-02 23:15:28 PDT

I had a problem getting the computed style of a borderWidth in webkit. It looks like a bug. testGetComputedShorthandValues100px : function() { var c1 = document.getElementById("c1"), inp = "120em 110em 100em 90.1em"; c1.style.borderWidth = inp; c1.style.borderStyle = "solid"; c1.style.fontSize = "100px"; var cs = getComputedStyle(c1, ""); //alert([cs.borderTopWidth, cs.borderRightWidth, cs.borderBottomWidth, cs.borderLeftWidth]); var out = dom.getStyle(c1, "borderWidth"); Assert.areEqual("12000px 11000px 10000px 9010px", out); } Safari 3.1 fails with "3808px 2808px 1808px 818px, (string)" The html can be simplified to: <body style="position: relative;margin:0;padding:0;"><div id="c1"></ div>l</body> In Firefox, I get the expexted result. I don't understand webkit's result. What's going on?

Attachments
First attempt (3.51 KB, patch) 2008-06-25 00:01 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (20.48 KB, patch) 2012-01-13 09:46 PST, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (22.65 KB, patch) 2012-01-13 12:57 PST, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch (23.36 KB, patch) 2012-01-18 02:31 PST, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch for landing (23.61 KB, patch) 2012-01-18 13:27 PST, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Patch for landing (23.62 KB, patch) 2012-01-19 03:40 PST, Alexis Menard (darktears)	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Rob Buis

Comment 1 2008-06-25 00:01:13 PDT

Created attachment 21925 [details] First attempt This simple patch allows bigger computed border widths that use em. (for some reason the WebCore/ChangeLog) did not pick up the tests). Cheers, Rob.

Darin Adler

Comment 2 2008-06-27 09:12:33 PDT

Comment on attachment 21925 [details] First attempt Why is 28 bits enough? Doesn't this just move the problem to a higher number?

Rob Buis

Comment 3 2008-07-09 12:52:44 PDT

Hi Darin, (In reply to comment #2) > (From update of attachment 21925 [details] [edit]) > Why is 28 bits enough? Doesn't this just move the problem to a higher number? Sure, just moving the problem, but we have to have some boundary I think. There is something wrong with the patch I noticed, when setting we use a short so the extra room gets lost. When I correct that the value can go up to 2684354em at 100px font size, which is more than FF. However it seems the zoom factor is also multiplied into the border widths, which means the border width values differ on zoom level, unlike FF. I am not sure what is correct. Cheers, Rob.

Eric Seidel (no email)

Comment 4 2008-07-24 23:03:41 PDT

Comment on attachment 21925 [details] First attempt It would be even better if this was a .js style test and it tested both sides of the expected failure point.

Eric Seidel (no email)

Comment 5 2008-08-01 15:37:01 PDT

Comment on attachment 21925 [details] First attempt r- for needing a better test case.

Alexis Menard (darktears)

Comment 6 2012-01-13 09:46:57 PST

Created attachment 122444 [details] Patch

Alexis Menard (darktears)

Comment 7 2012-01-13 12:57:05 PST

Created attachment 122486 [details] Patch

Tony Chang

Comment 8 2012-01-17 10:48:55 PST

Comment on attachment 122486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122486&action=review I think you're right that it's safe to use more bits because the size isn't changing (we're probably 4 byte aligning), but we should add a compile assert to verify. Color also packs poorly because of the bool it contains, but there's probably no way to get that back. > Source/WebCore/css/CSSStyleApplyProperty.cpp:1710 > + setPropertyHandler(CSSPropertyOutlineWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::outlineWidth, &RenderStyle::setOutlineWidth, &RenderStyle::initialOutlineWidth, NormalDisabled, ThicknessEnabled>::createHandler()); > + setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialOutlineWidth, NormalDisabled, ThicknessEnabled>::createHandler()); Should this be initialColumnWidth or initialColumnRuleWidth? This fix seems unrelated to this patch. > Source/WebCore/page/animation/AnimationBase.cpp:87 > +static inline unsigned blendFunc(const AnimationBase*, unsigned from, unsigned to, double progress) Is there a version of this function using "unsigned short" that we can remove? > Source/WebCore/rendering/RenderTheme.cpp:114 > + if (static_cast<unsigned>(borderBox.top().value()) != style->borderTopWidth()) { I would cast border*Width to an int because it's going to fit (since it's only 27 bits) and Length::value() could be negative. That said, this still makes me a bit nervous.

Alexis Menard (darktears)

Comment 9 2012-01-17 11:03:31 PST

(In reply to comment #8) > (From update of attachment 122486 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122486&action=review > > I think you're right that it's safe to use more bits because the size isn't changing (we're probably 4 byte aligning), but we should add a compile assert to verify. I can add it. > > Color also packs poorly because of the bool it contains, but there's probably no way to get that back. > > > Source/WebCore/css/CSSStyleApplyProperty.cpp:1710 > > + setPropertyHandler(CSSPropertyOutlineWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::outlineWidth, &RenderStyle::setOutlineWidth, &RenderStyle::initialOutlineWidth, NormalDisabled, ThicknessEnabled>::createHandler()); > > + setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialOutlineWidth, NormalDisabled, ThicknessEnabled>::createHandler()); > > Should this be initialColumnWidth or initialColumnRuleWidth? This fix seems unrelated to this patch. It's because the template requires a unsigned short defined function. I changed initialBorderWidth to return a unsigned therefore it doesn't compile. I created initialOutlineWidth which is unsigned short (and return the same as initialBorderWidth) for it. > > > Source/WebCore/page/animation/AnimationBase.cpp:87 > > +static inline unsigned blendFunc(const AnimationBase*, unsigned from, unsigned to, double progress) > > Is there a version of this function using "unsigned short" that we can remove? I guess no as we still have properties using short. > > > Source/WebCore/rendering/RenderTheme.cpp:114 > > + if (static_cast<unsigned>(borderBox.top().value()) != style->borderTopWidth()) { > > I would cast border*Width to an int because it's going to fit (since it's only 27 bits) and Length::value() could be negative. That said, this still makes me a bit nervous. why exactly?

Alexis Menard (darktears)

Comment 10 2012-01-18 02:31:31 PST

Created attachment 122893 [details] Patch

Tony Chang

Comment 11 2012-01-18 12:34:42 PST

Comment on attachment 122893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122893&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:1706 > + setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialOutlineWidth, NormalDisabled, ThicknessEnabled>::createHandler()); Can we also add a method to render style for initialColumnRuleWidth? It's confusing to use initialOutlineWidth here.

Alexis Menard (darktears)

Comment 12 2012-01-18 13:27:59 PST

Created attachment 122980 [details] Patch for landing

Rafael Brandao

Comment 13 2012-01-18 14:38:47 PST

Comment on attachment 122980 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=122980&action=review > Source/WebCore/platform/animation/AnimationUtilities.h:40 > + return static_cast<int>(lround(static_cast<double>(from) + static_cast<double>(to - from) * progress)); Shouldn't this use static_cast<unsigned> instead?

Tony Chang

Comment 14 2012-01-18 14:47:57 PST

Comment on attachment 122980 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=122980&action=review >> Source/WebCore/platform/animation/AnimationUtilities.h:40 >> + return static_cast<int>(lround(static_cast<double>(from) + static_cast<double>(to - from) * progress)); > > Shouldn't this use static_cast<unsigned> instead? Yes, you're right.

Alexis Menard (darktears)

Comment 15 2012-01-19 03:40:54 PST

Created attachment 123097 [details] Patch for landing

WebKit Review Bot

Comment 16 2012-01-19 05:00:08 PST

Comment on attachment 123097 [details] Patch for landing Clearing flags on attachment: 123097 Committed r105403: <http://trac.webkit.org/changeset/105403>

WebKit Review Bot

Comment 17 2012-01-19 05:00:14 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.