Bug 52543 – SVGTextContentElements textLength returns 0

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

[Dirk Schulze]

The W3C test attached as link above does not pass, because tt.textLength.baseVal.value returns 0 while tt.getComputedTextLength() returns 56.

[Dirk Schulze]

Created attachment 79111 [details]
textLength, reduction of the W3C test

Reduction of the W3C test

[Nikolas Zimmermann]

I'm aware of this, and it's also tested by existing tests in W3C-SVG-1.1-SE and even one in W3C-SVG-1.1.
It's not hard to fix this problem, but it needs special logic...

[Nikolas Zimmermann]

Created attachment 88927 [details]
Patch

Maybe the ChangeLogs are too short, I'm in hurry right now, uploading for EWS.

[Nikolas Zimmermann]

Created attachment 88928 [details]
Patch v2

Better ChangeLos, that's it patch is done.

[Dirk Schulze]

Comment on attachment 88928 [details]
Patch v2

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

> Source/WebCore/ChangeLog:16
> +        This is how Opera implements it, and it makes sense to a certain degree, as otherwhise getComputedTextLength()
> +        and textLength.baseVal.value would always be the same. Nor does the spec mention that textLength is honored
> +        in the SVG Text DOM API.

To be honest: It doesn't make sense for me, or I misunderstand you comment. If I set textLength.baseVal it affects the displayed text (it's length like the name mentions). computedTextLength should (just my opinion), consider this. If I use computedTextLength, I want the actual text length, similar to the CSS behavior. What else is computedTextLength good for, if we do not take SVG DOM changes into account? 

This is just a comment for discussion. I continue the review anyway.

[Dirk Schulze]

Comment on attachment 88928 [details]
Patch v2

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

I don't give a r- nor a r+. I want to know why you think, computedTestLength shouldn't take DOM changes on textLength into account.

> Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:708
> +        fragment.buildFragmentTransform(fragmentTransform);

Does the result of fragment.buildFragmentTransform differ to the previous calculations? If not, can't we save it and calculate it once?

> Source/WebCore/rendering/svg/SVGTextFragment.h:106
> +        result = result.isIdentity() ? lengthAdjustTransform : lengthAdjustTransform * result;

You copy transform to result first and copy lengthAdjustTransform to result afterwards. Can you try to optimize this? You may check x and y earlier from transformAroundOrigin to be more efficient.

> Source/WebCore/svg/SVGTextContentElement.cpp:-39
> -DEFINE_ANIMATED_LENGTH(SVGTextContentElement, SVGNames::textLengthAttr, TextLength, textLength)

Can you describe this a bit further please? Why do you handle it here manually? Does this affect the handling of textLength for animations? Especially when we support animVal and baseVal later?

> Source/WebCore/svg/SVGTextContentElement.h:59
> +    // textLength is not declared using the standard DECLARE_ANIMATED_LENGTH macro
> +    // as its getter needs special handling (return getComputedTextLength(), instead of m_textLength).

Ok, at least the first question is answered.

> LayoutTests/ChangeLog:10
> +        This matches Opera and a discussion on www-svg some weeks ago.

Can you point me to this discussion again please? Maybe you can add the link to the ChangeLog as well.

> LayoutTests/svg/dynamic-updates/script-tests/SVGTextElement-dom-lengthAdjust-attr.js:17
> +shouldBeTrue("lastLength = textElement.getComputedTextLength(); lastLength > 50 && lastLength < 100");

So normal DOM is affected as well. Can you take a SVG Font, so that we have the exact length? The same for the other tests.

[Nikolas Zimmermann]

