Bug 261656 – [SVG2] Allow leading and trailing whitespace in svg attributes using <integer>, <angle>, <number>, <length> and <percentage>

• Component: SVG
• Status: NEW

[Ahmad Saleem]

Hi Team,

This is another Blink's commit, where we are failing test cases:

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

Test Case (svg/parser/whitespace-number.html) - https://jsfiddle.net/34nqh1uy/show

Test Case (svg/parser/whitespace-length.html) - https://jsfiddle.net/34nqh1uy/1/show

and there are others as well.

^ We are failing lot of these tests on WebKit ToT as well. Chrome Canary 119 and Firefox Nightly 119 both pass these test case.

Just wanted to raise so we can track it.

Thanks!

[Karl Dubost]

The test:

data:text/html,<svg id="testcontainer"><defs><marker></marker><stop></stop><filter><feTurbulence></feTurbulence></filter></defs></svg>


document.getElementsByTagName('stop')[0].setAttribute('offset', '-47a')


Then they try to 
document.getElementsByTagName('stop')[0].getAttribute('offset')

which returns '-47a' in all browsers.

Where it differs is when requesting:
document.getElementsByTagName('stop')[0].offset.baseVal 

it returns 

-47 in Safari 
0   in Firefox
0   in Chrome

It seems Safari is normalizing the value by ignoring the trailing garbage.
While the others are saying bad values let's reset it.

In fact
document.getElementsByTagName('stop')[0].offset returns on Safari.

{animVal: -47, baseVal: -47}


https://svgwg.org/svg2-draft/pservers.html#GradientStopAttributes
the initial value is indeed 0.
The values

<number>
A number usually ranging from 0 to 1.
<percentage>
A percentage usually ranging from 0% to 100%.


I don't remember where are the processing rules for the invalid values in CSS. 

But baseVal and animVal are defined in
https://svgwg.org/svg2-draft/types.html#__svg__SVGAnimatedString__baseVal

[Karl Dubost]

Related(?) to
https://searchfox.org/wubkat/rev/f997a9edb80cd79caeb1d0a1b87610ffd7a56e88/Source/WebCore/svg/SVGStopElement.cpp#53-63

It converts to float but doesn't to check if the input value if the value looks like a float before converting it to a float. 

The patch from Chromium contains a parseNumber 
https://src.chromium.org/viewvc/blink/trunk/Source/core/svg/SVGParserUtilities.cpp?annotate=175785&pathrev=175785#l230


I wonder if it exists in WebKit?

[Karl Dubost]

Maybe around 
https://searchfox.org/wubkat/source/Source/WebCore/svg/SVGParserUtilities.cpp#142

[Karl Dubost]

So Chris Dumez helped me understand better


    WTF_EXPORT_PRIVATE float toFloat(bool* ok = nullptr) const;

So we could do something like

bool success = false;
float result = attribute.toFloat(&success);
if (success)
   // use result.
else
  // ...

[Karl Dubost]

Ahmad, I think I have a patch for this. It needs a bit more work but It's passing a lot more tests.

[Karl Dubost]

OK My patch gets all green for 
Test Case (svg/parser/whitespace-number.html) - https://jsfiddle.net/34nqh1uy/show



Now let's see the other. :) 
Test Case (svg/parser/whitespace-length.html) - https://jsfiddle.net/34nqh1uy/1/show

[Radar WebKit Bug Importer]

<rdar://problem/115963075>

[Ahmad Saleem]

Blink Commit - https://github.com/chromium/chromium/commit/77c8625da85bd956992b81233d451b87c39e7dea

[Ahmad Saleem]

> Source/WebCore/svg/SVGTransformable.cpp:

From: auto parsedNumber = parseNumber(buffer, SuffixSkippingPolicy::DontSkip);
To: auto parsedNumber = parseNumber(buffer, SVGWhitespaceMode::DisallowWhitespace);

and

From: auto parsedNumber = parseNumber(buffer, SuffixSkippingPolicy::DontSkip);
To: auto parsedNumber = parseNumber(buffer, SVGWhitespaceMode::DisallowWhitespace);

___

> Source/WebCore/svg/SVGPointList.cpp:

From: auto yPos = parseNumber(buffer, SuffixSkippingPolicy::DontSkip);
To: auto yPos = parseNumber(buffer, SVGWhitespaceMode::DisallowWhitespace);

___

> Source/WebCore/svg/SVGLengthValue.cpp:

From: auto convertedNumber = parseNumber(buffer, SuffixSkippingPolicy::DontSkip);
To: auto convertedNumber = parseNumber(buffer, SVGWhitespaceMode::AllowLeadingWhitespace);

___

> Source/WebCore/svg/SVGFitToViewBox.cpp:

From: auto height = parseNumber(buffer, SuffixSkippingPolicy::DontSkip);
To: auto height = parseNumber(buffer, SVGWhitespaceMode::AllowLeadingAndTrailingWhitespace);

___

> Source/WebCore/svg/SVGAnimationElement.cpp:

From: auto posD = parseNumber(buffer, SuffixSkippingPolicy::DontSkip);
To: auto posD = parseNumber(buffer, SVGWhitespaceMode::DisallowWhitespace);

____

> Source/WebCore/svg/SVGAngleValue.cpp:

From: auto valueInSpecifiedUnits = parseNumber(buffer, SuffixSkippingPolicy::DontSkip);
To: uto valueInSpecifiedUnits = parseNumber(buffer, SVGWhitespaceMode::AllowLeadingWhitespace);

___

> Source/WebCore/svg/SVGParserUtilities.h:

Delete below:

enum class SuffixSkippingPolicy {
    DontSkip,
    Skip
};

and add this:

enum class SVGWhitespaceMode {
    DisallowWhitespace = 0,
    AllowLeadingWhitespace = 0x1,
    AllowTrailingWhitespace = 0x2,
    AllowLeadingAndTrailingWhitespace = AllowLeadingWhitespace | AllowTrailingWhitespace
};

Change:

std::optional<float> parseNumber(StringParsingBuffer<LChar>&, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);

std::optional<float> parseNumber(StringParsingBuffer<LChar>&, SVGWhitespaceMode mode = SVGWhitespaceMode::AllowLeadingAndTrailingWhitespace);

and


std::optional<float> parseNumber(StringParsingBuffer<UChar>&, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);

std::optional<float> parseNumber(StringParsingBuffer<UChar>&, SVGWhitespaceMode mode = SVGWhitespaceMode::AllowLeadingAndTrailingWhitespace);

and

std::optional<float> parseNumber(StringView, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);

std::optional<float> parseNumber(StringView, SVGWhitespaceMode mode = SVGWhitespaceMode::AllowLeadingAndTrailingWhitespace);

____

> Source/WebCore/svg/SVGParserUtilities.cpp:

Function definition:

template <typename CharacterType, typename FloatType = float> static std::optional<FloatType> genericParseNumber(StringParsingBuffer<CharacterType>& buffer, SuffixSkippingPolicy skip = SuffixSkippingPolicy::Skip)

to:

template <typename CharacterType, typename FloatType = float> static std::optional<FloatType> genericParseNumber(StringParsingBuffer<CharacterType>& buffer, SVGWhitespaceMode mode = SVGWhitespaceMode::AllowLeadingAndTrailingWhitespace)

and in below:

if (mode == SVGWhitespaceMode::AllowLeadingWhitespace)
        skipOptionalSVGSpaces(buffer);

and

if (mode == SVGWhitespaceMode::AllowTrailingWhitespace)
        skipOptionalSVGSpacesOrDelimiter(buffer);

Another function definition:

std::optional<float> parseNumber(StringView string, SuffixSkippingPolicy skip)

to:

std::optional<float> parseNumber(StringView string, SVGWhitespaceMode mode)

and

return readCharactersForParsing(string, [skip](auto buffer) -> std::optional<float> {
        auto result = genericParseNumber(buffer, skip);

to:

return readCharactersForParsing(string, [mode](auto buffer) -> std::optional<float> {
        auto result = genericParseNumber(buffer, mode);

Another function definition:

std::optional<float> parseNumber(StringParsingBuffer<LChar>& buffer, SuffixSkippingPolicy skip)

to:

std::optional<float> parseNumber(StringParsingBuffer<LChar>& buffer, SVGWhitespaceMode mode)

and change return to:  return genericParseNumber(buffer, mode);

Another function definition:

std::optional<float> parseNumber(StringParsingBuffer<UChar>& buffer, SuffixSkippingPolicy skip)

to:

std::optional<float> parseNumber(StringParsingBuffer<UChar>& buffer, SVGWhitespaceMode mode)

and change return: return genericParseNumber(buffer, mode);

___

> Source/WebCore/svg/SVGParserUtilities.cpp:

parseNumberOptionalNumber:

auto y = parseNumber(buffer, SuffixSkippingPolicy::DontSkip); to auto y = parseNumber(buffer, SVGWhitespaceMode::AllowLeadingAndTrailingWhitespace);


parseRect:

auto height = parseNumber(buffer, SuffixSkippingPolicy::DontSkip); to auto height = parseNumber(buffer, SVGWhitespaceMode::DisallowWhitespace);

___

It does progress test cases but not where we have have scientific notations.

[Karl Dubost]

Ahmad, I think scientific notations are still not yet a spec thing.

[Karl Dubost]

If there are WPT tests relying on scientific notations they should be removed. 
A bit like it was done in https://github.com/web-platform-tests/wpt/commit/06e103ad6136bf0988f9d1614db05d8a652e0570

[Ahmad Saleem]

Syncing some passing tests here - https://github.com/WebKit/WebKit/pull/44306

At least, it would give some coverage while we try to progress remaining cases. I do have patch to make some progress more without any regression, so I think eve n partial progression is better than nothing. So might end-up doing PR later.

[Ahmad Saleem]

Pull request: https://github.com/WebKit/WebKit/pull/44323

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #13)
> Pull request: https://github.com/WebKit/WebKit/pull/44323

NOTE - I am changing it to to draft, it still progress test cases without regression:

https://jsfiddle.net/maseh9v7/ -> whitespace-angle-2.html (105 -> 156)
https://jsfiddle.net/pad9c3mo/ -> whitespace-angle-1.html (108 -> 288)
https://jsfiddle.net/2639xdzs/ -> whitespace-integer.html (27 - no change)
https://jsfiddle.net/6bf32qhk/ -> whitespace-number.html (1074 - no change)
https://jsfiddle.net/2b43fprz/ -> whitespace-length.html (704 -> 942)

Plus it is partial merge, I will try to do remaining bit and hopefully can progress it fully but either way - even landing as is for partial progression like above wouldn't be so bad.