Bug 258003 – SVGLengthValue.cpp should support rem (probably other) units for width and height attributes

• Status: NEW
• Component: SVG

[Karl Dubost]

https://searchfox.org/wubkat/rev/a420a9f3f6ab4c8d0c75aae3877d736d66affe36/Source/WebCore/svg/SVGSVGElement.cpp#210-216


data:text/html,<svg width="100" height="100"><circle cx="50" cy="50" r="50" /></svg>


# this is ok and valid. No issue
document.querySelector('svg').setAttribute('width', 200)


# This creates an error message in the console.
# Chrome accepts it, Safari and Firefox reject it.
# Not only it rejects the value but sets an entirely different value (this should probably not happen too.)
document.querySelector('svg').setAttribute('width', ‘2rem’)

[Karl Dubost]

SeeAlso

https://svgwg.org/svg2-draft/geometry.html#Sizing
https://svgwg.org/svg2-draft/styling.html#PresentationAttributes
https://svgwg.org/svg2-draft/types.html#presentation-attribute-css-value

[Radar WebKit Bug Importer]

<rdar://problem/110687649>

[Karl Dubost]

This is probably the cause of a webcompat issue on 
https://www.usnews.com/best-colleges/franklin-w-olin-college-of-engineering-39463
when scrolling up and when the menu is trying to set a right chevron.
The issue is happening also for Firefox.

[Karl Dubost]

# For em values
Starting with 
data:text/html,<svg></svg>

> document.querySelector('svg').width.baseVal
< SVGLength {unitType: 2, value: 300, valueInSpecifiedUnits: 100, valueAsString: "100%", newValueSpecifiedUnits: function, …}
> document.querySelector('svg').setAttribute('width', '2em')
< undefined
> document.querySelector('svg').width.baseVal
< SVGLength {unitType: 3, value: 32, valueInSpecifiedUnits: 2, valueAsString: "2em", newValueSpecifiedUnits: function, …}


# For rem values
Restarting again with 
data:text/html,<svg></svg>

> document.querySelector('svg').width.baseVal
< SVGLength {unitType: 2, value: 300, valueInSpecifiedUnits: 100, valueAsString: "100%", newValueSpecifiedUnits: function, …}
> document.querySelector('svg').setAttribute('width', '2rem')
[Error] Error: Invalid value for <svg> attribute width="2rem"
	Console Evaluation (Console Evaluation 2:3)
< undefined
> document.querySelector('svg').width.baseVal
< SVGLength = $1

unitType: 2

value: 32

valueAsString: "100%"

valueInSpecifiedUnits: 100

SVGLength Prototype

[Karl Dubost]

This is in fact here
https://searchfox.org/wubkat/source/Source/WebCore/svg/SVGLengthValue.cpp#65 

rem is missing.

[Karl Dubost]

Starting with data:text/html,<svg%20width="50"></svg>

We get for document.querySelector('svg').width.baseVal

unitType: 1
value: 50
valueAsString: "50"
valueInSpecifiedUnits: 50

Let's set to 3 rem.
document.querySelector('svg').setAttribute('width', '3rem')

unitType: 2 (SVG_LENGTHTYPE_PERCENTAGE)
value: 48
valueAsString: "100%"
valueInSpecifiedUnits: 100

Why the 48 here?


It reports the issue
    reportAttributeParsingError(parseError, name, newValue);

and then goes on with:

    SVGFitToViewBox::parseAttribute(name, newValue);
    SVGZoomAndPan::parseAttribute(name, newValue);
    SVGGraphicsElement::attributeChanged(name, oldValue, newValue, attributeModificationReason);

[Ahmad Saleem]

We do have WPT here: https://wpt.fyi/results/svg/types/scripted/SVGLength-rem.html?label=master&label=experimental&aligned=&q=safari%3Afail

Or this bug is different?

[Ahmad Saleem]

Blink Commit - https://src.chromium.org/viewvc/blink?view=revision&revision=193355

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #8)
> Blink Commit -
> https://src.chromium.org/viewvc/blink?view=revision&revision=193355

Based on this (inspired), I have local patch, which does progress sub-test of WPT but I think we have hardcoded font-size, so one sub-test continue to fail.

I think something is better than nothing, so I am happy to do PR but keeping this as master bug and separate for each length type.

[Karl Dubost]

I wonder if it would fix also assertions in 
http://wpt.live/svg/types/scripted/SVGLength.html
there are a lot of things failing there, which seems basic.

[Ahmad Saleem]

(In reply to Karl Dubost from comment #10)
> I wonder if it would fix also assertions in 
> http://wpt.live/svg/types/scripted/SVGLength.html
> there are a lot of things failing there, which seems basic.

I tried 'IDL' file changes and we start to progress few more tests but also start failing few of current tests in `svg/dom/'. So I never did the change.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #11)
> (In reply to Karl Dubost from comment #10)
> > I wonder if it would fix also assertions in 
> > http://wpt.live/svg/types/scripted/SVGLength.html
> > there are a lot of things failing there, which seems basic.
> 
> I tried 'IDL' file changes and we start to progress few more tests but also
> start failing few of current tests in `svg/dom/'. So I never did the change.

Plus - I also have `rem` patch locally - just want to land few of current PR and reduce my queue before doing PR to add `rem` support. :-)

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #11)
> (In reply to Karl Dubost from comment #10)
> > I wonder if it would fix also assertions in 
> > http://wpt.live/svg/types/scripted/SVGLength.html
> > there are a lot of things failing there, which seems basic.
> 
> I tried 'IDL' file changes and we start to progress few more tests but also
> start failing few of current tests in `svg/dom/'. So I never did the change.

From SVGLength.idl, we have `unrestricted` while as per spec, we shouldn't.

[Karl Dubost]

This is also creating a series of error messages in the console for 
https://vimeo.com/channels/staffpicks

[Error] Error: Invalid value for <svg> attribute width="1.5rem"
	setValueForProperty (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:2:20315)
	_updateDOMProperties (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:25153)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:21494)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	mountChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:19271)
	_createInitialChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:23382)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:21543)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	mountChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:19271)
	_createInitialChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:23382)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:21543)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	mountChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:19271)
	_createInitialChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:23382)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:21543)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	mountChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:19271)
	_createInitialChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:23382)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:21543)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	mountChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:19271)
	_createInitialChildren (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:23382)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:21543)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	performInitialMount (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:10790)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:3:9681)
	mountComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:22674)
	a (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:14516)
	perform (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:5:13022)
	s (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:14730)
	perform (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:5:13022)
	_renderNewRootComponent (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:15998)
	_renderSubtreeIntoContainer (react_prod_combined_ccb7d5c4cff728ceadb9996f385b2d09.min.js:4:17004)
	d (top_navigation.2644aa259e2ca7a1053f.bundle.min.js:54:157882)
	(anonymous function) (top_navigation.2644aa259e2ca7a1053f.bundle.min.js:28:565671)
	(anonymous function) (top_navigation.2644aa259e2ca7a1053f.bundle.min.js:28:565691)
	n (top_navigation.2644aa259e2ca7a1053f.bundle.min.js:1:115)
	(anonymous function) (top_navigation.2644aa259e2ca7a1053f.bundle.min.js:1:453)
	Global Code (top_navigation.2644aa259e2ca7a1053f.bundle.min.js:1:463)