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.
<rdar://problem/32066826>
Created attachment 459960 [details] Patch
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
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;
(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.
PR - https://github.com/WebKit/WebKit/pull/8655 Let's try.. @Said - I already took your review of this patch and incorporated it. :-)
Committed 258921@main (5c38a23d96c2): <https://commits.webkit.org/258921@main> Reviewed commits have been landed. Closing PR #8655 and removing active labels.