Skip to content

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Aug 11, 2024

Align URL Processing rules for SVG
https://bugs.webkit.org/show_bug.cgi?id=269522
rdar://problem/123475267

Reviewed by NOBODY (OOPS!).

This patch aligns WebKit with Blink / Chrome and Web Specification [1]:

[1] https://url.spec.whatwg.org/#url-parsing

As per web specification, it is to trim leading and trailing whitespaces for SVG fragment Identifier and IRI.
It was discussed in following:

[2] w3c/svgwg#781

* Source/WebCore/svg/SVGURIReference.cpp:
(SVGURIReference::fragmentIdentifierFromIRIString):
(SVGURIReference::targetElementFromIRIString):
* LayoutTests/TestExpectations: Add comment for progressed tests

2459f10

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

@Ahmad-S792 Ahmad-S792 added the SVG For bugs in the SVG implementation. label Aug 11, 2024
@Ahmad-S792 Ahmad-S792 self-assigned this Aug 11, 2024
@Ahmad-S792 Ahmad-S792 marked this pull request as ready for review August 12, 2024 00:18
Copy link
Contributor

@nikolaszimmermann nikolaszimmermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, makes sense. Note there's a typo in the commit message (algin).

https://bugs.webkit.org/show_bug.cgi?id=269522
rdar://problem/123475267

Reviewed by NOBODY (OOPS!).

This patch aligns WebKit with Blink / Chrome and Web Specification [1]:

[1] https://url.spec.whatwg.org/#url-parsing

As per web specification, it is to trim leading and trailing whitespaces for SVG fragment Identifier and IRI.
It was discussed in following:

[2] w3c/svgwg#781

* Source/WebCore/svg/SVGURIReference.cpp:
(SVGURIReference::fragmentIdentifierFromIRIString):
(SVGURIReference::targetElementFromIRIString):
* LayoutTests/TestExpectations: Add comment for progressed tests
@Ahmad-S792
Copy link
Contributor Author

Thanks, makes sense. Note there's a typo in the commit message (algin).

Done. Thanks for noting down.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 12, 2024
@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Aug 12, 2024
Copy link
Contributor

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this still do the incorrect thing if you have #foo<tab>bar or some such? I think the problem is that we're not putting all inputs through the URL parser and maybe recording there whether it was a local URL? That there's no good specification for this doesn't help.

@Ahmad-S792
Copy link
Contributor Author

Wouldn't this still do the incorrect thing if you have #foo<tab>bar or some such? I think the problem is that we're not putting all inputs through the URL parser and maybe recording there whether it was a local URL? That there's no good specification for this doesn't help.

Isn't isASCIIWhitespace deal with \t (tab) as well - https://searchfox.org/wubkat/rev/b36cbce69fddb7da33823f316bd8ead5bebee970/Source/WTF/wtf/ASCIICType.h#142

@annevk
Copy link
Contributor

annevk commented Aug 12, 2024

Not if all you do is trim. In particular this neither implements step 1.3 or step 3 of https://url.spec.whatwg.org/#concept-basic-url-parser correctly as far as I can tell, both pertaining to the removal of certain code points within a URL.

@Ahmad-S792
Copy link
Contributor Author

Closing this since if the change is more involved, I wouldn't be right person to tackle it. Commented on bugzilla as well to account for Anne's input.

@Ahmad-S792 Ahmad-S792 closed this Aug 12, 2024
@Ahmad-S792 Ahmad-S792 deleted the fix269522 branch August 12, 2024 16:07
@karlcow
Copy link
Member

karlcow commented Dec 17, 2024

@annevk @Ahmad-S792 is there an issue in using URLParser::parse

void URLParser::parse(std::span<const CharacterType> input, const URL& base, const URLTextEncoding* nonUTF8QueryEncoding)
{
URL_PARSER_LOG("Parsing URL <%s> base <%s>", String(input).utf8().data(), base.string().utf8().data());
m_url = { };
ASSERT(m_asciiBuffer.isEmpty());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SVG For bugs in the SVG implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants