Bug 171805 – SVG textLength not working correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Jason Davies]

Created attachment 309363 [details]
Minimal demonstration of textLength bug.

Open the attached bug.svg file, containing <text textLength="1200">test</text>.

Notice how the right-hand edge of the text does not line up with the red rectangle.

I've reported the same bug for Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=719522

FireFox exhibits the correct behaviour.

[Radar WebKit Bug Importer]

<rdar://problem/32066826>

[Tom Bigelajzen]

Created attachment 459960 [details]
Patch

[Tom Bigelajzen]

I created a minimal patch for this bug without tests etc. just to show that it's fixable by copying the logic from the Chromium bug fix from 5 years ago.

I don't have the time or knowledge to go through all the way with this patch (tests etc.) and will appreciate if someone with the right knowledge could complete the last mile with this fix

[Said Abou-Hallawa]

Comment on attachment 459960 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459960&action=review

> Source/WebCore/rendering/svg/SVGTextChunk.cpp:147
> +    float textLengthShift = desiredTextLength() - totalLength();
> +    if (totalCharacters() > 1) {
> +        textLengthShift /= totalCharacters() - 1;
> +    }

I think this code should look like this:

float textLengthShift = 0;
if (totalCharacters() > 1)
    textLengthShift = (desiredTextLength() - totalLength()) / (totalCharacters() - 1);

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:210
> -    if (textContentElement->lengthAdjust() == SVGLengthAdjustSpacing)
> -        m_textPathSpacing = (desiredTextLength - totalLength) / totalCharacters;
> +    if (textContentElement->lengthAdjust() == SVGLengthAdjustSpacing && totalCharacters > 1)
> +        m_textPathSpacing = (desiredTextLength - totalLength) / (totalCharacters - 1);
>      else
>          m_textPathScaling = desiredTextLength / totalLength;

I think this if-statement should be like this:

    if (textContentElement->lengthAdjust() == SVGLengthAdjustSpacing) {
        if (totalCharacters > 1)
            m_textPathSpacing = (desiredTextLength - totalLength) / (totalCharacters - 1);
    } else
        m_textPathScaling = desiredTextLength / totalLength;

[Said Abou-Hallawa]

(In reply to Tom Bigelajzen from comment #3)
> I created a minimal patch for this bug without tests etc. just to show that
> it's fixable by copying the logic from the Chromium bug fix from 5 years ago.
> 
> I don't have the time or knowledge to go through all the way with this patch
> (tests etc.) and will appreciate if someone with the right knowledge could
> complete the last mile with this fix

The failed tests need to be rebaslined since the inter-character spacing width has changed.

[Ahmad Saleem]

PR - https://github.com/WebKit/WebKit/pull/8655

Let's try..

@Said - I already took your review of this patch and incorporated it. :-)

[EWS]

Committed 258921@main (5c38a23d96c2): <https://commits.webkit.org/258921@main>

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