Text with manual kerning displays "unkerned" in browsers

[worms_x]

I'm using Inkscape 0.46+devel r20840, built Mar 6 2009 on Windows XP

After manual kerning of a text object, if you render the file in browsers (I tried Opera, Seamonkey and Firefox recent versions), the text appears as not kerned.

See the attachment, where the text on top has no manual kerning, and the bottom text has, in Inkscape.
See also the screenshot I'll attach of the rendering.

[Nuno-ponte (nuno-ponte)]

Created an attachment (id=218161)
Test case demonstration of the bug

Shows the problem and the correct behaviour.

[Bugzilla-tecnocode (bugzilla-tecnocode)]

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

[Malex-cs (malex-cs)]

Created an attachment (id=236237)
Initial patch attempt

[Malex-cs (malex-cs)]

Created an attachment (id=236424)
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.

[Malex-cs (malex-cs)]

Created an attachment (id=236426)
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.

[Malex-cs (malex-cs)]

Created an attachment (id=236436)
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.

[longsonr (longsonr)]

Check out bug 319345 . I think it is related.

Are you looking for comments on your initial patch attempt?

[Malex-cs (malex-cs)]

(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!

[Malex-cs (malex-cs)]

Created an attachment (id=236867)
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.

[Malex-cs (malex-cs)]

Created an attachment (id=236868)
patch - first revision

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

[longsonr (longsonr)]

(From update of attachment 236868)
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 (IsA...

[longsonr (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.

[Malex-cs (malex-cs)]

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?

[longsonr (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.

[longsonr (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?

[Malex-cs (malex-cs)]

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?

[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.

[longsonr (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.

[Mike-svgmaker (mike-svgmaker)]

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

SVG text and tspan elements in FF 2.0 support only an initial XY position.
The enhancement adds support for x,y,dx,dy lists on text and tspan elements, so that each character may be individually positioned.

Reproducible: Always

Steps to Reproduce:
1. View the example http://www.svgmaker.com/gallery/charpos.svg with FF 2.0.

Actual Results:
The first character is positioned at the initial XY. Subsequent characters are positioned using the default character advance.

Expected Results:
The first character should be positioned at the initial XY. Subsequent characters should be positioned using the XY values specified in the X and Y attributes of the tpans.

[Mike-svgmaker (mike-svgmaker)]

Created an attachment (id=272746)
Enhancement that implements x.y.dx.dy lists for SVG tspan and text elements

[Mike-svgmaker (mike-svgmaker)]

patch was implemented by Ken Stacey (<email address hidden>)

[longsonr (longsonr)]

This is a duplicate of bug 311986.

If you are looking for reviews you need to select a reviewer.

[longsonr (longsonr)]

(From update of attachment 272746)
One general comment.

I think it would be better all round if you tried to adapt the existing character placement mechanism used for textPath elements rather than having two separate implementations. i.e. use nsSVGCharacterPosition and make GetCharacterPosition return character positions if you have multiple y values. You may need to consider extending this so that you can have multiple characters with a single position for the first one so that you can write a string abcde with positions "x y" as character a at position x and then bcde in one go at position y.

>Index: layout/svg/base/src/nsISVGGlyphFragmentLeaf.h
>===================================================================
...
>- NS_IMETHOD_(void) SetGlyphPosition(float x, float y)=0;
>+ NS_IMETHOD_(void) SetGlyphPosition(float x, float y, PRUint16 baseline)=0;
>+ NS_IMETHOD_(void) GetNextGlyphPosition(float *x, float *y)=0;

If you change an interface you must update its ID i.e.
// {2C466AED-CF7B-4479-A807-98151215A645}
#define NS_ISVGGLYPHFRAGMENTLEAF_IID \
{ 0x2c466aed, 0xcf7b, 0x4479, { 0xa8, 0x7, 0x98, 0x15, 0x12, 0x15, 0xa6, 0x45 } }

> nsSVGGlyphFrame::nsSVGGlyphFrame(nsStyleContext* aContext)
> : nsSVGGlyphFrameBase(aContext),
> mWhitespaceHandling(COMPRESS_WHITESPACE)
> {
>+ mDoCharPlacement = PR_FALSE;
>+ mStrLength = 0;

Initialise member variables in the initialiser bit rather than the constructor body c.f. mWhitespaceHandling above. However see below.

>Index: layout/svg/base/src/nsSVGGlyphFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.h,v
>retrieving revision 1.25
>diff -u -8 -p -r1.25 nsSVGGlyphFrame.h
>--- layout/svg/base/src/nsSVGGlyphFrame.h 4 Jul 2007 03:39:03 -0000 1.25
>+++ layout/svg/base/src/nsSVGGlyphFrame.h 18 Jul 2007 04:32:36 -0000
>@@ -114,29 +114,30 @@ public:
> nsRefPtr<gfxFontGroup> mFontGroup;
> nsAutoPtr<gfxFontStyle> mFontStyle;
> gfxPoint mPosition;
> PRUint8 mWhitespaceHandling;
>+ nsAutoArrayPtr<nsSVGBaselinePosition> mBaselinePositions;
>+ PRBool mDoCharPlacement; // do per character placement

This bloats the class size. Reuse the textPath mechanism and calculate this when you need it.

Also don't use PRBool as a class member, use PRPackedBool.

And finally order any member variables in sizeof order to ensure minimal sizeof of the class as a whole.

>+ gfxPoint mNextGlyphPosition;
>+ float mBaselineOffset;
>+ PRUint32 mStrLength;

mStrLength just bloats the class size.

How many of these new members are absolutely mandatory? Most text will not have multiple x and y positions so we don't want to pay for that all the time. Neither do we want to store anything we can easily calculate.

[Mike-svgmaker (mike-svgmaker)]

Created an attachment (id=275429)
Enhancement 2 that implements x.y.dx.dy lists for SVG tspan and text elements

Reworked after Robert Longson's comments of 2007-07-18 02:21:56 PDT

[Tor-acm (tor-acm)]

A few comments:

  * you have some unchecked memory allocations that should be handled.

  * there a few unprotected printfs - wrap with DEBUG_* or remove.

  * since you're treating startOffset differently, nsSVGTextPathFrame::Init
    can be modified to remove the startOffset chunk and just request it from
    the content in GetStartOffset iff that attribute is set.

  * looks like GetStartOffset could use GetPathScale.

  * maybe modify the signature of SetGlyphPosition to take a gfxPoint&
    instead of moving data in and out of that structure?

[longsonr (longsonr)]

Could you update your tree to the trunk head before you submit your next patch please? The constructor for nsSVGGlyphFrame is now inline in nsSVGGlyphFrame.h for instance.

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #6)

> * you have some unchecked memory allocations that should be handled.

Not sure what you mean. Which allocations ?

> * there a few unprotected printfs - wrap with DEBUG_* or remove.

Thanks, I thought I got all of them.

> * since you're treating startOffset differently, nsSVGTextPathFrame::Init
> can be modified to remove the startOffset chunk and just request it from
> the content in GetStartOffset iff that attribute is set.

ok.

> * looks like GetStartOffset could use GetPathScale.

GetStartOffset does scale the value if needed.

> * maybe modify the signature of SetGlyphPosition to take a gfxPoint&
> instead of moving data in and out of that structure?

ok. But will have to move (float) x,y in and out of a gfxPoint in an outer loop of nsSVGTextFrame::UpdateGlyphPositioning(). Because nsIDOMSVGLength::GetValue gets a float.

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #7)

Thanks Robert.

I can generate a new patch. But I can't test now because the build fails in a macro in extensions/spellcheck/hunspell/src/atypes.hxx #$@%!!

Is this a common problem: update the source tree and then have the build fail ?
Is there a "safe" source I should update from ?

[longsonr (longsonr)]

That's minefield for you. You are suffering from bug 391147. I'll post a patch for that later today unless someone else beats me to it.

[longsonr (longsonr)]

(In reply to comment #8)

> Not sure what you mean. Which allocations ?

+ mCharPositions = new gfxPoint[strLength];

for instance

[longsonr (longsonr)]

Created an attachment (id=275600)
testcase

[longsonr (longsonr)]

Created an attachment (id=275601)
expected rendering for testcase

[longsonr (longsonr)]

*** Bug 311986 has been marked as a duplicate of this bug. ***

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #11)
> + mCharPositions = new gfxPoint[strLength];
>
> for instance

ok.

So, this one should be checked too ?

  nsSVGCharacterPosition *cp = new nsSVGCharacterPosition[strLength];

It is unchecked in latest source (nsSVGGlyphFrame::GetCharacterPosition)

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=275926)
startOffset in textPath lost when title element

I was creating some textPath test cases and ran across this strange problem. Using a current build, the startOffset should be 50% but ends up 0. If you remove the title element from the document, then it works ok.

This file works as expected with Firefox 2.0.0.6 with or without the title.

The extent to which I tracked it was
nsSVGTextPathFrame::GetFlattenedPath()
  nsSVGTextPathFrame::GetPathFrame()
    nsSVGUtils::GetReferencedFrame()
      *aRefFrame = aPresShell->GetPrimaryFrameFor(content); - fails

?

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=276088)
Enhancement that implements x.y.dx.dy lists for SVG tspan and text elements

Updated patch addressing recent comments

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=276089)
layout examples of x,y,dx,dy positioning + textPath

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=276090)
rendering for layout example

screen capture of layout example (id=276089) using patch (id=276088)

[longsonr (longsonr)]

(From update of attachment 276088)
General points:

If you are working with gfxFloat then literal assignment should not use f as gfxFloat is a double. If you are working with float then you should use an f suffix on literals.

Does inheritance of tspan values work? Have you tested that? Something like this contrived example...

<text x="1 2 3 4 5" y="4 5 6 7">h<tspan dx="5 6">e<tspan>l</tspan>lo</tspan></text>

The first l in hello has no dx in its tspan so it should pick up the parent tspan dx, presumably the 6.

Neither of the tspans have x or y values so they should pick up the text values where they exist.

>+float
>+nsSVGGlyphFrame::GetSubStringAdvance(PRUint32 charnum, PRUint32 fragmentChars)
>+{

>+ if (fragmentChars == 0) // do the whole run
>+ fragmentChars = text.Length();

Fix the caller to pass in the right value rather than having a magic number.

>+ for (PRUint32 i = charnum; i < dxcount; i++) {
>+ float dx = 0.0;

0.0f

>+void
>+nsSVGGlyphFrame::SetCharPosition(gfxTextRun *textRun,
>+ gfxPoint &ctp)
>+{

>+ gfxFloat pathScale = 1.0f; //only applied to dx,dy lists since x,y lists nsnull for nsSVGTextPathFrame

Put the comment on the previous line to avoid a long line.

>+ x -= advance/2.0f;

2.0

> NS_IMETHODIMP
> nsSVGGlyphFrame::GetStartPositionOfChar(PRUint32 charnum, nsIDOMSVGPoint **_retval)
> {

>+ if (angle != 0.0f) {

0.0

> NS_IMETHODIMP
> nsSVGGlyphFrame::GetEndPositionOfChar(PRUint32 charnum, nsIDOMSVGPoint **_retval)
>@@ -834,23 +1013,30 @@ nsSVGGlyphFrame::GetEndPositionOfChar(PR
> return NS_ERROR_OUT_OF_MEMORY;
>
>+ gfxFloat angle = cp[charnum].angle;
>+ if (angle != 0.0f) {

0.0

>Index: layout/svg/base/src/nsSVGGlyphFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.h,v
>retrieving revision 1.26
>diff -u -8 -p -r1.26 nsSVGGlyphFrame.h
>--- layout/svg/base/src/nsSVGGlyphFrame.h 3 Aug 2007 08:39:13 -0000 1.26
>+++ layout/svg/base/src/nsSVGGlyphFrame.h 10 Aug 2007 06:01:19 -0000

>- gfxPoint mPosition;
>+ gfxPoint mPosition; // character position of first glyph, includes baseline offset
>+ nsAutoArrayPtr<gfxPoint> mCharPositions; // character position of each glyph,
>+ // valid if not a simple textrun and/or textPath

Put the comments above the variables so we don't have long lines.

How about storing mCharPositions in a property. It would not be a member variable here then so the memory used by nsSVGGlyphFrame would be that bit smaller in most cases.

You would have

gfxPoint * charPositions = new gfxPoint[strLength];
SetProperty(nsGkAtoms::d, charPositions, positionDtorFunc);

positionDtorFunc would delete[] the charPositions data.

I picked d as there is no xy atom and d goes with paths.

Then if you needed individual character positions you could call

      charPositions = static_cast<gfxPoint *>
                            (GetProperty(nsGkAtoms::d));

There are various examples of Set/GetProperty: nsBlockFrame shows some usage as does nsSVGUtils (although that uses pointers to classes rather than poin...

[longsonr (longsonr)]

(In reply to comment #20)
> >- gfxPoint mPosition;
> >+ gfxPoint mPosition; // character position of first glyph, includes baseline offset
> >+ nsAutoArrayPtr<gfxPoint> mCharPositions; // character position of each glyph,
> >+ // valid if not a simple textrun and/or textPath
>

Even better than a property would be not storing the values at all and simply calculating them on the fly. All you really need is the starting point of the first glyph (mPosition) as that may depend on where the previous frame left you and so would be O(N2). Everything else you should be able to calculate using GetParent where necessary. If we find it is unacceptably slow we can always add caching of the positions in later.

This article explains where we were and what we're trying to avoid :-)
http://weblogs.mozillazine.org/roc/archives/2006/02/anatomy_of_bloat.html

I think it should be possible to get rid of everything except the mPosition member variable (although getting rid of the whitespacehandling member belongs in another bug).

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #20)

Thanks for the comments Robert.

> Does inheritance of tspan values work? Have you tested that? Something like
> this contrived example...
>
> <text x="1 2 3 4 5" y="4 5 6 7">h<tspan dx="5
> 6">e<tspan>l</tspan>lo</tspan></text>

Inherited positioning values still don't work. I wanted to support "simple" or direct lists first then address the implications of inheritance with another bug.

As I see it, the inheritance needs to be done in nsSVGTextFrame::UpdateGlyphPositioning. The looping of glyph framgments needs to change to traverse the text nodes (like in your comment https://bugzilla.mozilla.org/show_bug.cgi?id=333698#c18) and pass the appropriate (sub)sections of lists to nsSVGGlyphFrame::SetGlyphPosition().

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #21)

> Even better than a property would be not storing the values at all and simply
> calculating them on the fly. All you really need is the starting point of the
> first glyph (mPosition) as that may depend on where the previous frame left you
> and so would be O(N2). Everything else you should be able to calculate using
> GetParent where necessary. If we find it is unacceptably slow we can always add
> caching of the positions in later.

The code is already slow.

mPosition only stores the first glyph position. No problem if everything is a
simple text run and no inherited x,y,dx,dy lists.

To support your contrived example in Comment #20,

> <text x="1 2 3 4 5" y="4 5 6 7">h<tspan dx="5
> 6">e<tspan>l</tspan>lo</tspan></text>

even having mPosition for each fragment, you still have to determine if layout
calculations are required. That means going out to the current text element,
checking for inherited lists and then re-layout (calculate) each GlyphFragment
until you get back to the one you want the positioning for. In the example
above, determining the position of 'o' requires this action.

There would be a significant performance hit for painting the contents of a
text element using nsSVGGlyphFrame::PaintSVG(). For each successive glyph
fragment in a text element you would have to go back and check/recalculate all
the previous fragments. Resizing the browser window with a lot of text in the
document will become slower than it is. I can't see how SMIL animation of text
could ever be acceptable.

> This article explains where we were and what we're trying to avoid :-)
> http://weblogs.mozillazine.org/roc/archives/2006/02/anatomy_of_bloat.html

The article seems to point the finger mostly at XPCOM objects.

>
> I think it should be possible to get rid of everything except the mPosition
> member variable (although getting rid of the whitespacehandling member belongs
> in another bug).
>

Why even keep mPosition? or do UpdateGlyphPositioning() at all? or have
mPositioningDirty ? Just recalculate everything on demand.

Seriously, it would be helpful to understand how decisions are made regarding
bloat vs performance. Are there general guidelines I can use or is it hit or
miss at every patch?

[longsonr (longsonr)]

(In reply to comment #23)

> even having mPosition for each fragment, you still have to determine if layout
> calculations are required. That means going out to the current text element,
> checking for inherited lists and then re-layout (calculate) each GlyphFragment
> until you get back to the one you want the positioning for. In the example
> above, determining the position of 'o' requires this action.
>
> There would be a significant performance hit for painting the contents of a
> text element using nsSVGGlyphFrame::PaintSVG(). For each successive glyph
> fragment in a text element you would have to go back and check/recalculate all
> the previous fragments. Resizing the browser window with a lot of text in the
> document will become slower than it is. I can't see how SMIL animation of text
> could ever be acceptable.

It's performance vs memory. We can always put in caching afterwards.

> The article seems to point the finger mostly at XPCOM objects.

But also partly at frames storing things that content has.

>
> Why even keep mPosition? or do UpdateGlyphPositioning() at all? or have
> mPositioningDirty ? Just recalculate everything on demand.

Because you can only calculate mPosition if you know where the previous fragment ended and as frames are connected via a singly linked list there is no GetPreviousFrame function. All you could do is call GetParent then iterate through its children which would be order N^2.

>
> Seriously, it would be helpful to understand how decisions are made regarding
> bloat vs performance. Are there general guidelines I can use or is it hit or
> miss at every patch?
>

What I've picked over the patches I've submitted and review comments I've received is this...

Go for lack of bloat first and then add performance later if necessary in a separate patch. Order N performance is acceptable, order N^2 generally not.

It's easier to have a discussion on speeding up performance of a feature once that feature exists rather than as that feature is being developed.

In the FX3 timeframe we've removed most of the caching that happened in glyph frames. We used to cache all the characters for instance after we'd whitespace compressed them.

One additional argument is that when Tor implemented textPath, he didn't cache the glyph positions and he's in charge ;-)

You've chosen something that is quite complicated for a first patch. I'm glad you are persevering.

Best regards

Robert

[longsonr (longsonr)]

See also bug 282579 comment 9 which suggested caching positions for textPath and Tor's reply bug 282579 comment 10

[Tor-acm (tor-acm)]

My general feeling is to go for the simplest implementation first, and revisit if needed for performance or footprint reasons. At least to my interpretation, "simplest" should be within reason - avoiding gratuitous performance bottlenecks or data bloat.

A pointer for storing an array which is only allocated when needed (multiple x/y/dx/dy) seems under this threshold and could go in if it simplifies the code, which from Ken's description it seems to do. Individual character positions would seem to be the exceptional case for most SVG content, so it storing cached values won't affect most users.

[longsonr (longsonr)]

Ken, can you fix the other issues in comment 20 i.e. those other than inheritance support and caching removal? Then set the review flag on the attachment for me as a reviewer.

Please raise another mozilla bug for inheritance of x, y, dx, dy so we can track that we still need to do that.

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=277664)
x.y.dx.dy lists for SVG tspan and text elements

(In reply to comment #27)

Robert,

Fixed all the other issues and removed mBaselineOffset from nsSVGGlyphFrame and calculate it on demand. This has a side effect of fixing a bug relating to dominant-baseline.

<text dominant-baseline="alphabetic">
 <tspan>alphabetic</tspan>
 <tspan dominant-baseline="hanging">hanging</tspan>
</text>

the old code ignores the nested (hanging) baseline.

[longsonr (longsonr)]

Ken, you need to select ? from the review dropdown and put my email address in as the requestee.

There is good information here http://wiki.mozilla.org/Bugzilla:Review

Once you get a review + you would repeat the process for super-review, for which I would suggest you chose Tor.

Best regards

Robert

[longsonr (longsonr)]

(From update of attachment 277664)
>+static void
>+GetListValue(nsIDOMSVGLengthList *list, PRUint32 ival, gfxFloat *val)

Make this return the value you want rather than taking a gfxFloat *val argument. It will make the calling code simpler.

> static void
>-GetSingleValue(nsISVGGlyphFragmentLeaf *fragment,
>- nsIDOMSVGLengthList *list, float *val)
>+GetSingleValue(nsIDOMSVGLengthList *list, gfxFloat *val)

Again I think it would be simpler just to return the result you want rather than passing a pointer argument, even though that's what the current code does ;-)

> already_AddRefed<gfxFlattenedPath>
> nsSVGTextPathFrame::GetFlattenedPath()
> {
>- nsIFrame *path = GetPathFrame();
>- if (!path)
>- return nsnull;
>+ return GetFlattenedPath(nsnull);

I think this should be implemented as:

  nsIFrame *path = path = GetPathFrame();
  if (!path)
    return nsnull;
  return GetFlattenedPath(path);

>+}
>+
>+already_AddRefed<gfxFlattenedPath>
>+nsSVGTextPathFrame::GetFlattenedPath(nsIFrame *path)
>+{
>+ if (!path) {
>+ path = GetPathFrame();
>+ if (!path)
>+ return nsnull;
>+ }

This change is then not necessary and can be removed.

>+gfxFloat
>+nsSVGTextPathFrame::GetStartOffset() {

Nit: Could you put the { on a new line please?

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #29)

Thanks, Robert. Wasn't sure what to do there.

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #30)
> (From update of attachment 277664 [details])
> >+static void
> >+GetListValue(nsIDOMSVGLengthList *list, PRUint32 ival, gfxFloat *val)
>
> Make this return the value you want rather than taking a gfxFloat *val
> argument. It will make the calling code simpler.
>

Should be ok since the calling code checks the list length (as a shortcut) before calling the function.

> > static void
> >-GetSingleValue(nsISVGGlyphFragmentLeaf *fragment,
> >- nsIDOMSVGLengthList *list, float *val)
> >+GetSingleValue(nsIDOMSVGLengthList *list, gfxFloat *val)
>
> Again I think it would be simpler just to return the result you want rather
> than passing a pointer argument, even though that's what the current code does
> ;-)

In this case the calling code takes advantage of the fact that *val is not changed if there is no list or an empty list.

What value should the function return in these cases? How will the calling code know that the return value should be ignored? Either the calling code has to check before making the call (like above) or pass in a default value and return that.

>
> > already_AddRefed<gfxFlattenedPath>
> > nsSVGTextPathFrame::GetFlattenedPath()
> > {
> >- nsIFrame *path = GetPathFrame();
> >- if (!path)
> >- return nsnull;
> >+ return GetFlattenedPath(nsnull);
>
> I think this should be implemented as:
>
> nsIFrame *path = path = GetPathFrame();
> if (!path)
> return nsnull;
> return GetFlattenedPath(path);
>
> >+}
> >+
> >+already_AddRefed<gfxFlattenedPath>
> >+nsSVGTextPathFrame::GetFlattenedPath(nsIFrame *path)
> >+{
> >+ if (!path) {
> >+ path = GetPathFrame();
> >+ if (!path)
> >+ return nsnull;
> >+ }
>
> This change is then not necessary and can be removed.

Shouldn't there at least be

  if (!path)
    return nsnull;

In case the private method is called with path == nsnull ?

At present, GetFlattenedPath is called in 3 places

1. nsSVGGlyphFrame::GetCharacterPosition(...)
     textPath->GetFlattenedPath(); // public method

2. nsSVGTextPathFrame::GetStartOffset()
     GetFlattenedPath(nsnull); //private

3. nsSVGTextPathFrame::GetPathScale()
     GetFlattenedPath(pathFrame); //private

If the "(!path) etc" is removed, then case#2 needs to change to call the public method or do GetPathFrame() and check before calling.

Case#3 would be ok since that code would not be called if pathFrame == nsnull anyway.

>
> >+gfxFloat
> >+nsSVGTextPathFrame::GetStartOffset() {
>
> Nit: Could you put the { on a new line please?
>

No problem.

[longsonr (longsonr)]

(In reply to comment #32)
> (In reply to comment #30)
> > (From update of attachment 277664 [details] [details])
> > >+static void
> > >+GetListValue(nsIDOMSVGLengthList *list, PRUint32 ival, gfxFloat *val)
> >
> > Make this return the value you want rather than taking a gfxFloat *val
> > argument. It will make the calling code simpler.
> >
>
> Should be ok since the calling code checks the list length (as a shortcut)
> before calling the function.

I wouldn't bother with the shortcut check. This code is only called when things change; either once on load or if you do something in DOM so I think simplicity is better.

nsSVGGlyphFrame::SetCharPosition becomes much simpler.

+ gfxFloat dx = 0.0, dy = 0.0;
+ if (dxcount)
+ GetListValue(dxlist, 0, &dx);
+ x += dx;

would become

+ x += GetListValue(dxlist, 0);

for instance

>
> > > static void
> > >-GetSingleValue(nsISVGGlyphFragmentLeaf *fragment,
> > >- nsIDOMSVGLengthList *list, float *val)
> > >+GetSingleValue(nsIDOMSVGLengthList *list, gfxFloat *val)
> >
> > Again I think it would be simpler just to return the result you want rather
> > than passing a pointer argument, even though that's what the current code does
> > ;-)
>
> In this case the calling code takes advantage of the fact that *val is not
> changed if there is no list or an empty list.

OK, leave it as you have it then.

> > >+already_AddRefed<gfxFlattenedPath>
> > >+nsSVGTextPathFrame::GetFlattenedPath(nsIFrame *path)
> > >+{
> > >+ if (!path) {
> > >+ path = GetPathFrame();
> > >+ if (!path)
> > >+ return nsnull;
> > >+ }
> >
> > This change is then not necessary and can be removed.
>
> Shouldn't there at least be
>
> if (!path)
> return nsnull;

How about

    NS_ASSERTION(path, NS_ERROR_INVALID_POINTER);

instead? Since this is a private function, it's reasonable for it to assume its callers know not to call it with nsnull.

>
> In case the private method is called with path == nsnull ?
>
>
> At present, GetFlattenedPath is called in 3 places
>
> 1. nsSVGGlyphFrame::GetCharacterPosition(...)
> textPath->GetFlattenedPath(); // public method
>
> 2. nsSVGTextPathFrame::GetStartOffset()
> GetFlattenedPath(nsnull); //private
>
> 3. nsSVGTextPathFrame::GetPathScale()
> GetFlattenedPath(pathFrame); //private
>
> If the "(!path) etc" is removed, then case#2 needs to change to call the public
> method or do GetPathFrame() and check before calling.
>

Calling the public method seems best.

[longsonr (longsonr)]

(From update of attachment 277664)
clear invalid flag

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #33)
> > Should be ok since the calling code checks the list length (as a shortcut)
> > before calling the function.
>
> I wouldn't bother with the shortcut check. This code is only called when things
> change; either once on load or if you do something in DOM so I think simplicity
> is better.

Without the shortcut check there is no way to know if the return value is valid or not (similar to discussion for GetSingleValue() in nsSVGTextFrame.cpp)

I take your point about making it simpler. I made an SVGLengthListHelper class to hide the list length checking etc.

> > > >+already_AddRefed<gfxFlattenedPath>
> > > >+nsSVGTextPathFrame::GetFlattenedPath(nsIFrame *path)
> > > >+{
> > > >+ if (!path) {
> > > >+ path = GetPathFrame();
> > > >+ if (!path)
> > > >+ return nsnull;
> > > >+ }
> > >
> > > This change is then not necessary and can be removed.
> >
> > Shouldn't there at least be
> >
> > if (!path)
> > return nsnull;
>
> How about
>
> NS_ASSERTION(path, NS_ERROR_INVALID_POINTER);
>
> instead? Since this is a private function, it's reasonable for it to assume its
> callers know not to call it with nsnull.

Sounds good.

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=278721)
x.y.dx.dy lists for SVG tspan and text elements

[longsonr (longsonr)]

(From update of attachment 278721)
Apologies for the delay. Somehow I got it into my head that I'd already commented on this.

> nsSVGGlyphFrame::GetCharacterPosition(gfxContext *aContext,
> const nsString &aText,
>

>+ nsSVGTextPathFrame *textPath = FindTextPathParent();
>+ if (!textPath) { // normal x,y,dx,dy placement

Get rid of the ! in the if and reverse the order of the following parts to suit. The fewer !s the better.

>+class SVGLengthListHelper
>+{
>+ nsCOMPtr<nsIDOMSVGLengthList> mList;
>+ PRUint32 mCount;
>+public:
>+ SVGLengthListHelper(const already_AddRefed<nsIDOMSVGLengthList> list);

I don't think you can use already_AddRefed as a function argument. AFAIK that construct is only for return values. You want to make the argument a raw pointer and probably use already_AddRefed when you call it.

>+SVGLengthListHelper::SVGLengthListHelper(const already_AddRefed<nsIDOMSVGLengthList> list)
>+{
>+ mList = list;
>+ mCount = 0;

Use constructor style initialisation for member variables.
  : mList(list), mCount(0)

>+void
>+nsSVGGlyphFrame::SetCharPosition(gfxTextRun *textRun,
>+ gfxPoint &ctp)
>+{

>+ if (!textPath) {

Again, please get rid of the ! and swap the order of the clauses below so the logic still works.

>
>+gfxFloat
>+nsSVGTextPathFrame::GetStartOffset() {

Please put the { on a new line

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #37)
> >+class SVGLengthListHelper
> >+{
> >+ nsCOMPtr<nsIDOMSVGLengthList> mList;
> >+ PRUint32 mCount;
> >+public:
> >+ SVGLengthListHelper(const already_AddRefed<nsIDOMSVGLengthList> list);
>
> I don't think you can use already_AddRefed as a function argument. AFAIK that
> construct is only for return values. You want to make the argument a raw
> pointer and probably use already_AddRefed when you call it.

Yes, on further research (bug #172030), |already_AddRefed<T>| is used as a temporary _short_ lived return value.

Also (bug #178187), using the raw pointer result of already_AddRefed<T>::get() doesn't make clear the transfer of ownership.

I struggled with this for a while because I wanted the ownership to be inside the helper class. Rather than extract and pass the raw pointer it might be clearer to keep the ownership outside of the class.

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=280428)
x.y.dx.dy lists for SVG tspan and text elements

Fixes for Comment #37

[longsonr (longsonr)]

(From update of attachment 280428)
>+ if (((xlist.count() <= 1) && (ylist.count() <= 1) &&
>+ (dxlist.count() <= 1) && (dylist.count() <= 1))
>+ || (strLength == 1)) {

Nit: Usual convention is to have || at the end of the previous line.

r=longsonr

[Ken-svgmaker (ken-svgmaker)]

Robert,

Thanks for the +. I'm not sure what to do next. Do I need to submit another patch and request Tor for a super review ? Or do I request Tor on the existing patch ?

Thanks,
Ken

[longsonr (longsonr)]

up to you really. you could just ask tor for sr with this and if he wants any changes fix it as part of that or if not just go with this patch for check in.

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=282058)
x.y.dx.dy lists for SVG tspan and text elements

Fixed "||" nit

[Tor-acm (tor-acm)]

(From update of attachment 282058)
It seems that SetCharPosition is only called from SetGlyphPosition. If that is the case and you don't have future plans for other things to call it, it seems that the code should be in the latter.

SetCharPosition allocates the mCharPositions array for a textPath even if there aren't adjustments needed.

GetBaselineOffset - do you mean to only use the dominant-baseline from the outer <text> container, and not any nested text content elements?

GetStartOffset - you can get the start offset more directly, ie: static_cast<nsSVGTextPathElement*>(mContent)->mLengthAttributes[STARTOFFSET].GetBaseValue(). You will need to split the class definition of nsSVGTextPathElement into a seperate file for that.

GetPathScale likewise could be simplified if nsSVGTextPathFrame was also made a friend of nsSVGPathElement.

[longsonr (longsonr)]

(In reply to comment #44)
> GetStartOffset - you can get the start offset more directly, ie:
> static_cast<nsSVGTextPathElement*>(mContent)->mLengthAttributes[STARTOFFSET].GetBaseValue().
> You will need to split the class definition of nsSVGTextPathElement into a
> seperate file for that.

That should be GetAnimValue rather than GetBaseValue as startOffset is animatable.

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #44)
> It seems that SetCharPosition is only called from SetGlyphPosition. If that is
> the case and you don't have future plans for other things to call it, it seems
> that the code should be in the latter.

OK

> SetCharPosition allocates the mCharPositions array for a textPath even if there
> aren't adjustments needed.

OK, mCharPosions only allocated when adjustments are needed.

> GetBaselineOffset - do you mean to only use the dominant-baseline from the
> outer <text> container, and not any nested text content elements?

It is meant to work like the latter - use the nearest parent text content element's dominant-baseline.

The loop at the beginning searches back up towards the text parent only while
dominant-baseline="auto" (NS_STYLE_DOMINANT_BASELINE_AUTO). This is the initial value for dominant-baseline and works like "inherit" for nested text content elements.

> GetStartOffset - you can get the start offset more directly, ie:
> static_cast<nsSVGTextPathElement*>(mContent)->mLengthAttributes[STARTOFFSET].GetBaseValue().
> You will need to split the class definition of nsSVGTextPathElement into a
> seperate file for that.

OK

> GetPathScale likewise could be simplified if nsSVGTextPathFrame was also made a
> friend of nsSVGPathElement.
>

OK

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=284402)
x.y.dx.dy lists for SVG tspan and text elements

New header file in seperate attachment

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=284403)
New file - nsSVGTextPathElement.h - for patch 284402

[Ken-svgmaker (ken-svgmaker)]

A comment on attachment (id=284402)

Lately, I have been unable to update my source tree. I tracked it down to a line in client.mk. (included in patch (id=284402))

 CONFIG_STATUS_DEPS := \
  $(TOPSRCDIR)/configure \
  $(TOPSRCDIR)/.mozconfig.mk \
  $(wildcard $(TOPSRCDIR)/nsprpub/configure) \
  $(wildcard $(TOPSRCDIR)/directory/c-sdk/configure) \
  $(wildcard $(TOPSRCDIR)/config/milestone.txt) \
  $(wildcard $(TOPSRCDIR)/config/chrome-versions.sh) \
- $(wildcard $(addsuffix confvars.sh,$(wildcard */))) \
+ $(wildcard $(addsuffix confvars.sh,$(wildcard $(TOPSRCDIR)/*/))) \
  $(NULL)

Note that $(wildcard */) depends on the current directory.

This variable is ifdefed out for the first checkout.

When updating the source tree, this variable is expanded from $(TOPSRCDIR), then
the following line is executed in checkout:

 @cd $(ROOTDIR) && $(MAKE) -f mozilla/client.mk real_checkout

When making real_checkout it is evaluated from $(ROOTDIR). On my computer, make failed while trying to open a hidden system directory in the rootdir when evaluating $(wildcard */). The only confvars.h files I could find are in subdirs of $(TOPSRCDIR) so I changed the line to get it to not fail. No apparrent problems with build.

A bug ?

[longsonr (longsonr)]

(In reply to comment #48)
> Created an attachment (id=284403) [details]
> New file - nsSVGTextPathElement.h - for patch 284402
>

While you don't have write access to cvs you should use the cvsdo perl script to add new files to your local repository. You can then submit a single patch which will include the new file. See http://viper.haque.net/~timeless/redbean/

[longsonr (longsonr)]

(In reply to comment #49)

> - $(wildcard $(addsuffix confvars.sh,$(wildcard */))) \
> + $(wildcard $(addsuffix confvars.sh,$(wildcard $(TOPSRCDIR)/*/))) \

My client.mk looks like the - line and bonsai agrees. http://mxr.mozilla.org/mozilla/source/client.mk?mark=1020#1009

[Ken-svgmaker (ken-svgmaker)]

Created an attachment (id=284581)
x.y.dx.dy lists for SVG tspan and text elements

Includes new file nsSVGTextPathElement.h

[Ken-svgmaker (ken-svgmaker)]

(In reply to comment #52)

Meant to set superreview for tor but hit enter at wrong time.

[longsonr (longsonr)]

I've used part of this bug to fix bug 406312

[longsonr (longsonr)]

I've checked in the fix for bug 406312 so perhaps 25% of this patch is no longer required.

Also the changes to nsSVGTextContainerFrame.cpp are probably not desirable as the code will work without them.

Just to let you know what's going on, this is new functionality and I'm afraid it missed the cutoff time to land for firefox 3.0. An updated patch could still make firefox 3.1

[Ft2-wiki (ft2-wiki)]

Visiting to report/confirm - see Wikimedia bug report

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

[worms_x (wormsxulla)]

I'm using Inkscape 0.46+devel r20840, built Mar 6 2009 on Windows XP

After manual kerning of a text object, if you render the file in browsers (I tried Opera, Seamonkey and Firefox recent versions), the text appears as not kerned.

See the attachment, where the text on top has no manual kerning, and the bottom text has, in Inkscape.
See also the screenshot I'll attach of the rendering.

[worms_x (wormsxulla)]

Attaching the screenshot of the rendering (I didn't bother to convert to path)

[jazzynico (jazzynico)]

Confirmed on Ubuntu 8.10, build 20941, with Firefox 3.0.7 and Eye of Gnome.
But it may be due to a bad browser support (none is 100% SVG compliant). dx attribute, in the tspan element, is not supported by most svg readers and browsers.
We should test on beta (alpha?) releases of opera and firefox. Or another reliable SVG reader?

[worms_x (wormsxulla)]

I have tested in a recent "Minefield" aka Firefox development trunk build, and it renders both "Text" with the same length.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre

I'll ask in the mozilla SVG channel to test with an even more recent build.

[worms_x (wormsxulla)]

It appears to be a browser bug indeed:

Opera 10a1 shows the kerning correctly (I tested on 9.6x earlier, it was incorrect)
Safari 4b1 shows the kerning correctly

Minefield (Firefox trunk) does not, this is bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=388547

and people on Mozilla now know about it (more).

So it's not an Inkscape bug, after all :-)

[jazzynico (jazzynico)]

Thanks for your tests!
Good to see new browsers are improving (uhh, except one...)!

Closed invalid (not an Inkscape bug).

[Rafał Miłecki (zajec5)]

Robert Longson: could you find some time for rewriting your patch againts current trunk? Would be nice to see your patch in Gecko 1.9.2 together will all other SVG improvements.

[Rafał Miłecki (zajec5)]

I meant Ken Stacey of course, sorry.

[Roc-ocallahan (roc-ocallahan)]

Robert, just so you know, we're planning some changes to the SVG text code that would impact this quite heavily ... to be precise, rewriting it to use hidden nsBlockFrame/nsInlineFrame/nsTextFrames for layout and then extracting glyph data from them for rendering. The main goal is to handle bidi better.

[longsonr (longsonr)]

I'm hoping to reuse the existing textpath positioning code so hopefully I won't make that rewrite any worse. I'll be doing this one small step at a time too.

[longsonr (longsonr)]

Created an attachment (id=389307)
update to tip and add support for rotate

[Kurosawa Takeshi (takenspc)]

Created an attachment (id=389356)
Screenshot (with attachment 389307)

(In reply to comment #60)
> Created an attachment (id=389307) [details]
> update to tip and add support for rotate

This patch regresses the <tspan dy> in a <textPath> testcase :-(.
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-path-01-b.html

[longsonr (longsonr)]

Created an attachment (id=389392)
updated patch with reftest

Fixes reported issue.

[Roc-ocallahan (roc-ocallahan)]

+GetNumberOfNumberListItems(nsIDOMSVGNumberList *list)
+GetLengthListValue(nsIDOMSVGLengthList *list, PRUint32 index)
+GetNumberListValue(nsIDOMSVGNumberList *list, PRUint32 index)

Use a* for parameters

+ NS_IMETHOD_(void) SetGlyphPosition(gfxPoint &position, PRBool aForceGlobalTransform)=0;

Prefer gfxPoint* here for the in-out parameter.

+ // have we run off the end of the path?
+ if (pos.x + halfAdvance > length)
+ break;

Is it possible for us to go past the end of the path but then negative dx pulls us back into the path?

You're not handling 'rotate' for text on a path?

Can't GetDx/GetDy/GetRotate just return a non-addrefed pointer?

We really need some love for nsIDOMSVGLengthList/nsIDOMSVGNumberList so that we have optimized storage and a COM-less internal interface, but I guess that can wait.

Why are you checking for the presence of the attributes in GetDx/GetDy/GetRotate? An optimization? Seems like these could be animated so that even if there's no attribute, there's still an animated value?

[longsonr (longsonr)]

Created an attachment (id=390669)
address review comments

(In reply to comment #63)
> +GetNumberOfNumberListItems(nsIDOMSVGNumberList *list)
> +GetLengthListValue(nsIDOMSVGLengthList *list, PRUint32 index)
> +GetNumberListValue(nsIDOMSVGNumberList *list, PRUint32 index)
>
> Use a* for parameters

Done.

>
> + NS_IMETHOD_(void) SetGlyphPosition(gfxPoint &position, PRBool
> aForceGlobalTransform)=0;
>
> Prefer gfxPoint* here for the in-out parameter.

Done.

>
> + // have we run off the end of the path?
> + if (pos.x + halfAdvance > length)
> + break;
>
> Is it possible for us to go past the end of the path but then negative dx pulls
> us back into the path?

Fixed.

>
> You're not handling 'rotate' for text on a path?

Fixed.

>
> Can't GetDx/GetDy/GetRotate just return a non-addrefed pointer?

I'm going to punt that to whatever fix addresses nsIDOMSVGLengthList/nsIDOMSVGNumberList if that's OK.

>
> We really need some love for nsIDOMSVGLengthList/nsIDOMSVGNumberList so that we
> have optimized storage and a COM-less internal interface, but I guess that can
> wait.

Indeed.

>
> Why are you checking for the presence of the attributes in
> GetDx/GetDy/GetRotate? An optimization? Seems like these could be animated so
> that even if there's no attribute, there's still an animated value?

Fixed.

[Roc-ocallahan (roc-ocallahan)]

(From update of attachment 390669)
(In reply to comment #64)
> (In reply to comment #63)
> > + // have we run off the end of the path?
> > + if (pos.x + halfAdvance > length)
> > + break;
> >
> > Is it possible for us to go past the end of the path but then negative dx
> > pulls us back into the path?
>
> Fixed.

Can you add a test for this? Generally I think it's good to create regression tests even for the bugs you find during development before checking in.

> > You're not handling 'rotate' for text on a path?
>
> Fixed.

You don't seem to have any tests for rotate? We need tests with and without a path.

> > Can't GetDx/GetDy/GetRotate just return a non-addrefed pointer?
>
> I'm going to punt that to whatever fix addresses
> nsIDOMSVGLengthList/nsIDOMSVGNumberList if that's OK.

OK

r+ assuming those tests are added.

[Kurosawa Takeshi (takenspc)]

*** Bug 463990 has been marked as a duplicate of this bug. ***

[Kurosawa Takeshi (takenspc)]

*** Bug 517421 has been marked as a duplicate of this bug. ***

[longsonr (longsonr)]

Created an attachment (id=405745)
update to tip and add rotate test

[longsonr (longsonr)]

landed then backed out due to mac test orange. The test looks the same as the reference to me so I don't know what's wrong at the moment.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255263471.1255264753.32026.gz&fulltext=1#err1

[longsonr (longsonr)]

*** Bug 521637 has been marked as a duplicate of this bug. ***

[Jonathan Watt (jwatt)]

These tests are from the SVG 1.2 Tiny test suite, but they should also work in a 1.1 Full implementation after this bug is fixed, I think:

http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-text-04-t.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-text-05-t.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-text-06-t.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-text-text-07-t.html

[Jonathan Watt (jwatt)]

*** Bug 537062 has been marked as a duplicate of this bug. ***