333698 - nested tspan don't inherit coordinate x and y properties correctly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Nuno Ponte]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060203 Fedora/1.5.0.1-1.1.fc4.nr Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060203 Fedora/1.5.0.1-1.1.fc4.nr Firefox/1.5.0.1

Leaf tspan elements don't inherit ancestor's coordinate properties x and y properly.
The text won't be correctly positioned.

Reproducible: Always

Steps to Reproduce:
1. Open the nested_tspan_bug.svg.xml attached file.

Comment 1

[Nuno Ponte]

Attachment: Test case demonstration of the bug

Shows the problem and the correct behaviour.

Comment 2

[Philip Withnall (unavailable)]

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a1) Gecko/20060413 Firefox/3.0a1 - Build ID: 0000000000
Confirmed.

Comment 3

[Alex Menzies]

Attachment: Initial patch attempt

Comment 4

[Alex Menzies]

Attachment: Test Case

This is a test for doubly nested tspan elements and the text-anchor property.  The text:

Isn't
SVG
Just
The Best

Should appear twice on the left side of the test.  The top version uses doubly nested tspan elements, while the bottom version uses no nesting at all.

On the right side of the test, several lines using different text-anchor and dx attributes are displayed.

Comment 5

[Alex Menzies]

Attachment: Text Path Anchor Testcase

This test case tests the use of the text-anchor attribute with the textPath element.  The last test checks to see dx and dy attributes inside a textPath are appropriately scaled according to the pathLength attribute.

Comment 6

[Alex Menzies]

Attachment: Test current text position on textPath

This test case explores the various ways a textPath can be affected when one if its children or a parent declares x, y, dx, or dy attributes.

Comment 7

[Robert Longson [:longsonr]]

Check out bug 319345 . I think it is related.

Are you looking for comments on your initial patch attempt?

Comment 8

[Alex Menzies]

