339039 - Text spans return incorrect number of characters with getNumberOfChars()

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Thomas Wisniewski]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060523 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060523 Minefield/3.0a1

When multiple tspans are in a text element, the number of characters in those tspans considers all sibling elements after the one being considered.

Reproducible: Always

Steps to Reproduce:
1. View page given in "addition information"
2.
3.

Actual Results:  
t1: 19
t2: 14
t3: 9
t4: 4

Expected Results:  
t1: 4
t2: 4
t3: 4
t4: 4

<?xml version="1.0" ?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg xmlns="http://www.w3.org/2000/svg" onload="window.setTimeout('delay()', 1000);">
	<script><![CDATA[
		function delay() {
			alert( "t1:" + document.getElementById("t1").getNumberOfChars() + "\nt2:" + document.getElementById("t2").getNumberOfChars() + "\nt3:" + document.getElementById("t3").getNumberOfChars() + "\nt4:" + document.getElementById("t4").getNumberOfChars() );
		}
	]]></script>
	<text fill="black" transform="translate(0,20)">
		<tspan id="t1">Test</tspan>
		<tspan id="t2">Test</tspan>
		<tspan id="t3">Test</tspan>
		<tspan id="t4">Test</tspan>
	</text>
</svg>

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Testcase posted by the author.

Attached the testcase described by the author.

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

If we traverse to a frame with a different parent than that of our initiating frame then we've run out of child frames. Testcase now produces expected results.

Comment 3

[tor]

Offhand it seems that we're adding this root parameter everywhere to keep working in an interative fashion instead of using recursion.  Whether that's desired or not is another question.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is there a reason to not use recursion?

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 223178 [details] [diff] [review]
patch

I'll try to figure out how to do it as you suggest.

Comment 6

[Robert Longson [:longsonr]]

Attachment: recursive implementation with additional fixes

GetNumberOfChars, GetComputedTextLength, GetSubStringLength and GetCharNumAtPosition now implemented recursively.

Also fixed problems with textPaths if they are not the first child of <text>

Moved duplicated code from nsSVGTextFrame and nsSVGTSpanFrame into nsSVGUtils:
BuildGlyphFragmentTree, GetFirstGlyphFragmentChildNode and GetNextGlyphFragmentChildNode

Comment 7

[Robert Longson [:longsonr]]

Attachment: fix argument validation

I realised the validation of arguments is incorrect. The previous patch contains useless checks that unsigned value are >= 0 and misses out the validation check that we have characters before trying to determine their length.

Comment 8

[tor]

Comment on attachment 223896 [details] [diff] [review]
fix argument validation

For new code, CallQueryInterface should be used instead of do_QueryInterface.

	

nsSVGGlyphFrame::GetCharacterPosition and nsSVGGlyphFrame::IsChildOfTextPath contain much the same code.  Rename and slightly modify the latter to FindTextPathParent to share code?

nsSVGGlyphFrame::GetCharNumAtPosition adjusts the point for non-textpath children.  Why not leave this for nsSVGCairoGlyphMetrics::GetCharNumAtPosition?

Instead of passing mFrames.FirstChild() to all the nsSVGUtil methods, why not have the methods accept mFrameList so mFrames can be passed directly?  

nsSVGUtils text methods that only set a single out parameter and always return NS_OK should be changed to just return the value.

For things like GetSubStringLength and GetStartPositionOfChar you have sanity checks on the arguments in the frame code.  Why not move these to the nsSVGUtil methods?

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comments

> For new code, CallQueryInterface should be used instead of do_QueryInterface.

Done.

> nsSVGGlyphFrame::GetCharacterPosition and nsSVGGlyphFrame::IsChildOfTextPath
> contain much the same code.  Rename and slightly modify the latter to
> FindTextPathParent to share code?

Done.

> nsSVGGlyphFrame::GetCharNumAtPosition adjusts the point for non-textpath
> children.  Why not leave this for nsSVGCairoGlyphMetrics::GetCharNumAtPosition?

Done for all methods that need to modify their results for non-textpath children.

> Instead of passing mFrames.FirstChild() to all the nsSVGUtil methods, why not
> have the methods accept mFrameList so mFrames can be passed directly?  

Done.

