Make the SVG parser interpret `form feed` as whitespace #23059

Ahmad-S792 · 2024-01-22T21:03:21Z

`15c6cea`

Make the SVG parser interpret `form feed` as whitespace

https://bugs.webkit.org/show_bug.cgi?id=77755
rdar://problem/95488677

Reviewed by Anne van Kesteren.

This patch is to align WebKit with Chromium / Blink, Gecko / Firefox and Web Specification [1]:

[1] https://lists.w3.org/Archives/Public/public-svg-wg/2014AprJun/0068.html

This patch uses 'isASCIIWhite' across SVG Parser and code base to enable it to handle 'form feed'.

It is inspired by following change in Blink, which enables 'leading' and 'trailing' whitespace
in SVG Attributes [2]:

[2] https://src.chromium.org/viewvc/blink?view=revision&;revision=175785

Credits to Jacob Goldstein  <jacobg@adobe.com> for 'test case'.

* Source/WebCore/svg/SVGParserUtilities.cpp:
(parseGlyphName):
* Source/WebCore/svg/SVGParserUtilities.h:
(isSVGSpace): Deleted
(isSVGSpaceOrComma): Updated
(skipOptionalSVGSpaces):
(skipOptionalSVGSpacesOrDelimiter): Both templates
* Source/WebCore/svg/SVGStringList.cpp:
(SVGStringList::parse):
* LayoutTests/svg/transforms/svg-formFeed-as-whitespace.html: Add Test Case
* LayoutTests/svg/transforms/svg-formFeed-as-whitespace-expected.html: Add Test Case Expectation
* LayoutTests/svg/transforms/svg-line-tabulation-as-not-whitespace.html: Add Test Case
* LayoutTests/svg/transforms/svg-line-tabulation-as-not-whitespace-expected.html: Add Test Case Expectation

Canonical link: https://commits.webkit.org/273353@main

384b9bf

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-01-22T21:06:17Z

EWS run on previous version of this PR (hash 1a812a2)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
⏳ 🧪 webkitpy	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	✅ 🛠 gtk
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	⏳ 🛠 tv-sim
	✅ 🛠 watch
	⏳ 🛠 watch-sim

Ahmad-S792 · 2024-01-22T21:06:45Z

As per GitHub issue - the tests are still missing but it is just preparatory patch and not do all changes, which were done in Blink patch, I think we can do other changes one by one to align with other browsers. It was just the quick one which I could carve-out. So I did it.

Ahmad-S792 · 2024-01-22T21:13:29Z

It was also clarified in: https://lists.w3.org/Archives/Public/public-svg-wg/2014AprJun/0068.html

webkit-early-warning-system · 2024-01-22T22:53:06Z

EWS run on previous version of this PR (hash 70e0d27)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
⏳ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 wincairo
⏳ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
⏳ 🧪 webkitperl	❌ 🧪 ios-wk2	⏳ 🧪 api-mac	⏳ 🧪 api-wpe
⏳ 🧪 webkitpy	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	⏳ 🛠 gtk
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	~~🧪 mac-wk2~~	⏳ 🧪 gtk-wk2
	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 api-gtk
	⏳ 🛠 tv-sim	❌ 🧪 mac-wk2-stress
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2024-01-22T23:47:56Z