(In reply to comment #7)

What I am looking for at the moment are some comments on the overall structure of my patch.  I have already found a few corner cases that I missed in my initial patch, so it's probably best to hold off on any detailed analysis until I post a new version with the necessary modifications.  Thanks!

Comment 9

[Alex Menzies]

Attachment: Updated Text Position on textPath Test Case

Old version had a test with a textPath element inside a tspan which is invalid according to the script.  This version also has some additional text on a path tests.

Comment 10

[Alex Menzies]

Attachment: patch - first revision

Updated patch handles text on a path correctly and passes all of the attached test cases.

Comment 11

[Robert Longson [:longsonr]]

Comment on attachment 236868 [details] [diff] [review]
patch - first revision

My 2c, for what its worth.

I think my fundamental structural concern is that this does not really use either the GetNextGlyphFragmentChildNode or GetNextGlyphFragment to traverse the fragment trees using functions defined in the node/fragment interfaces and I think, if possible it should. I'm not absolutely sure which one you want. The first is recursive and skips nsSVGGlyphFrames, the second traverses rather than recurses and includes nsSVGGlyphFrames. I see signs of both in the code below.


>         *val = data->GetLength()*percent/100.0f;
>-      } else if (pathFrame->GetContent()->HasAttr(kNameSpaceID_None, 
>+      } else if (pathFrame->GetContent()->HasAttr(kNameSpaceID_None,
>                                                   nsGkAtoms::pathLength)) {
>-         nsCOMPtr<nsIDOMSVGPathElement> pathElement = 
>+         nsCOMPtr<nsIDOMSVGPathElement> pathElement =
>                                      do_QueryInterface(pathFrame->GetContent());

These are the same to my eye. Could you have inserted DOS style LF/CR into these lines?

>         pathLength->GetAnimVal(&pl);
>-        if (pl) 
>+        if (pl)
>            *val *= data->GetLength() / pl;
>-        else 
>+        else
>            *val = 0;

As above.


>+PRBool
>+IsAbsolutelyPositioned(nsIFrame* frame)

I'm not sure why you implemented this here rather than using the implementation in nsSVGGlyphFrame.cpp. If you are going to change where it lives, I would have thought it made more sense to make this a member function of nsSVGTextContainerFrame it would not need any arguments then. 

>-  {
>-    nsCOMPtr<nsIDOMSVGLengthList> list = GetX();
>-    GetSingleValue(firstFragment, list, &x);
>+/*
>+ *  Performs a pre-order traversal on the subtree rooted at “node” until an
>+ *  absolutely positioned node is found or until the entire subtree has been
>+ *  explored.  Sum is increased by the total advance of all the nodes visited.
>+ *  Returns PR_TRUE if the subtree contained an absolutely positioned node.
>+ */
>+PRBool
>+GetSubTreeGlyphAdvance(nsIFrame* node,
>+                       nsSVGTextPathFrame** textPath,
>+                       float *sum)
>+{
>+  if (!node)
>+    return PR_FALSE;
>+
>+  if (node->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+    *textPath = NS_STATIC_CAST(nsSVGTextPathFrame*, node);
>+  } else if (node->GetType() == nsLayoutAtoms::svgTSpanFrame) {
>+    nsSVGTextContainerFrame *cf = NS_STATIC_CAST(nsSVGTextContainerFrame*,node);
>+
>+    float dx = 0;
>+    nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetDx();
>+    GetSingleValue(*textPath, list, &dx);
>+    *sum += dx;
>+
>+  } else if (node->GetType() == nsLayoutAtoms::svgGlyphFrame) {
>+    nsCOMPtr<nsISVGGlyphFragmentLeaf> fragment = do_QueryInterface(node);
>+    *sum += fragment->GetAdvance();
>   }
>-  {
>-    nsCOMPtr<nsIDOMSVGLengthList> list = GetY();
>-    GetSingleValue(firstFragment, list, &y);
>+
>+  nsIFrame* nextNode = node->GetFirstChild(nsnull);

Could you work with GetFirstGlyphFragmentChildNode and GetNextGlyphFragmentChildNode. Can GetSubTreeGlyphAdvance be part of the nsSVGGlyphFragmentNode interface?

>+  while (nextNode) {
>+    if (IsAbsolutelyPositioned(nextNode) ||
>+        GetSubTreeGlyphAdvance(nextNode, textPath, sum))
>+      return PR_TRUE;
>+    nextNode = nextNode->GetNextSibling();
>   }
>+  return PR_FALSE;
>+}
> 

>+/*
>+ *  Returns the total advancement for all glyphs between “node” and the next
>+ *  absolutely positioned node.
>+ */
>+float
>+GetChunkLength(nsIFrame* node, nsSVGTextPathFrame* textPath, nsIFrame* root)
>+{
>+  float sum = 0;
>+  while (node) {
>+    if (GetSubTreeGlyphAdvance(node, &textPath, &sum)) {
>+      return sum;
>+    //The subtree rooted at NODE doesn't contain any absolutely positioned nodes
>+    if (node->GetType() == nsLayoutAtoms::svgTextPathFrame)
>+      textPath = nsnull;
>+
>+    nsIFrame* nextNode = node->GetNextSibling();
>+    if (!nextNode) {
>+      //The entire subtree rooted at NODE's parent has been explored.  Go up
>+      //the tree until a parent with siblings is found or until we reach the
>+      //root textFrame element

This kind of navigation suggests you should be traversing with GetNextGlyphFragment and working with glyph fragments.

>+      nsIFrame* parent = node->GetParent();
>+      while (parent && node != root) {
>+        if (parent->GetType() == nsLayoutAtoms::svgTextPathFrame)
>+          textPath = nsnull;
>+        nextNode = parent->GetNextSibling();
>+        if (nextNode)
>           break;
>+        parent = parent->GetParent();
>       }
>     }

>+void
>+nsSVGTextFrame::UpdateGlyphPositioningHelper(nsIFrame* node,
>+                                    nsSVGTextPathFrame *textPath,
>+                                    nsCOMPtr<nsISVGGlyphFragmentLeaf> *fragment,
>+                                    float *x, float *y)
>+{

>+  if (node->GetType() == nsLayoutAtoms::svgTextFrame ||
>+      node->GetType() == nsLayoutAtoms::svgTSpanFrame ||
>+      node->GetType() == nsLayoutAtoms::svgTextPathFrame)

Use CallQueryInterface and see if the node supports nsISVGTextContentMetrics instead of the above tests.

>+  {
>+    nsSVGTextContainerFrame *cf = NS_STATIC_CAST(nsSVGTextContainerFrame*,node);

Nit: space after comma.

>+    if (node->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+      textPath = NS_STATIC_CAST(nsSVGTextPathFrame*, node);
>+      *fragment = nsnull;
>+      *x = *y = 0;
>+    } else {
>       {
>-        nsCOMPtr<nsIDOMSVGLengthList> list = fragment->GetDx();
>-        GetSingleValue(fragment, list, &dx);
>+        nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetX();
>+        GetSingleValue(textPath, list, x);
>       }
>       {
>-        nsCOMPtr<nsIDOMSVGLengthList> list = fragment->GetDy();
>-        GetSingleValue(fragment, list, &dy);
>+        nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetY();
>+        GetSingleValue(textPath, list, y);
>+      }
>+    }
>+    {
>+      float dx = 0;
>+      nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetDx();
>+      GetSingleValue(textPath, list, &dx);
>+      *x += dx;
>+    }
>+    {
>+      float dy = 0;
>+      nsCOMPtr<nsIDOMSVGLengthList> list = cf->GetDy();
>+      GetSingleValue(textPath, list, &dy);
>+      *y += dy;
>+    }
>+
>+    PRUint8 anchor = node->GetStyleSVG()->mTextAnchor;
>+    if (anchor != NS_STYLE_TEXT_ANCHOR_START && IsAbsolutelyPositioned(node)) {
>+      float chunkLength = GetChunkLength(node, textPath, this);
>+      if (anchor == NS_STYLE_TEXT_ANCHOR_MIDDLE)
>+        *x -= chunkLength/2.0f;
>+      else if (anchor == NS_STYLE_TEXT_ANCHOR_END)
>+        *x -= chunkLength;
>+    }
>+  } else if (node->GetType() == nsLayoutAtoms::svgGlyphFrame) {
>+    nsCOMPtr<nsISVGGlyphFragmentLeaf> curFrag = do_QueryInterface(node);
>+    float baseline_offset = curFrag->GetBaselineOffset(GetBaseline());
>+    curFrag->SetGlyphPosition(*x, *y - baseline_offset);
>+    *x += curFrag->GetAdvance();
>+
>+    if (textPath && curFrag->GetNumberOfChars() > 0) {
>+      nsIDOMSVGPoint* pt;
>+      curFrag->GetEndPositionOfChar(0, &pt);
>+      if (pt) {
>+        *fragment = curFrag;
>       }
>+    }
>+  }
> 
>-      float baseline_offset = fragment->GetBaselineOffset(baseline);
>-      fragment->SetGlyphPosition(x + dx, y + dy - baseline_offset);
>+  nsIFrame* nextNode = node->GetFirstChild(nsnull);
>+  while (nextNode) {
>+    UpdateGlyphPositioningHelper(nextNode, textPath, fragment, x, y);
>+    nextNode = nextNode->GetNextSibling();
>+  }
> 
>-      x += dx + fragment->GetAdvance();
>-      y += dy;
>-      fragment = fragment->GetNextGlyphFragment();
>-      if (fragment && fragment->IsAbsolutelyPositioned())
>-        break;
>+  if (node->GetType() == nsLayoutAtoms::svgTextPathFrame) {
>+    if (*fragment) {
>+      nsIDOMSVGPoint* pt;
>+      PRUint32 numChars = (*fragment)->GetNumberOfChars();
>+      for (PRUint32 i = numChars - 1; i >= 0; --i) {
>+        (*fragment)->GetEndPositionOfChar(i, &pt);

This is going to be very slow. I think you need a function to all the positions at once and return them in an array if you need to do this.

>+        if (pt) {
>+          pt->GetX(x);

>Index: layout/svg/base/src/nsSVGTextFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGTextFrame.h,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 nsSVGTextFrame.h
>--- layout/svg/base/src/nsSVGTextFrame.h	28 Jun 2006 15:23:40 -0000	1.1
>+++ layout/svg/base/src/nsSVGTextFrame.h	5 Sep 2006 21:09:22 -0000
>@@ -37,16 +37,17 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef NS_SVGTEXTFRAME_H
> #define NS_SVGTEXTFRAME_H
> 
> #include "nsSVGTextContainerFrame.h"
> #include "nsISVGValueObserver.h"
> #include "nsWeakReference.h"
>+#include "nsSVGTextPathFrame.h"

You only need class nsSVGTextPathFrame here.

>+  /*
>+   *  Performs a pre-order traversal on the subtree rooted at “node” and updates
>+   *  the position of each glyph fragment as its svgGlyphFrame is visited.
>+   *  node     – may be any type of frame that can be contained within an
>+   *             svgTextFrame
>+   *  textPath – node's most recent ancestor of type svgTextPathFrame.  nsnull
>+   *             if no such ancestor exists.
>+   *  fragment - If node has an ancestor of type svgTextPathFrame, then fragment
>+   *             is the most recent glyph fragment to have drawn at least 1
>+   *             character onto the path.

Shouldn't this function argument be an ordinary pointer rather than an XPCOM thing?

>+   *  x, y     – absolute coordinates containing the current text position.
>+   */
>+  void UpdateGlyphPositioningHelper(nsIFrame* node,
>+                                    nsSVGTextPathFrame *textPath,
>+                                    nsCOMPtr<nsISVGGlyphFragmentLeaf> *fragment,
>+                                    float *x, float *y);

Comment 12

[Robert Longson [:longsonr]]

A couple of other things...

There are strange characters in the patch that appear as diamonds with a question mark in them (also visible in my previous comment post).


+      PRUint32 numChars = (*fragment)->GetNumberOfChars();
+      for (PRUint32 i = numChars - 1; i >= 0; --i) {
+        (*fragment)->GetEndPositionOfChar(i, &pt);
+        if (pt) {
+          pt->GetX(x);
+          pt->GetY(y);
+          break;

Actually you don't need to return all the position data do you, just write a new method that returns the end position of the last valid character.

Comment 13

[Alex Menzies]

Thank you very much for your insightful feedback.  I have been successful in implementing many of the changes you recommended but have run into problems with the ones mentioned below. 

>I think my fundamental structural concern is that this does not really use 
>either the GetNextGlyphFragmentChildNode or GetNextGlyphFragment to traverse 
>the fragment trees...

I agree that it would be nice to use the pre-existing GlyphFragment code to traverse the tree, but I have had some difficulty in making this approach work.  The major problem I have run into is that calling GetDx/GetDy on a nsISVGGlyphFragmentLeaf does not necessarily return the offset for that fragment.  

Take the following svg snippet for example
    <tspan dy="18"><tspan>T</tspan>ext</tspan>

Calling GetDy on the “T” fragment will return the dy of its parent, which is 0.  Similarly, calling GetDy on “ext” will return 18.  For this reason, my patch avoids calling GetDx/GetDy on fragments by traversing over the frames.  Another possible solution would be to have each fragment walk up the tree, summing the offsets of its parent frames, until a node that is not the first child of its parent is reached.  Unfortunately, we still need to be able determine if we are inside a textPath in order to scale the offsets by pathLength if necessary, so for the average case we will have to walk the tree all the way to the root for each fragment we process.  This would result in more tree walking but might fit more neatly into the existing code.

>These are the same to my eye. Could you have inserted DOS style LF/CR into
> these lines?
>>-        if (pl) 
>>+        if (pl)

The (subtle) difference here is that trailing white space has been removed.  


>>+PRBool
>>+IsAbsolutelyPositioned(nsIFrame* frame)

>I'm not sure why you implemented this here rather than using the 
>implementation in nsSVGGlyphFrame.cpp. If you are going to change where it 
>lives, I would have thought it made more sense to make this a member function 
>of nsSVGTextContainerFrame it would not need any arguments then. 

I reimplemented IsAbsolutelyPositioned because I needed function to determine if a given frame defined a new absolute position.  I have not been able to find a clean way of using nsSVGGlyphFrame::IsAbsolutelyPositioned to do this.  If we decide to iterate over the fragments then I can use the original version, otherwise I can move my version into nsSVGTextContainerFrame and possibly remove the old version depending on what else needs it.

>Can GetSubTreeGlyphAdvance be part of the nsSVGGlyphFragmentNode interface?
>I think this will work if we iterate over the fragments.

>There are strange characters in the patch that appear as diamonds with a 
>question mark in them (also visible in my previous comment post).

I'm afraid I haven't been able to find the bizarre characters you describe, could you give me a line on which they appear?

Comment 14

[Robert Longson [:longsonr]]

(In reply to comment #13)
> I agree that it would be nice to use the pre-existing GlyphFragment code to
> traverse the tree, but I have had some difficulty in making this approach work.
>  The major problem I have run into is that calling GetDx/GetDy on a
> nsISVGGlyphFragmentLeaf does not necessarily return the offset for that
> fragment.  
> 
> Take the following svg snippet for example
>     <tspan dy="18"><tspan>T</tspan>ext</tspan>
> 
> Calling GetDy on the �T� fragment will return the dy of its parent, which is 0.
>  Similarly, calling GetDy on �ext� will return 18.  For this reason, my patch

Around the T and the ext I see strange diamond characters.

> avoids calling GetDx/GetDy on fragments by traversing over the frames.  Another
> possible solution would be to have each fragment walk up the tree, summing the
> offsets of its parent frames, until a node that is not the first child of its
> parent is reached.  Unfortunately, we still need to be able determine if we are
> inside a textPath in order to scale the offsets by pathLength if necessary, so
> for the average case we will have to walk the tree all the way to the root for
> each fragment we process.  This would result in more tree walking but might fit
> more neatly into the existing code.

I think that's the way I'd go, we've got FindTextPathParent. We could always optimise it later by putting in some boolean member variable to hold whether we're the child of a text path although we'd have to track that against DOM changes. We don't need to go back to the root though, just to the first parent text element we find. If we haven't found a text path by then we're not going to.

> 
> >These are the same to my eye. Could you have inserted DOS style LF/CR into
> > these lines?
> >>-        if (pl) 
> >>+        if (pl)
> 
> The (subtle) difference here is that trailing white space has been removed.  

OK.

> I reimplemented IsAbsolutelyPositioned because I needed function to determine
> if a given frame defined a new absolute position.  I have not been able to find
> a clean way of using nsSVGGlyphFrame::IsAbsolutelyPositioned to do this.  If we
> decide to iterate over the fragments then I can use the original version,
> otherwise I can move my version into nsSVGTextContainerFrame and possibly
> remove the old version depending on what else needs it.

I think that just the code you are rewriting uses it :)

> >There are strange characters in the patch that appear as diamonds with a 
> >question mark in them (also visible in my previous comment post).
> 
> I'm afraid I haven't been able to find the bizarre characters you describe,
> could you give me a line on which they appear?
> 

See above.

Comment 15

[Robert Longson [:longsonr]]

(In reply to comment #13)

> I agree that it would be nice to use the pre-existing GlyphFragment code to
> traverse the tree, but I have had some difficulty in making this approach work.
>  The major problem I have run into is that calling GetDx/GetDy on a
> nsISVGGlyphFragmentLeaf does not necessarily return the offset for that
> fragment.  
> 
> Take the following svg snippet for example
>     <tspan dy="18"><tspan>T</tspan>ext</tspan>
> 
> Calling GetDy on the �T� fragment will return the dy of its parent, which is 0.
>  Similarly, calling GetDy on �ext� will return 18.  For this reason, my patch
> avoids calling GetDx/GetDy on fragments by traversing over the frames.  Another
> possible solution would be to have each fragment walk up the tree, summing the
> offsets of its parent frames, until a node that is not the first child of its
> parent is reached.  Unfortunately, we still need to be able determine if we are
> inside a textPath in order to scale the offsets by pathLength if necessary, so
> for the average case we will have to walk the tree all the way to the root for
> each fragment we process.  This would result in more tree walking but might fit
> more neatly into the existing code.

Or perhaps you could extend the fragment (or node) interface to pass along whether the node is a child of a text path in the getNext... arguments?

Comment 16

[Alex Menzies]

I am reworking the code so that it iterates over glyph fragments, but have run into a problem that might occur given the following svg code:

  <text> Hello <tspan dx="50"/> World </text>

Here we see a tspan element with no glyph fragments as children.  It is unclear to me what the correct behavior is under these conditions, but logic would seem to dictate that the current text position should be advanced by 50 after the word hello.  ASV renders the above code with the offset while Batik and Opera ignore it.  The problem I am having with iterating over the fragments is that since the tspan is childless, I have no way of knowing that it exists so I cannot apply to offset.  Any suggestions for a work around?  Perhaps a frame based approach might end up being cleaner?

Comment 17

[Jonathan Watt [:jwatt]]

It would be nice to stay away from frames where possible I think, so that the user can use JavaScript to obtain measurements of the text when there are no frames for the content.

Comment 18

[Robert Longson [:longsonr]]

(In reply to comment #16)
> I am reworking the code so that it iterates over glyph fragments, but have run
> into a problem that might occur given the following svg code:
> 
>   <text> Hello <tspan dx="50"/> World </text>
> 
> Here we see a tspan element with no glyph fragments as children.  It is unclear
> to me what the correct behavior is under these conditions, but logic would seem
> to dictate that the current text position should be advanced by 50 after the
> word hello.  ASV renders the above code with the offset while Batik and Opera
> ignore it.  The problem I am having with iterating over the fragments is that
> since the tspan is childless, I have no way of knowing that it exists so I
> cannot apply to offset.  Any suggestions for a work around?  Perhaps a frame
> based approach might end up being cleaner?
> 

Perhaps you should iterate using GetNextGlyphFragmentChildNode. That will include the elements as well as the fragments in the traverse allowing you to track what's going on more easily.

Comment 19

[FT2]

Visiting to report/confirm - see Wikimedia bug report

https://bugzilla.wikimedia.org/show_bug.cgi?id=13580

Comment 23

[Brandon Mitchell]

(In reply to comment #22):

> *** Bug 555436 has been marked as a duplicate of this bug. ***
>
> <tspan y="120" x="0"><tspan>Line 4</tspan></tspan>
> 
> should be treated the same as
> 
> <tspan><tspan y="120" x="0">Line 4</tspan></tspan>
> 
> but as bug 333698 says, it doesn't inherit the coordinates

I'm sorry if I'm being dense, but why does it render *correctly* if I insert a newline after the outer tspan tag???

<tspan y="120" x="0">
                     <tspan>Line 4</tspan></tspan>

Aren't newlines normally ignored (or collapsed into a single space) inside tspan, unless space-preserving is explicitly requested?

Comment 24

[Robert Longson [:longsonr]]

you're pretty much there. The spaces and newlines collapse to one space so the markup becomes

<tspan y="120" x="0"> <tspan>Line 4</tspan></tspan>

and then we can draw a space at 120, 0 followed by the text.

Without the whitespace we have no hook to hang onto.

Comment 25

[Brandon Mitchell]

Your explanation makes sense; however, it doesn't seem like a space is actually drawn at 0, 120, as I can see the serif "L" in "Line" contacting the edge of the client area (whereas I do see a space if I use   for NBSP). So apparently the encountered whitespace acts as a hook even if it is not drawn.

Comment 26

[Brian Birtles (:birtles)]

Where are we up to with this? We have a patch from 2006. Alex are you still working on this?

Comment 27

[Robert Longson [:longsonr]]

I think it best to start again. The code now is quite different to what it was then.

<text x="1, 2, 3, 4, 5">ab<tspan x="6">cd</tspan>e</text>

I think we'd want to build up arrays of positions data when we did glyph positioning that we pushed as arguments. When you draw the text above you need to push the x="3, 4, 5" to the tspan, it then overwrites this to make x="6, 4, 5" and uses the 6 and 4. Finally we pop that and go back to 1, 2, 3, 4, 5 knowing we're at index 5 to draw the e.

We'd need to keep the position data in the glyph frame so that when we came to paint it we could use it.

Comment 28

[Robert Longson [:longsonr]]

Actually a better way might be to create a single copy of the data and avoid the push pop though you'll need to be careful not to go past the length of the constituent elements e.g.

<text x="1, 2, 3, 4, 5">ab<tspan x="6, 7, 8">cd</tspan>e</text>

The 8 needs to be ignored so you'd need to create x="1, 2, 6, 7, 5". If you cache the lengths of the text and tspans this may well be more performant.

Comment 29

[Takeshi Kurosawa :takenspc]

Attachment: Patch

Patch (main part)

I'll post reftest part shortly.

Comment 30

[Takeshi Kurosawa :takenspc]

Attachment: Patch (reftest)

Reftest part

Comment 31

[Robert Longson [:longsonr]]

The first two examples in attachment 236867 [details] are different in Opera. Can you look into why that is please?

Comment 32

[Robert Longson [:longsonr]]

Comment on attachment 471377 [details] [diff] [review]
Patch

>+NS_IMETHODIMP_(void)
>+nsSVGGlyphFrame::SetOffset(PRUint32 offset)
>+{
>+  mOffset = offset;
>+}

It looks like this is the start index into the position and rotation data. If so
then SetStartIndex/mStartIndex is a rather better name. If not then you still
need something better than SetOffset/mOffset as it's not clear at first glance
what that is and it's easily confused with the textPath startOffset.

>+
>+NS_IMETHODIMP_(void)
>+nsSVGGlyphFrame::GetEffectiveXY(nsTArray<float> &aX, nsTArray<float> &aY)
>+{
>+  nsSVGTextContainerFrame *containerFrame;
>+  containerFrame = static_cast<nsSVGTextContainerFrame *>(mParent);

Combine the above lines.

>+  if (!containerFrame)
>+    return;

How can this be? Assert instead perhaps?

>+
>+  nsTArray<float> x, y;
>+  containerFrame->GetEffectiveXY(x, y);
>+
>+  PRUint32 strLength = GetNumberOfChars();
>+  PRUint32 xCount = (x.Length() > mOffset) ? x.Length() - mOffset : 0;

NS_MIN(x.Length() - mOffset, 0)

>+  xCount = PR_MIN(xCount, strLength);

Use NS_MIN, rather than PR_MIN throughout.

>+  if (xCount > 0) {
>+    aX.AppendElements(x.Elements() + mOffset, xCount);
>+  }

Is the if (xCount > 0) required?

> nsSVGGlyphFrame::GetDxDy(SVGUserUnitList *aDx, SVGUserUnitList *aDy)
> {
>   nsSVGTextContainerFrame *containerFrame;
>   containerFrame = static_cast<nsSVGTextContainerFrame *>(mParent);

Combine the above lines?

>   if (containerFrame)

Assert rather than test as this can't fail can it?

>     containerFrame->GetDxDy(aDx, aDy);

In fact you don't need a containerFrame variable since you can just
static cast the mParent and then call the method.

>+void
>+nsSVGGlyphFrame::GetEffectiveRotate(nsTArray<float> &aRotate)
>+{

Need to add a comment on what effective rotate means (as opposed to rotate)

>+
>+void
>+nsSVGTextContainerFrame::CopyPositionList(nsTArray<float> *parentList,
>+                                          SVGUserUnitList *selfList,
>+                                          nsTArray<float> &dstList,
>+                                          PRUint32 aOffset)
>+{
>+  PRUint32 strLength = nsSVGTextContainerFrame::GetNumberOfChars();

is nsSVGTextContainerFrame:: required?

>+  PRUint32 parentCount = 0;
>+  if (parentList && parentList->Length() > aOffset) {
>+    parentCount = PR_MIN(parentList->Length() - aOffset, strLength);
>+  }
>+
>+  PRUint32 selfCount = selfList->Length();
>+  selfCount = PR_MIN(selfCount, strLength);

PRUint32 selfCount = NS_MIN(selfList->Length(), strLength);

>+
>+  PRUint32 count = PR_MAX(parentCount, selfCount);
>+  if (!dstList.SetLength(count))
>+    return;
>+  for (PRUint32 i = 0; i < count; i++) {
>+    if (selfCount <= i) {
>+      dstList[i] = (*parentList)[aOffset + i];
>+    } else {
>+      dstList[i] = (*selfList)[i];
>+    }
>+  }

Maybe have two loops one for selfCount<=i and the other for not rather
than testing in the loop.

>+}
>+
>+void
>+nsSVGTextContainerFrame::CopyPositionList(nsTArray<float> *parentList,
>+                                          nsCOMPtr<nsIDOMSVGNumberList> selfList,
>+                                          nsTArray<float> &dstList,
>+                                          PRUint32 aOffset

Better called CopyRotateList since that's what it is.

)


>+PRUint32
>+nsSVGTextContainerFrame::BuildPositionList(PRUint32 aOffset,
>+                                           PRUint32 aDepth)
>+{
>+  nsSVGTextContainerFrame *parent = do_QueryFrame(mParent);
>+  nsTArray<float> *parentX, *parentY, *parentDx, *parentDy, *parentRotate;
>+  parentX = parent ? &(parent->mX) : nsnull;
>+  parentY = parent ? &(parent->mY) : nsnull;
>+  parentDx = parent ? &(parent->mDx) : nsnull;
>+  parentDy = parent ? &(parent->mDy) : nsnull;
>+  parentRotate = parent ? &(parent->mRotate) : nsnull;

Probably better as a single if (parent) test.

>+
>+  SVGUserUnitList x, y;
>+  GetXY(&x, &y);
>+  mX.Clear();
>+  mY.Clear();

Move the Clear calls into CopyPositionList.

>+  PRUint32 offset = 0;
>+  nsIFrame* kid = mFrames.FirstChild();
>+  while (kid) {
>+    nsSVGTextContainerFrame *text = do_QueryFrame(kid);
>+    nsISVGGlyphFragmentLeaf *leaf = do_QueryFrame(kid);
>+    if (text) {
>+      offset += text->BuildPositionList(offset, aDepth+1);

Spaces around +

>+{
>+  if (!mX.IsEmpty()) {
>+    aX.AppendElements(mX.Elements(), mX.Length());
>+  }

Is the if test required?

The above comments apply in more than one location as the location and rotation code is pretty similar

Pretty good overall. I'd like to see a new patch with some answer to comment 31 though.

Comment 33

[Robert Longson [:longsonr]]

Comment on attachment 471396 [details] [diff] [review]
Patch (reftest)

>diff --git a/layout/reftests/svg/reftest.list b/layout/reftests/svg/reftest.list
>--- a/layout/reftests/svg/reftest.list
>+++ b/layout/reftests/svg/reftest.list
>@@ -161,16 +161,38 @@ random-if(gtk2Widget) != text-language-0
> == text-layout-02.svg text-layout-02-ref.svg
> == text-scale-01.svg text-scale-01-ref.svg
> == text-style-01a.svg text-style-01-ref.svg
> == text-style-01b.svg text-style-01-ref.svg
> == text-style-01c.svg text-style-01-ref.svg
> == text-style-01d.svg text-style-01-ref.svg
> == text-style-01e.svg text-style-01-ref.svg
> == thin-stroke-01.svg pass.svg
>+== tspan-dxdy-01.svg tspan-dxdy-ref.svg
>+== tspan-dxdy-02.svg tspan-dxdy-ref.svg
>+== tspan-dxdy-03.svg tspan-dxdy-ref.svg
>+== tspan-dxdy-04.svg tspan-dxdy-ref.svg
>+== tspan-dxdy-05.svg tspan-dxdy-ref.svg
>+== tspan-dxdy-06.svg tspan-dxdy-ref.svg

Call the file tspan-dxdy-01-ref.svg

Similarly with the other ref files.
r=longsonr with that.

Comment 34

[Robert Longson [:longsonr]]

Comment on attachment 471377 [details] [diff] [review]
Patch

> +  // for each character within a 'text', 'tspan', 'tref' and 'altGlyph' element
> +  // which has an x or y attribute value assigned to it explicitly
> +  nsTArray<float> x, y;
> +  GetEffectiveXY(x, y);
> +  if (!x.IsEmpty() || !y.IsEmpty()) {
> +    return PR_TRUE;
> +  }
> +
   return PR_FALSE;

One more thing... This can become 

   return (!x.IsEmpty() || !y.IsEmpty());

Comment 35

[Takeshi Kurosawa :takenspc]

(In reply to comment #31)
> The first two examples in attachment 236867 [details] are different in Opera. Can you
> look into why that is please?

SVG 1.1 Spec says that
http://www.w3.org/TR/SVG/text.html#TextpathLayoutRules

... any ‘x’ attributes on ‘text’, ‘tspan’, ‘tref’ or ‘altGlyph’ elements represent new absolute offsets along the path, thus providing explicit new values for startpoint-on-the-path. Any ‘y’ attributes on ‘text’, ‘tspan’, ‘tref’ or ‘altGlyph’ elements are ignored.

But we currently doesn't ignore first value of 'y' attribute in nsSVGGlyphFrame::GetCharacterPositions. (and doesn't uses multiple 'x' values)
In the next patch, I'll incorporate that.

Comment 36

[Robert Longson [:longsonr]]

Perhaps you could fold the first bit of bug 593813 comment 4 if that's not too much trouble and doesn't break anything.

Comment 37

[Takeshi Kurosawa :takenspc]

Attachment: Patch (main part)

To fix textPath + y positioning, I introduced mPositionDelta which holding dx and dy value of the first character of the nsSVGGlyphFrame.
This also fixes bug 593813.

Comment 38

[Takeshi Kurosawa :takenspc]

Comment on attachment 474620 [details] [diff] [review]
Patch (main part)

Sorry, this patch causes various rendering issues :-(

Comment 39

[Takeshi Kurosawa :takenspc]

Attachment: Patch rv.3

New patch. This doesn't use position delta

Comment 40

[Robert Longson [:longsonr]]

Comment on attachment 474664 [details] [diff] [review]
Patch rv.3

> 
>   // Owning pointer, must call gfxTextRunWordCache::RemoveTextRun before deleting
>   gfxTextRun *mTextRun;
>   gfxPoint mPosition;
>   PRUint8 mWhitespaceHandling;
>+  // The start index into the position and rotation data
>+  PRUint32 mStartIndex;

mStartIndex must come before mWhitespaceHandling as we should have members in size order.

Do you have check in rights? If not can you provide a patch with this change in please?

All of the tests attached to this bug pass now don't they?

Comment 41

[Takeshi Kurosawa :takenspc]

Attachment: Patch rv.3.1

Comment addressed patch combining codes + tests.

I manually tests all attachments of this bug.
I believe all of tests pass although the Test Six and Test Seven of attachment 236867 [details] differ from other UAs.
This is caused by two reasons.

1. Mozilla takes into |pathLength| in textPath processing, others not.
2. Calculation of x position of textPath is vary with UAs.

For point 2, in case like below,
  
 <text>
   <textPath startOffset="77">
     <tspan x="33" dx="55">Test Seven</tspan>
   </textPath>
 </text>

Mozilla does
  X = @dx + @startOffset

Opera (at least 10.62) does
  if there is @startOffset
    X = @dx + @startOffset
  else
    X = @x + @dx

Chrome and Batik do
  X = @x + @dx + @startOffset

The attachment 236867 [details] doesn't contain @startOffset (treated as 0 except Opera).
Can I separate textPath issue from this bug?

Comment 42

[Robert Longson [:longsonr]]

(In reply to comment #41)
> Can I separate textPath issue from this bug?

Sure, I'm not sure either of those issues above are actually bugs. It's worth raising each one with w3c.

Comment 43

[Robert Longson [:longsonr]]

This is a great improvement to our text positioning. I think we should take this as we are really the odd UI out without it. Having a text element define positions while using tspans to do effects is relatively common and generally broken without this patch.

It's a fair sized piece of work but it's not terribly complicated in implementation.

Comment 44

[Brian Birtles (:birtles)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/909665f6e951
http://hg.mozilla.org/mozilla-central/rev/6271366695f9

Thanks Taken-san!