Bug 35770 – CSSPrimitiveValue.getFloatValue does not convert sizes

• Component: DOM
• Status: RESOLVED FIXED

Created attachment 50057 [details]
Test Case

CSSPrimitiveValue.getFloatValue always returns the original magnitude of the style declaration but does not convert the magnitude to other units as per http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSPrimitiveValue .
IE: a style declaration of length 2em will always return 2, for all units. 

A side note is that because getComputedStyle converts this to px it will always return the px of a css value that is a length.

[Alexey Proskuryakov]

This issue sounds familiar, as if I saw (or even fixed?) a similar report one ago. But I can't find it now.

[Alexey Proskuryakov]

s/one ago/years ago/

[Alexander Pavlov (apavlov)]

I've got a patch to convert between all units except rem/em/ex, since these require having RenderStyle of self and/or parent/root elements at hand. Does anyone have a suggestion on how this issue could be treated? We don't want CSSPrimitiveValue to reference its owner style do we?

(In reply to comment #3)
> I've got a patch to convert between all units except rem/em/ex, since these require having RenderStyle of self and/or parent/root elements at hand. Does anyone have a suggestion on how this issue could be treated? We don't want CSSPrimitiveValue to reference its owner style do we?

From the spec: "Conversions are allowed between absolute values (from millimeters to centimeters, from degrees to radians, and so on) but not between relative values."

[Alexander Pavlov (apavlov)]

(In reply to comment #4)
> (In reply to comment #3)
> > I've got a patch to convert between all units except rem/em/ex, since these require having RenderStyle of self and/or parent/root elements at hand. Does anyone have a suggestion on how this issue could be treated? We don't want CSSPrimitiveValue to reference its owner style do we?
> 
> From the spec: "Conversions are allowed between absolute values (from millimeters to centimeters, from degrees to radians, and so on) but not between relative values."

Which means the original complaint "a style declaration of length 2em will always return 2, for all units" is invalid. Thanks for the clarification, will prepare the patch soon.

[Alexander Pavlov (apavlov)]

Created attachment 73745 [details]
[PATCH] Suggested solution

Comment on attachment 73745 [details]
[PATCH] Suggested solution

View in context: https://bugs.webkit.org/attachment.cgi?id=73745&action=review

> WebCore/css/CSSPrimitiveValue.cpp:48
> +#ifndef M_PI
> +#define M_PI 3.14159265358979323846
> +#endif

Please use piDouble. Include MathExtras.h if needed.

> WebCore/css/CSSPrimitiveValue.cpp:80
> +CSSPrimitiveValue::UnitCategories CSSPrimitiveValue::unitCategory(UnitTypes type)

This can be a file-static function.

> WebCore/css/CSSPrimitiveValue.cpp:489
> +            factor = 180.0 / M_PI;

Use piDouble and drop the .0

> WebCore/css/CSSPrimitiveValue.cpp:495
> +            factor = 360.0;

Drop the .0

> WebCore/css/CSSPrimitiveValue.cpp:499
> +            factor = 1000.0;

Drop the .0

> WebCore/css/CSSPrimitiveValue.h:98
> +    enum UnitCategories {

enum names are usually singular

> WebCore/css/CSSPrimitiveValue.h:99
> +        UNIT_NUMBER,

This is not the standard style for enum values.

[Alexander Pavlov (apavlov)]

Created attachment 73760 [details]
[PATCH] Comments addressed

[Simon Fraser (smfr)]

Comment on attachment 73760 [details]
[PATCH] Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=73760&action=review

r=me with comments addressed.

> WebCore/ChangeLog:11
> +        CSSPrimitiveValue.getFloatValue does not convert sizes
> +
> +        Implemented all same-category unit conversions (length, angle, time, frequency) and left
> +        absolute-relative (cm, mm, in, pt, pc) length conversions intact (assuming a monitor resolution of 96ppi).
> +        Illegal unit conversion attempts will throw an INVALID_ACCESS_ERR DOMException.
> +        https://bugs.webkit.org/show_bug.cgi?id=35770
> +

The bug URL should go above the title.

> WebCore/css/CSSPrimitiveValue.cpp:54
> +    // Here we violate the spec (http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSPrimitiveValue)
> +    // and allow conversions between CSS_PX and relative length units assuming a 96ppi screen (see CSSHelper.h).

Why? Please justify.

> WebCore/css/CSSPrimitiveValue.cpp:91
>  // A more stylish solution than sharing would be to turn CSSPrimitiveValue (or CSSValues in general) into non-virtual,
> -// non-refcounted simple type with value semantics. In practice these sharing tricks get similar memory benefits 
> +// non-refcounted simple type with value semantics. In practice these sharing tricks get similar memory benefits

This sentence needs a period at the end.

> WebCore/css/CSSPrimitiveValue.cpp:459
>  static double scaleFactorForConversion(unsigned short unitType)

This function has a really bad name. It would be better named as conversionToCanonicalUnitsScaleFactor(). Also, who decides what the canonical units are?

> WebCore/css/CSSPrimitiveValue.cpp:505
>  double CSSPrimitiveValue::getDoubleValue(unsigned short unitType, ExceptionCode& ec)

Can all the getFooValue methods be |const|?

[Alexander Pavlov (apavlov)]

Will land with the following corrections in place:

(In reply to comment #9)
> (From update of attachment 73760 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73760&action=review
> 
> r=me with comments addressed.
> 
> > WebCore/ChangeLog:11
> > +        CSSPrimitiveValue.getFloatValue does not convert sizes
> > +
> > +        Implemented all same-category unit conversions (length, angle, time, frequency) and left
> > +        absolute-relative (cm, mm, in, pt, pc) length conversions intact (assuming a monitor resolution of 96ppi).
> > +        Illegal unit conversion attempts will throw an INVALID_ACCESS_ERR DOMException.
> > +        https://bugs.webkit.org/show_bug.cgi?id=35770
> > +
> 
> The bug URL should go above the title.

Done.

> > WebCore/css/CSSPrimitiveValue.cpp:54
> > +    // Here we violate the spec (http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSPrimitiveValue)
> > +    // and allow conversions between CSS_PX and relative length units assuming a 96ppi screen (see CSSHelper.h).
> 
> Why? Please justify.

Put an explicit reference to a solid treatment of the issue in CSSHelper.h

> > WebCore/css/CSSPrimitiveValue.cpp:91
> >  // A more stylish solution than sharing would be to turn CSSPrimitiveValue (or CSSValues in general) into non-virtual,
> > -// non-refcounted simple type with value semantics. In practice these sharing tricks get similar memory benefits 
> > +// non-refcounted simple type with value semantics. In practice these sharing tricks get similar memory benefits
> 
> This sentence needs a period at the end.

This line is not the last line of the sentence :)

> > WebCore/css/CSSPrimitiveValue.cpp:459
> >  static double scaleFactorForConversion(unsigned short unitType)
> 
> This function has a really bad name. It would be better named as conversionToCanonicalUnitsScaleFactor(). Also, who decides what the canonical units are?

Renamed the function. "Canonical" units were chosen according to the way CSSParser::validUnit() converts plain numbers into unit-based values in the non-strict mode (see the "case CSSPrimitiveValue::CSS_NUMBER" branch).

> > WebCore/css/CSSPrimitiveValue.cpp:505
> >  double CSSPrimitiveValue::getDoubleValue(unsigned short unitType, ExceptionCode& ec)
> 
> Can all the getFooValue methods be |const|?

Done.

[Alexander Pavlov (apavlov)]

Committed r72082: <http://trac.webkit.org/changeset/72082>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72082 might have broken Chromium Mac Release

[Alexander Pavlov (apavlov)]

Committed r72189: <http://trac.webkit.org/changeset/72189>