282579 - Implement <svg:textPath>

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Paul Ortyl]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050210 Firefox/1.0 (Debian package 1.0+dfsg.1-6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050105 Debian/1.7.5-1

The text on the path in the example below is not shown, the arc should be part
of a circle, it is not.

<?xml version="1.0" encoding="iso-8859-1"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 20001102//EN"
"http://www.w3.org/TR/2000/CR-SVG-20001102/DTD/svg-20001102.dtd">
<svg viewBox="0 0 200 100" zoomAndPan="disable" 
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <path id="p" d="M15 100a85 85 0 0 1 170 0" stroke="blue" fill="none"/>
  <text fill="black" text-anchor="middle">
  <textPath xlink:href="#p" startOffset="50%">centered text on a path</textPath>
  </text>
  <text x="100" y="100" text-anchor="middle">centered text in the middle</text>
</svg>


Reproducible: Always

Steps to Reproduce:
1. save the above example to .svg file
2. open in mozilla 
3.

Actual Results:  
the text on the path is not displayed

Expected Results:  
the text should be displayed centered on the path
(Adobe SVG plugin renders it accoding to the specs)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: testcase

Comment 2

[Martin Hejral]

Attachment: another case - text on the path is again NOT rendered

Comment 3

[Martin Hejral]

text on a path (textPath) is not displayed - ALWAYS!!!

browser:

Deer Park 1.0+
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050531
Firefox/1.0+

test systems:
- standard Linux Fedora Core 2
- Windows 2000

Comment 4

[tor]

Attachment: work in progress

Snapshot of work.  Cairo only at this point.  Bounding box and hit detection
among the items not currently implemented.

Comment 5

[tor]

Attachment: snapshot

cairo work: stroke/clip cases, hit testing.

gdiplus work: path flattening, stubbed out metrics to compile.

Comment 6

[tor]

Attachment: getBBox work, some polish

Comment 7

[tor]

Setting blocking-avairy1.1 - it's a useful and unique feature of SVG, and as we
have it patch form we should try to get in for 1.1.  While this is a new
feature, it's a new feature of a new feature (SVG), so the added risk is small.

Comment 8

[tor]

Small change to nsSVGPathFrame::GetFlattenedPath per afri's comments:

NS_IMETHODIMP
nsSVGPathFrame::GetFlattenedPath(nsSVGPathData **data,
                                 nsIFrame *parent)
{
  nsIFrame *oldParent = mParent;
  nsCOMPtr<nsISVGRendererRegion> dirty_region;

  if (parent)
    mParent = parent;
  else
    SetMatrixPropagation(PR_FALSE);

  GetGeometry()->Update(nsISVGGeometrySource::UPDATEMASK_CANVAS_TM,
                        getter_AddRefs(dirty_region));

  GetGeometry()->Flatten(data);

  if (parent)
    mParent = oldParent;
  else
    SetMatrixPropagation(PR_TRUE);

  GetGeometry()->Update(nsISVGGeometrySource::UPDATEMASK_CANVAS_TM,
                        getter_AddRefs(dirty_region));

  return NS_OK;
}

Comment 9

[Alex Fritze]

Comment on attachment 186161 [details] [diff] [review]
getBBox work, some polish

>Index: dom/public/idl/svg/nsIDOMSVGTextPathElement.idl
>===================================================================
>[...]
>+[scriptable, uuid(5c29a76c-3489-48fe-b9ea-ea0f5b196dff)]
>+interface nsIDOMSVGTextPathElement
>+  : nsIDOMSVGTextContentElement
>+/*
>+        The SVG DOM makes use of multiple interface inheritance.
>+        Since XPCOM only supports single interface inheritance,
>+        the best thing that we can do is to promise that whenever
>+        an object implements _this_ interface it will also
>+        implement the following interfaces. (We then have to QI to
>+        hop between them.)
>+        
>+    nsIDOMSVGURIReference,
>+*/
>+{
>+  readonly attribute nsIDOMSVGAnimatedLength startOffset;
>+};

What about 
 readonly attribute SVGAnimatedEnumeration method; &
 readonly attribute SVGAnimatedEnumeration spacing;
?

>Index: layout/svg/base/src/nsSVGGlyphFrame.cpp
>===================================================================
>[...]
>+static void
>+FindPoint(nsSVGPathData *data,
>+          float aX, float aY, float aAdvance,
>+          nsSVGCharacterPosition *aCP)
>+{
>[...]
>+/* readonly attribute nsSVGCharacterPostion characterPosition; */
>+NS_IMETHODIMP
>+nsSVGGlyphFrame::GetCharacterPosition(nsSVGCharacterPosition **aCharacterPosition)
>+{

How about maintaining segment state between invocations of FindPoint,
so that you don't have to recount from the first segment every time?
I'm thinking complex paths with lots of segments here, e.g. text
around a circle.

Also, I'm not quite sure about the check for textpathframe parents. In
GetCharacterPosition() you walk up the tree (which presumably you need
to do to support things like <textPath><tspan>...</tspan></textPath>),
but in IsAbsolutelyPositioned() you just check mParent. Is that
intended? Maybe you could tag glyph frames that are textPath children
at frame construction time instead of walking up the tree every time?

>Index: layout/svg/base/src/nsSVGPathFrame.cpp
>===================================================================
>+NS_IMETHODIMP
>+nsSVGPathFrame::GetFlattenedPath(nsSVGPathData **data,
>+                                 nsIFrame *parent)
>+{
>+  nsIFrame *oldParent = mParent;
>+  nsCOMPtr<nsISVGRendererRegion> dirty_region;
>+
>+  if (parent) {
>+    mParent = parent;
>+    GetGeometry()->Update(nsISVGGeometrySource::UPDATEMASK_CANVAS_TM,
>+                          getter_AddRefs(dirty_region));
>+  }
>+
>+  GetGeometry()->Flatten(data);
>+
>+  if (parent) {
>+    mParent = oldParent;
>+    GetGeometry()->Update(nsISVGGeometrySource::UPDATEMASK_CANVAS_TM,
>+                          getter_AddRefs(dirty_region));
>+  }
>+
>+  return NS_OK;
>+}

This is sooo nasty for GDI+ and Libart, but what the heck. Cairo's the future
:-)

>Index: layout/svg/base/src/nsSVGTextFrame.cpp
>===================================================================
>[...]
> static void
>+GetSingleValue(nsISVGGlyphFragmentLeaf *fragment,
>+               nsIDOMSVGLengthList *list, float *val)
>[...]
>+    /* check for % sizing of textpath */
>[...]
>+      if (textPath) {
>+        nsSVGPathData *data;
>+        textPath->GetFlattenedPath(&data, nsnull);
>+
>+        if (!data)
>+          return;
>+
>+        float percent;
>+        length->GetValueInSpecifiedUnits(&percent);
>+
>+        *val = data->Length()*percent/100.0f;
>+        delete data;

Hmm, ok we don't do multiple x/y's yet, but I'm beginning to wonder
whether it would be worthwhile caching the nsSVGPathData on textPaths
rather than generating it everytime. I don't know how common
individually placed percentage-based glyphs are, but I'm thinking
SVG export filters from drawing programs here. E.g. at some point
ooffice drawing app's PS export filter placed each glyph
individually...  What do you reckon?

>Index: layout/svg/renderer/public/nsISVGGlyphMetricsSource.idl
>===================================================================
>[...]
>   /**
>+   * @name Character positioning information
>+   * @{
>+   */
>+  [noscript] void GetCharacterPosition(out nsSVGCharacterPosition aCP);
    ^^^^^^^^^^
Or you could just make the whole interface unscriptable. I think some of them
already are.


>Index: layout/svg/renderer/public/nsISVGRendererGlyphGeometry.idl
>===================================================================
>[...]
>+
>+  /**
>+   * Transformed ounding box (does not include stroke width)
>+   */

*b*ounding box

>Index: layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGlyphGeometry.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGlyphGeometry.cpp,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 nsSVGGDIPlusGlyphGeometry.cpp
>--- layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGlyphGeometry.cpp	25 Apr 2005 23:51:32 -0000	1.10
>+++ layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGlyphGeometry.cpp	13 Jun 2005 23:08:36 -0000
>@@ -54,16 +54,19 @@ using namespace Gdiplus;
> #include "nsISVGRendererRegion.h"
> #include "nsISVGGlyphGeometrySource.h"
> #include "nsPromiseFlatString.h"
> #include "nsSVGGDIPlusGlyphMetrics.h"
> #include "nsISVGGDIPlusGlyphMetrics.h"
> #include "nsPresContext.h"
> #include "nsMemory.h"
> #include "nsSVGGDIPlusGradient.h"
>+#include "nsSVGTypeCIDs.h"
>+#include "nsIComponentManager.h"
>+#include "nsIDOMSVGRect.h"
> 
> /**
>  * \addtogroup gdiplus_renderer GDI+ Rendering Engine
>  * @{
>  */
> //////////////////////////////////////////////////////////////////////
> /**
>  * Helper class used by nsSVGGDIPlusGlyphGeometry
>@@ -831,8 +834,67 @@ nsSVGGDIPlusGlyphGeometry::UpdateRegions
>   nsCOMPtr<nsPresContext> presContext;
>   mSource->GetPresContext(getter_AddRefs(presContext));
> 
>   NS_NewSVGGDIPlusClonedRegion(getter_AddRefs(mCoveredRegion),
>                                mHitTestRegion,
>                                presContext);
> }
> 
>+NS_IMETHODIMP
>+nsSVGGDIPlusGlyphGeometry::GetBoundingBox(nsIDOMSVGRect * *aBoundingBox)
>+{
>[...]
>+  //Region region(*(metrics->GetBoundingRect()));
>+  const RectF *bounds = metrics->GetBoundingRect();
>[...]
>+  Matrix m;
>+  GetGlobalTransform(&m);
>+  m.TransformPoints(pts, 4);
>+  REAL xmin, ymin, xmax, ymax;

This will give you an inflated bounding box.
matrix->GetBoundingRect() measures in transformed space and then
converts to untransformed space. Maybe you could add a parameter to
nsSVGGDIPlusGlyphMetrics::Get(Sub)BoundingRect() that, when set, skips
restoring of the GraphicsState before obtaining the bounds (~ line
424).

Comment 10

[tor]

(In reply to comment #9)
> (From update of attachment 186161 [details] [diff] [review] [edit])
> >Index: dom/public/idl/svg/nsIDOMSVGTextPathElement.idl
> >===================================================================
> >[...]
> >+[scriptable, uuid(5c29a76c-3489-48fe-b9ea-ea0f5b196dff)]
> >+interface nsIDOMSVGTextPathElement
> >+  : nsIDOMSVGTextContentElement
> >+{
> >+  readonly attribute nsIDOMSVGAnimatedLength startOffset;
> >+};
> 
> What about 
>  readonly attribute SVGAnimatedEnumeration method; &
>  readonly attribute SVGAnimatedEnumeration spacing;
> ?

Since we only do align/exact, I thought adding the attributes would be giving
false hope to people.  I can add these and ignore them if you like.

> >Index: layout/svg/base/src/nsSVGGlyphFrame.cpp
> >===================================================================
> >[...]
> >+static void
> >+FindPoint(nsSVGPathData *data,
> >+          float aX, float aY, float aAdvance,
> >+          nsSVGCharacterPosition *aCP)
> >+{
> >[...]
> >+/* readonly attribute nsSVGCharacterPostion characterPosition; */
> >+NS_IMETHODIMP
> >+nsSVGGlyphFrame::GetCharacterPosition(nsSVGCharacterPosition
**aCharacterPosition)
> >+{
> 
> How about maintaining segment state between invocations of FindPoint,
> so that you don't have to recount from the first segment every time?
> I'm thinking complex paths with lots of segments here, e.g. text
> around a circle.
> 
> Also, I'm not quite sure about the check for textpathframe parents. In
> GetCharacterPosition() you walk up the tree (which presumably you need
> to do to support things like <textPath><tspan>...</tspan></textPath>),
> but in IsAbsolutelyPositioned() you just check mParent. Is that
> intended? Maybe you could tag glyph frames that are textPath children
> at frame construction time instead of walking up the tree every time?

As you point out here and later on in your comments, there are opportunities to
speed thing sup by caching various bits of data.  My thought was to go for a
simple implementation and do the other bits when/if this code starts being a
bottleneck according to profiles.

> >Index: layout/svg/base/src/nsSVGPathFrame.cpp
> >===================================================================
> >+NS_IMETHODIMP
> >+nsSVGPathFrame::GetFlattenedPath(nsSVGPathData **data,
> >+                                 nsIFrame *parent)
> >+{
> >+  nsIFrame *oldParent = mParent;
> >+  nsCOMPtr<nsISVGRendererRegion> dirty_region;
> >+
> >+  if (parent) {
> >+    mParent = parent;
> >+    GetGeometry()->Update(nsISVGGeometrySource::UPDATEMASK_CANVAS_TM,
> >+                          getter_AddRefs(dirty_region));
> >+  }
> >+
> >+  GetGeometry()->Flatten(data);
> >+
> >+  if (parent) {
> >+    mParent = oldParent;
> >+    GetGeometry()->Update(nsISVGGeometrySource::UPDATEMASK_CANVAS_TM,
> >+                          getter_AddRefs(dirty_region));
> >+  }
> >+
> >+  return NS_OK;
> >+}
> 
> This is sooo nasty for GDI+ and Libart, but what the heck. Cairo's the future
> :-)

Theoretically, yes

Comment 11

[Alex Fritze]

(In reply to comment #10)
> > What about 
> >  readonly attribute SVGAnimatedEnumeration method; &
> >  readonly attribute SVGAnimatedEnumeration spacing;
> > ?
> 
> Since we only do align/exact, I thought adding the attributes would be giving
> false hope to people.  I can add these and ignore them if you like.

I think they should go in, even if they are just implemented by stubs. That's
what we've done for all other SVG interfaces so far, and, who knows, maybe
someone will step forward and implement them. Yeah, right ;-)

> > Also, I'm not quite sure about the check for textpathframe parents. In
> > GetCharacterPosition() you walk up the tree (which presumably you need
> > to do to support things like <textPath><tspan>...</tspan></textPath>),
> > but in IsAbsolutelyPositioned() you just check mParent. Is that
> > intended? Maybe you could tag glyph frames that are textPath children
> > at frame construction time instead of walking up the tree every time?
> 
> As you point out here and later on in your comments, there are opportunities to
> speed thing sup by caching various bits of data.  My thought was to go for a
> simple implementation and do the other bits when/if this code starts being a
> bottleneck according to profiles.

Fair enough, but in that case, don't you need to walk the parent chain in
IsAbsolutelyPositioned() as well?

Comment 12

[tor]

Attachment: work in progress - address afri's comments

Comment 13

[tor]

Attachment: implement for GDI+

Comment 14

[Chris Beard]

not holding on this, however, if it comes up, pending reviews including security
and its safe and timely we'll take it for 1.8b4

Comment 15

[tor]

Attachment: update to trunk

Comment 16

[Alex Fritze]

Comment on attachment 192523 [details] [diff] [review]
update to trunk

r=afri

Looks good. Sorry for the delay in reviewing.

In your checkin comment could you please state that the checkin also removes
the gdi+ text highlighting code?
This is just to make it easier to locate the highlighting code in future by
looking at the revision history.

Comment 17

[tor]

Attachment: update for post-branch trunk

Update patch for the trunk - fixes some conflicts with changes that happened
post 1.8-branch.  This won't work with the cairo that's on the trunk, as
something broke in the cairo font code between 0.5.0 (branch version) and
1.0.0.	It does work with keithp's cairo scaled glyph patch.

Comment 18

[tor]

Attachment: update for post-branch branch

Comment 19

[tor]

Landed on the trunk.  Turns out cairo-0.9.0 is ok once I did the minor update to
not use a null surface (patch we used to apply to cairo).

Leaving open for branch decision.

Comment 20

[tor]

Attachment: stubs for libart

Stubs for libart, since a number of ports tinderboxes still build this
depreciated backend.  Checked into trunk.

Comment 21

[simsalabimladen]

The example on http://www.zvon.org/xxl/svgReference/Output/exd0e20220.html
doesn't work as shown on the example image. There is a space after the comma,
and the text is wider than the SVG. 

Comment 22

[tor]

Not sure exactly what you're refering to, as that example looks ok on both linux
and win32 here.  On my win32 setup it appears we select a slightly larger font
than whatever produced the reference image, so the last two characters are off
the end of the path and are omitted per spec.

Comment 23

[Jeff Schiller]

http://www.zvon.org/xxl/svgReference/Output/exd0e20220.html works for me too 
in 20050826 build.  Looks identical to IE+ASV from what I can tell.

All samples at http://www.svgbasics.com/text2.html work for me too.

Comment 24

[Jeff Schiller]

Doh!  Ignore my statement about 
http://www.zvon.org/xxl/svgReference/Output/exd0e20220.html, I was looking at 
the raster! :)

Yes, there is an error, it seems that font may be larger, but the last word 
shows here as "agi".  If it was simply chopping off the last two characters 
then it should be "aga".  Something funny going on there.

Comment 25

[Jeff Schiller]

probably 'a' is too wide that we skip, but then 'i' looks like it would fit so 
it is printed?

Comment 26

[tor]

Attachment: fix logic when falling off the end of a text path

Comment 27

[Holger Will]

Attachment: testcase

this file shows two bugs, 
1) somtimes letters get cliped,
2) text when animated, leaves a visible trail.
besides that, it works great, tor++ !