EWS run on previous version of this PR (hash f8a22d0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

annevk

Ideally there would be web platform tests coverage for this as well. And maybe include coverage that U+000B is not whitespace.

Overall though this seems like a good change and helps with cleaning up our whitespace handling: https://bugs.webkit.org/show_bug.cgi?id=255467.

Thanks!

Ahmad-S792 · 2024-01-23T11:09:11Z

Ideally there would be web platform tests coverage for this as well. And maybe include coverage that U+000B is not whitespace.

Overall though this seems like a good change and helps with cleaning up our whitespace handling: https://bugs.webkit.org/show_bug.cgi?id=255467.

Thanks!

I tried this - https://jsfiddle.net/74j5pftn/3/, all browsers are same in dealing with 'U+000B' and confirms that it is not whitespace. As for WPT test coverage, I think based on GitHub link posted above, it mentions that the tests are yet there.

webkit-early-warning-system · 2024-01-23T11:18:39Z

EWS run on current version of this PR (hash 384b9bf)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

Ahmad-S792 · 2024-01-23T11:19:16Z

@annevk - I added another test. Is it good to go now?

annevk

Yeah, sorry, I should have been clearer. "Ideal" wasn't meant to be a merge blocker. Thanks for adding the additional coverage.

https://bugs.webkit.org/show_bug.cgi?id=77755 rdar://problem/95488677 Reviewed by Anne van Kesteren. This patch is to align WebKit with Chromium / Blink, Gecko / Firefox and Web Specification [1]: [1] https://lists.w3.org/Archives/Public/public-svg-wg/2014AprJun/0068.html This patch uses 'isASCIIWhite' across SVG Parser and code base to enable it to handle 'form feed'. It is inspired by following change in Blink, which enables 'leading' and 'trailing' whitespace in SVG Attributes [2]: [2] https://src.chromium.org/viewvc/blink?view=revision&revision=175785 Credits to Jacob Goldstein <jacobg@adobe.com> for 'test case'. * Source/WebCore/svg/SVGParserUtilities.cpp: (parseGlyphName): * Source/WebCore/svg/SVGParserUtilities.h: (isSVGSpace): Deleted (isSVGSpaceOrComma): Updated (skipOptionalSVGSpaces): (skipOptionalSVGSpacesOrDelimiter): Both templates * Source/WebCore/svg/SVGStringList.cpp: (SVGStringList::parse): * LayoutTests/svg/transforms/svg-formFeed-as-whitespace.html: Add Test Case * LayoutTests/svg/transforms/svg-formFeed-as-whitespace-expected.html: Add Test Case Expectation * LayoutTests/svg/transforms/svg-line-tabulation-as-not-whitespace.html: Add Test Case * LayoutTests/svg/transforms/svg-line-tabulation-as-not-whitespace-expected.html: Add Test Case Expectation Canonical link: https://commits.webkit.org/273353@main

webkit-commit-queue · 2024-01-23T13:35:17Z

Committed 273353@main (15c6cea): https://commits.webkit.org/273353@main

Reviewed commits have been landed. Closing PR #23059 and removing active labels.

Ahmad-S792 added the SVG For bugs in the SVG implementation. label Jan 22, 2024

Ahmad-S792 self-assigned this Jan 22, 2024

Ahmad-S792 force-pushed the fix-svg-parser-to-use-isASCIIWhitespace branch from 1a812a2 to 70e0d27 Compare January 22, 2024 22:49

Ahmad-S792 changed the title ~~Use 'isASCIIWhitespace' in SVGParserUtilities.cpp|h and SVGStringList.cpp~~ SVG parser does not interpret "form feed" as white space Jan 22, 2024

Ahmad-S792 changed the title ~~SVG parser does not interpret "form feed" as white space~~ Make the SVG parser interpret form feed as whitespace Jan 22, 2024

Ahmad-S792 force-pushed the fix-svg-parser-to-use-isASCIIWhitespace branch from 70e0d27 to f8a22d0 Compare January 22, 2024 23:39

Ahmad-S792 marked this pull request as ready for review January 22, 2024 23:54

Ahmad-S792 requested review from smfr, heycam, rwlbuis, shallawa and nikolaszimmermann January 22, 2024 23:55

annevk approved these changes Jan 23, 2024

View reviewed changes

Ahmad-S792 force-pushed the fix-svg-parser-to-use-isASCIIWhitespace branch from f8a22d0 to 384b9bf Compare January 23, 2024 11:18

annevk approved these changes Jan 23, 2024

View reviewed changes

Ahmad-S792 added the merge-queue Applied to send a pull request to merge-queue label Jan 23, 2024

webkit-commit-queue force-pushed the fix-svg-parser-to-use-isASCIIWhitespace branch from 384b9bf to 15c6cea Compare January 23, 2024 13:35

webkit-commit-queue merged commit 15c6cea into WebKit:main Jan 23, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 23, 2024

Ahmad-S792 deleted the fix-svg-parser-to-use-isASCIIWhitespace branch January 24, 2024 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the SVG parser interpret `form feed` as whitespace #23059

Make the SVG parser interpret `form feed` as whitespace #23059

Ahmad-S792 commented Jan 22, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 22, 2024 •

edited

Ahmad-S792 commented Jan 22, 2024

Ahmad-S792 commented Jan 22, 2024

webkit-early-warning-system commented Jan 22, 2024 •

edited

webkit-early-warning-system commented Jan 22, 2024 •

edited

annevk left a comment

Ahmad-S792 commented Jan 23, 2024

webkit-early-warning-system commented Jan 23, 2024 •

edited

Ahmad-S792 commented Jan 23, 2024

annevk left a comment

webkit-commit-queue commented Jan 23, 2024

Make the SVG parser interpret form feed as whitespace #23059

Make the SVG parser interpret form feed as whitespace #23059

Conversation

Ahmad-S792 commented Jan 22, 2024 • edited by webkit-early-warning-system

15c6cea

webkit-early-warning-system commented Jan 22, 2024 • edited

Ahmad-S792 commented Jan 22, 2024

Ahmad-S792 commented Jan 22, 2024

webkit-early-warning-system commented Jan 22, 2024 • edited

webkit-early-warning-system commented Jan 22, 2024 • edited

annevk left a comment

Choose a reason for hiding this comment

Ahmad-S792 commented Jan 23, 2024

webkit-early-warning-system commented Jan 23, 2024 • edited

Ahmad-S792 commented Jan 23, 2024

annevk left a comment

Choose a reason for hiding this comment

webkit-commit-queue commented Jan 23, 2024

Make the SVG parser interpret `form feed` as whitespace #23059

Make the SVG parser interpret `form feed` as whitespace #23059

Ahmad-S792 commented Jan 22, 2024 •

edited by webkit-early-warning-system

`15c6cea`

webkit-early-warning-system commented Jan 22, 2024 •

edited

webkit-early-warning-system commented Jan 22, 2024 •

edited

webkit-early-warning-system commented Jan 22, 2024 •

edited

webkit-early-warning-system commented Jan 23, 2024 •

edited