Bug 258003 - SVGLengthValue.cpp should support rem (probably other) units for width and height attributes
Summary: SVGLengthValue.cpp should support rem (probably other) units for width and he...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: Safari 17
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: BrowserCompat, InRadar
Depends on:
Blocks: 253937
  Show dependency treegraph
 
Reported: 2023-06-12 21:56 PDT by Karl Dubost
Modified: 2024-04-23 00:49 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Karl Dubost 2023-06-12 21:56:38 PDT
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’)
Comment 2 Radar WebKit Bug Importer 2023-06-12 21:58:09 PDT
<rdar://problem/110687649>
Comment 3 Karl Dubost 2023-06-12 21:59:32 PDT
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.
Comment 4 Karl Dubost 2023-06-12 22:20:26 PDT
# 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
Comment 5 Karl Dubost 2023-06-12 22:21:01 PDT
This is in fact here
https://searchfox.org/wubkat/source/Source/WebCore/svg/SVGLengthValue.cpp#65 

rem is missing.
Comment 6 Karl Dubost 2023-06-13 18:33:51 PDT
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);
Comment 7 Ahmad Saleem 2024-02-01 19:49:32 PST
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?
Comment 8 Ahmad Saleem 2024-02-01 20:07:30 PST
Blink Commit - https://src.chromium.org/viewvc/blink?view=revision&revision=193355
Comment 9 Ahmad Saleem 2024-02-01 20:32:19 PST
(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.
Comment 10 Karl Dubost 2024-02-15 22:17:25 PST
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.
Comment 11 Ahmad Saleem 2024-02-15 22:18:40 PST
(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.
Comment 12 Ahmad Saleem 2024-02-15 22:19:18 PST
(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. :-)
Comment 13 Ahmad Saleem 2024-02-15 22:20:11 PST
(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.
Comment 14 Karl Dubost 2024-04-23 00:47:42 PDT
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)