note: you will need a fast computer for this testcase

Comment 28

[tor]

Attachment: fix missed use of a null surface

Comment 29

[Jonathan Watt [:jwatt]]

Comment on attachment 193956 [details] [diff] [review]
fix logic when falling off the end of a text path

r=jwatt

Comment 30

[tor]

Logic fix checked in on trunk.

Comment 31

[Holger Will]

Attachment: testcase - dynamicly change textpath data

another testcase showing the problem with dynamic updates.

Comment 32

[tor]

Attachment: observe path so invalidations occur

Thanks for those testcases, Holger.

Comment 33

[Alex Fritze]

Comment on attachment 194183 [details] [diff] [review]
observe path so invalidations occur

r=afri

Comment 34

[tor]

Comment on attachment 194183 [details] [diff] [review]
observe path so invalidations occur

Observer patch checked in on trunk.

Comment 35

[jpl24]

#include "nsIDOMSVGANimatedPathData.h"

The N should be lowercase, this is currently making prometheus burn.

Comment 36

[Bruce Rindahl]

Attachment: SVG code to center text along a path - Text does not render in Mozilla

Example of textPath centered on a line.  Renders fine with IE6+ASV but does not
render the text in Mozilla.
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050829

Comment 37

[Holger Will]

i believe the last testcase is invalid, i couldn't find anything in the spec
alowing textPath inside tspan elements.
i've just send an email to the svg-developers list, to clearify this issue.

