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: | P2 | ||||||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
URL: | http://dhtmlkitchen.com/ape/test/tests/dom/style-f-test.html | ||||||||||||||||
Attachments: |
|
Description
Garrett Smith
2008-04-02 23:15:28 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.
Comment on attachment 21925 [details]
First attempt
Why is 28 bits enough? Doesn't this just move the problem to a higher number?
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. 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.
Comment on attachment 21925 [details]
First attempt
r- for needing a better test case.
Created attachment 122444 [details]
Patch
Created attachment 122486 [details]
Patch
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. (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? Created attachment 122893 [details]
Patch
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. Created attachment 122980 [details]
Patch for landing
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? 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. Created attachment 123097 [details]
Patch for landing
Comment on attachment 123097 [details] Patch for landing Clearing flags on attachment: 123097 Committed r105403: <http://trac.webkit.org/changeset/105403> All reviewed patches have been landed. Closing bug. |