Bug 258003

Summary: SVGLengthValue.cpp should support rem (probably other) units for width and height attributes
Product: WebKit Reporter: Karl Dubost <karlcow>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: NEW    
Severity: Normal CC: ahmad.saleem792, sabouhallawa, webkit-bug-importer, zimmermann
Priority: P2 Keywords: BrowserCompat, InRadar
Version: Safari 17   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 253937    

Karl Dubost
Reported 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’)
Attachments
Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 2 2023-06-12 21:58:09 PDT
<rdar://problem/110687649>
Karl Dubost
Comment 3 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.
Karl Dubost
Comment 4 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
Karl Dubost
Comment 5 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.
Karl Dubost
Comment 6 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);
Ahmad Saleem
Comment 7 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?
Ahmad Saleem
Comment 8 2024-02-01 20:07:30 PST
Blink Commit - https://src.chromium.org/viewvc/blink?view=revision&revision=193355
Ahmad Saleem
Comment 9 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.
Karl Dubost
Comment 10 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.
Ahmad Saleem
Comment 11 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.
Ahmad Saleem
Comment 12 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. :-)
Ahmad Saleem
Comment 13 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.
Karl Dubost
Comment 14 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)
Note You need to log in before you can comment on or make changes to this bug.