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.
This issue sounds familiar, as if I saw (or even fixed?) a similar report one ago. But I can't find it now.
s/one ago/years ago/
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."
(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.
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.
Created attachment 73760 [details] [PATCH] Comments addressed
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|?
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.
Committed r72082: <http://trac.webkit.org/changeset/72082>
http://trac.webkit.org/changeset/72082 might have broken Chromium Mac Release
Committed r72189: <http://trac.webkit.org/changeset/72189>