> nsSVGUtils text methods that only set a single out parameter and always return
> NS_OK should be changed to just return the value.

Done.

> For things like GetSubStringLength and GetStartPositionOfChar you have sanity
> checks on the arguments in the frame code.  Why not move these to the nsSVGUtil
> methods?

The nsSVGUtils methods are called recursively from nsSVGTSpanFrame.cpp. If I moved the sanity checks to nsSVGUtils then they would be called for every node, however they only need to validate the user supplied arguments are OK. After that arguments must be valid all the way down the recursion stack. 

The sanity check code is rather shorter now that your other comments have been actioned.

Comment 10

[tor]

Comment on attachment 224195 [details] [diff] [review]
address review comments

It seems that whenever GetGlyphPositionX is called, so is GetGlyphPositionY.  Combine into one method?

You have a number of methods in nsSVGCairoGlyphMetrics calling FindTextPathParent to check if an offset needs to be done.  This doesn't need to be called, as the !cp branch of the code means you're in that circumstance.

With the previous corrected, the only place FindTextPathParent is called is in nsSVGGlyphFrame::GetCharacterPosition, so the code could be moved there.

Comment 11

[Robert Longson [:longsonr]]

Attachment: address additional review comments

> It seems that whenever GetGlyphPositionX is called, so is GetGlyphPositionY. 
> Combine into one method?

Done.

> You have a number of methods in nsSVGCairoGlyphMetrics calling
> FindTextPathParent to check if an offset needs to be done.  This doesn't need
> to be called, as the !cp branch of the code means you're in that circumstance.

Done.

> With the previous corrected, the only place FindTextPathParent is called is in
> nsSVGGlyphFrame::GetCharacterPosition, so the code could be moved there.

As we were :-)

Comment 12

[Robert Longson [:longsonr]]

Attachment: without IsEmpty changes

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why can't error checks like

+  if (charnum >= nsSVGUtils::GetNumberOfChars(&mFrames)) {
+    *_retval = nsnull;
+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
+  }

be folded into the nsSVGUtils functions that follow?

Comment 14

[Robert Longson [:longsonr]]

In the nsISVGGlyphFragmentNode interface in nsSVGTSpanFrame I call nsSVGUtils::GetSubStringLength recursively. The arguments must be valid at that point as long as the original arguments passed validation in the nsISVGTextContentMetrics implementation (nsSVGTSpanFrame and nsSVGTextFrame).

I therefore put all the argument validation in the same files - nsSVGTSpanFrame and nsSVGTextFrame, rather than having some nsSVGUtils functions do validation and some not. 

Validation for those functions that work iteratively (GetStartPositionOfChar, GetEndPositionOfChar, GetExtentOfChar and GetRotationOfChar) rather than recursively (GetSubStringLength) could be moved to nsSVGUtils without affecting performance.

So I see 3 alternatives:
a) Move all validation to nsSVGUtils
b) Move non-recursive function validation to nsSVGUtils
c) As I am, possibly with additional code comments.

I'm happy to go with your preferred option as Tor asked the same question in comment 8.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I vote for option d), move all validation to nsSVGUtils, and for GetSubStringLength, validate the parameters and then call a GetSubStringLengthRecursive function (which could be a static global function).

Comment 16

[Robert Longson [:longsonr]]

Attachment: option d)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How do we know that TSpanFrame::GetSubStringLength can only be called by GetSubStringLengthRecursive?

Comment 18

[Robert Longson [:longsonr]]

nsSVGTSpanFrame implements two interfaces, one which must validate its arguments and one which does not have to.

I could create a new class nsSVGTextContainerFrame say, inheriting from nsSVGDisplayContainerFrame acting as a base class for nsSVGTSpanFrame, nsSVGTextFrame and possibly nsSVGTextPathFrame. The new class could implement all the common functionality rather than having it in nsSVGUtils. That way making the methods protected would limit their use somewhat although it would not be foolproof.

Alternatively we could just always validate the arguments for both interfaces and not have a nsSVGUtils::GetSubStringLengthRecursive function at all.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 224851 [details] [diff] [review]
option d)

Okay, let's not call it *Recursive, let's call it *NoValidation instead.

Comment 20

[Robert Longson [:longsonr]]

Attachment: version checked in

Checked in. With NoValidation change.