(In reply to comment #6)
> (From update of attachment 88928 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88928&action=review
> 
> I don't give a r- nor a r+. I want to know why you think, computedTestLength shouldn't take DOM changes on textLength into account.
Why DOM changes only? computedTextLength just never takes textLength/lengthAdjust into account.

> 
> > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:708
> > +        fragment.buildFragmentTransform(fragmentTransform);
> 
> Does the result of fragment.buildFragmentTransform differ to the previous calculations? If not, can't we save it and calculate it once?
Sure it differs, per fragment.

> 
> > Source/WebCore/rendering/svg/SVGTextFragment.h:106
> > +        result = result.isIdentity() ? lengthAdjustTransform : lengthAdjustTransform * result;
> 
> You copy transform to result first and copy lengthAdjustTransform to result afterwards. Can you try to optimize this? You may check x and y earlier from transformAroundOrigin to be more efficient.
Will check.

> 
> > Source/WebCore/svg/SVGTextContentElement.cpp:-39
> > -DEFINE_ANIMATED_LENGTH(SVGTextContentElement, SVGNames::textLengthAttr, TextLength, textLength)
> 
> Can you describe this a bit further please? Why do you handle it here manually? Does this affect the handling of textLength for animations? Especially when we support animVal and baseVal later?
Well we want to override the getter, normally we'd just always return m_textLength, if someone asks for textLength(BaseValue). Here we only want to return m_textLength, if it has been set, otherwhise return getComputedTextLength(). That's special, so we can't use the macros.

> 
> > Source/WebCore/svg/SVGTextContentElement.h:59
> > +    // textLength is not declared using the standard DECLARE_ANIMATED_LENGTH macro
> > +    // as its getter needs special handling (return getComputedTextLength(), instead of m_textLength).
> 
> Ok, at least the first question is answered.
Ok :-)

> 
> > LayoutTests/ChangeLog:10
> > +        This matches Opera and a discussion on www-svg some weeks ago.
> 
> Can you point me to this discussion again please? Maybe you can add the link to the ChangeLog as well.
Will lookup the link.

> 
> > LayoutTests/svg/dynamic-updates/script-tests/SVGTextElement-dom-lengthAdjust-attr.js:17
> > +shouldBeTrue("lastLength = textElement.getComputedTextLength(); lastLength > 50 && lastLength < 100");
> 
> So normal DOM is affected as well. Can you take a SVG Font, so that we have the exact length? The same for the other tests.
Nope, this will always differ, unfortunately. This approximation is enough. I just want to assure it's around the value, not exactly. If you look below, after we're changing lengthAdjust, the value is matched exactly.

[Nikolas Zimmermann]

(In reply to comment #7)
> > > Source/WebCore/rendering/svg/SVGTextFragment.h:106
> > > +        result = result.isIdentity() ? lengthAdjustTransform : lengthAdjustTransform * result;
> > 
> > You copy transform to result first and copy lengthAdjustTransform to result afterwards. Can you try to optimize this? You may check x and y earlier from transformAroundOrigin to be more efficient.
> Will check.
Switched to a more optimized version, as discussed on IRC.

> > 
> > Can you point me to this discussion again please? Maybe you can add the link to the ChangeLog as well.
> Will lookup the link.
Added a link to the relevant SVG minutes in the ChangeLog.

> 
> > 
> > > LayoutTests/svg/dynamic-updates/script-tests/SVGTextElement-dom-lengthAdjust-attr.js:17
> > > +shouldBeTrue("lastLength = textElement.getComputedTextLength(); lastLength > 50 && lastLength < 100");
> > 
> > So normal DOM is affected as well. Can you take a SVG Font, so that we have the exact length? The same for the other tests.
> Nope, this will always differ, unfortunately. This approximation is enough. I just want to assure it's around the value, not exactly. If you look below, after we're changing lengthAdjust, the value is matched exactly.
I chose to use lastLength >0 && lastLength < 200, to assure it's just different from 200, and not null.

[Dirk Schulze]

Comment on attachment 88928 [details]
Patch v2

Changes discussed on IRC and where commented in previous posts here. r=me. Great Patch!

[Nikolas Zimmermann]

Landed in r83710.