[webkit-reviews] review denied: [Bug 29084] getComputedStyle returns percentage values for left / right / top / bottom : [Attachment 190170] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 8 13:15:01 PST 2013
Eric Seidel <eric at webkit.org> has denied Tim 'mithro' Ansell
<mithro at mithis.com>'s request for review:
Bug 29084: getComputedStyle returns percentage values for left / right / top /
bottom
https://bugs.webkit.org/show_bug.cgi?id=29084
Attachment 190170: Patch
https://bugs.webkit.org/attachment.cgi?id=190170&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190170&action=review
OK. This change looks great. My only concern is about testing. There are a
lot of cases here. I'm not sure you covered position: sticky. You shoudl be
aware of the many types of lenghts:
enum LengthType {
Auto, Relative, Percent, Fixed,
Intrinsic, MinIntrinsic,
MinContent, MaxContent, FillAvailable, FitContent,
Calculated,
ViewportPercentageWidth, ViewportPercentageHeight, ViewportPercentageMin,
ViewportPercentageMax,
Undefined
};
You shouldn't need to care about those, but its good to knwo it exists.
> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:641
> + // See
http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSview-getComputedStyle
Maybe you meant this?
http://dev.w3.org/csswg/cssom/#widl-Window-getComputedStyle-CSSStyleDeclaration
-Element-elt
If the property applies to a positioned element and the resolved value of the
'display' property is not none, the resolved value is the used value. Otherwise
the resolved value is the computed value.
> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:645
> + return zoomAdjustedPixelValueForLength(style->left(), style);
I might have moved these switch statements into helper functions. Then this
code looks like:
if (!renderView || !renderer || !renderer->isBox())
return zoomAdjustedPixelValueForLength(lengthForProperty(propertyId),
style);
One could do similar for the other switches, presumably:
relativePositionForProperty(propertyId, box);
and
positionForProperty(propertyId, box, containingBlock);
> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-661
> - else if (l.isViewportPercentage())
Do we have test coverage for this behavior? You appear to be removing it in
the display: none case.
> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:659
> + if (box->isRelPositioned() || !containingBlock) {
I don't believe we can hit this case? You will always have a containing block
if you're in the DOM tree, I think? Unless we're talking about <html
style='position: relative'>? If the element isn't in the tree, it can't have a
renderer.
> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:687
> + return 0;
We need tests requesting top/right/bottom/left on elements which are not
positioned.
More information about the webkit-reviews
mailing list