Comment 38

[Bruce Rindahl]

(In reply to comment #37)
> i believe the last testcase is invalid, i couldn't find anything in the spec
> alowing textPath inside tspan elements.
> i've just send an email to the svg-developers list, to clearify this issue.

Confirmed this is the the cause of the text not rendering - awaiting 
clarification.

Comment 39

[Jonathan Watt [:jwatt]]

Bruce, it's perfectly valid to put the <tspan> inside the <textPath> instead,
and that should do what you want in both ASV and Firefox. For the record ASV
will allow dy on the <textPath> too. I'm  not sure if that or what you have in
your testcase is valid though off the top of my head. I suspect it isn't (or at
least shouldn't be).

Comment 40

[Holger Will]

Jon Ferraiolo just confirmed that this is indeed a bug in ASV.
here is his response:
http://groups.yahoo.com/group/svg-developers/message/51477

Comment 41

[Bruce Rindahl]

Thanks - I will adjust my code to reflect to correct coding.  Sorry about the 
posting.

Comment 42

[Holger Will]

without your testcase i wouldnt be able to track down the problem ;-) so thanks
to you !

Comment 43

[tor]

Comment on attachment 193962 [details] [diff] [review]
fix missed use of a null surface

Null surface patch checked in.

Comment 44

[tor]

Closing bug - please file any more problems found as seperate bugs.

Comment 45

[Holger Will]

i've uploaded a set of testcases for textPath,
http://www.treebuilder.de/firefox/textPath/
i will add more cases later...

Comment 46

[Chris Beard]

Comment on attachment 192523 [details] [diff] [review]
update to trunk

per discussion, not taking <svg:textPath> for 1.8