318597 - Implement various text, tspan and textPath DOM functions

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051201 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051201 Firefox/1.6a1

Currently GetSubStringLength, GetStartPositionOfChar, GetEndPositionOfChar, GetRotationOfChar are not implemented.
GetComputedTextLength is implemented by getting the bounding box which is incorrect for vertical characters and makes tspans need to be implemented as GraphicElements rather than Styleable elements which they should really be.
GetExtentOfChar is not implemented for textPath elements either.


Reproducible: Always

Steps to Reproduce:
1. try calling any of the above functions on text, tspan or textPath elements

Actual Results:  
Should not throw a not implemented error.

Comment 1

[Robert Longson [:longsonr]]

Attachment: work in progress

This is a work in progress. It encompases the changes in bug 314627 and I intend to submit a smaller patch here for review once that is checked in. It allows tspan elements to become stylable rather than graphic elements again as they no longer need to call GetBBox. This implements getExtentOfChar for textPath as identified in bug 314627 and also fixes bug 317709.

I would appreciate comments on this anyway.

a) Was I correct to change the IDs of the interfaces I added new members to.
b) Are the implementations of getComputedTextLength and getSubStringLength correct when applied to textPath elements. The answers don't agree with those of ASV3 but then I can't understand how ASV3 gets the answers it does for those functions.

Comment 2

[Robert Longson [:longsonr]]

Attachment: implement various text DOM functions for text, tspan and textPath elements

I guess items a) and b) from the comments for the previous patch still apply.

The addition of the IsEventName mothod to nsSVGTextPathElement.cpp fixes bug 317709.

Comment 3

[tor]

Comment on attachment 205067 [details] [diff] [review]
implement various text DOM functions for text, tspan and textPath elements

Thanks for working on this, sorry about the delay in reviewing.

nsSVG{TSpan,Text,TextPath}Element::GetRotationOfChar/GetCharNumAtPosition
don't set the return value when an error happens.

Could you move the fix for bug 317709 into that bug, to help with
the cvs history?

Style nit - in nsSVGUtils you have a few instances of "while(condition)",
while the prevaling mozilla style is to have a space after the "while".

>+nsresult
>+nsSVGUtils::GetCharNumAtPosition(nsISVGGlyphFragmentNode* node,
>+                                     nsIDOMSVGPoint *point,
>+                                     PRInt32 *_retval)
>+{
>+  PRInt32 index = -1;
>+  nsISVGGlyphFragmentLeaf *fragment = node->GetFirstGlyphFragment();
>+
>+  while(fragment) {
>+    if (fragment->GetNumberOfChars() > 0) {
...
>+      nsresult rv = metrics->GetCharNumAtPosition(position, &index);
>+      if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
>+    }
>+    fragment = fragment->GetNextGlyphFragment();
>+  }

Should this have an early exit so we don't keep looking at fragments,
or is to catch the case of self-overlapping fragment tree?

>+nsSVGCairoGlyphMetrics::GetComputedTextLength(float *_retval)
>+{
...
>+  *_retval = extent.x_advance + extent.y_advance;

Why add the y advance?

>+nsSVGCairoGlyphMetrics::GetSubStringLength(PRUint32 charnum, PRUint32 nchars, float *_retval)
>+{
...
>+  *_retval = extent.x_advance + extent.y_advance;

Same.

>+/** Implements float getRotationOfChar(in unsigned long charnum); */
>+NS_IMETHODIMP
>+nsSVGCairoGlyphMetrics::GetRotationOfChar(PRUint32 charnum, float *_retval)
>+{
>+  const double pi = 3.141592653;

Please use M_PI, with appropriate ifdef/def for msvc.

>+nsSVGCairoGlyphMetrics::GetCharNumAtPosition(nsIDOMSVGPoint *point, PRInt32 *_retval)
>+{
...
>+    cairo_text_path(mCT,
>+                    NS_ConvertUCS2toUTF8(Substring(text, i, 1)).get());
>+    cairo_set_matrix(mCT, &matrix);
>+
>+    if (cairo_in_fill(mCT, x, y))
>+      *_retval = i;

GetCharNumAtPosition is supposed to do testing on character boxes, like
the hit testing.

Comment 4

[Robert Longson [:longsonr]]

Attachment: address

Comment 5

[Robert Longson [:longsonr]]

Attachment: address review comments

> nsSVG{TSpan,Text,TextPath}Element::GetRotationOfChar/GetCharNumAtPosition
> don't set the return value when an error happens.

Fixed.

> 
> Could you move the fix for bug 317709 into that bug, to help with
> the cvs history?

Done.

> 
> Style nit - in nsSVGUtils you have a few instances of "while(condition)",
> while the prevaling mozilla style is to have a space after the "while".

Fixed.

> 
> >+nsresult
> >+nsSVGUtils::GetCharNumAtPosition(nsISVGGlyphFragmentNode* node,
> >+                                     nsIDOMSVGPoint *point,
> >+                                     PRInt32 *_retval)
> >+{
> >+  PRInt32 index = -1;
> >+  nsISVGGlyphFragmentLeaf *fragment = node->GetFirstGlyphFragment();
> >+
> >+  while(fragment) {
> >+    if (fragment->GetNumberOfChars() > 0) {
> ...
> >+      nsresult rv = metrics->GetCharNumAtPosition(position, &index);
> >+      if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
> >+    }
> >+    fragment = fragment->GetNextGlyphFragment();
> >+  }
> 
> Should this have an early exit so we don't keep looking at fragments,
> or is to catch the case of self-overlapping fragment tree?

Yes. If you had a circular textPath for instance then you would need to
return the last matching character and not the first. Comment added to
source code to make this clear.

> 
> >+nsSVGCairoGlyphMetrics::GetComputedTextLength(float *_retval)
> >+{
> ...
> >+  *_retval = extent.x_advance + extent.y_advance;
> 
> Why add the y advance?

I assume the y_advance could be non-zero for Chinese glyphs
(in which case the x_advance would be zero), so the addition really just
selects one or the other. I have now changed this to abs(x) + abs(y) in
case Hebrew glyphs have a negative x_advance.

> 
> >+nsSVGCairoGlyphMetrics::GetSubStringLength(PRUint32 charnum, PRUint32 nchars, float *_retval)
> >+{
> ...
> >+  *_retval = extent.x_advance + extent.y_advance;
> 
> Same.
> 
> >+/** Implements float getRotationOfChar(in unsigned long charnum); */
> >+NS_IMETHODIMP
> >+nsSVGCairoGlyphMetrics::GetRotationOfChar(PRUint32 charnum, float *_retval)
> >+{
> >+  const double pi = 3.141592653;
> 
> Please use M_PI, with appropriate ifdef/def for msvc.

Done with infrastructure under bug 323907.

> 
> >+nsSVGCairoGlyphMetrics::GetCharNumAtPosition(nsIDOMSVGPoint *point, PRInt32 *_retval)
> >+{
> ...
> >+    cairo_text_path(mCT,
> >+                    NS_ConvertUCS2toUTF8(Substring(text, i, 1)).get());
> >+    cairo_set_matrix(mCT, &matrix);
> >+
> >+    if (cairo_in_fill(mCT, x, y))
> >+      *_retval = i;
> 
> GetCharNumAtPosition is supposed to do testing on character boxes, like
> the hit testing.
> 
> 

cairo_in_fill did not do what I thought it did. Now fixed.

Comment 6

[Robert Longson [:longsonr]]

Attachment: testcase for various text DOM functions

Contains calls to getStartPositionOfChar, getCharNumAtPosition, getExtentOfChar for text and textPath.

Comment 7

[tor]

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

It looks like you accidentally attached the previous version of the patch again.

Comment 8

[Robert Longson [:longsonr]]

Why do you think it's an old version? Is there something I should have changed that I have not.

I did attach the change twice - the address patch is the same as address review comments but in address I failed to obsolete the 2005-12-05 and to write in any comments about the changes I had made.

Robert.

Comment 9

[tor]

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

I say it looks like the old one because it still has things like this:

> protected:
>+  // nsSVGElement overrides
>+  virtual PRBool IsEventName(nsIAtom* aName);
> 

> //----------------------------------------------------------------------
>+// nsSVGElement overrides
>+
>+PRBool
>+nsSVGTSpanElement::IsEventName(nsIAtom* aName)
>+{
>+  return IsGraphicElementEventName(aName);
>+}

Comment 10

[Robert Longson [:longsonr]]

bug 317709 just fixes textPath elements to be clickable. 

tspan elements are currently clickable because I changed them in bug 311031 from being derived from nsSVGStylableElement to being derived from nsSVGGraphicElement (so that I could call GetBBox). This had the side effect of making them clickable since all nsSVGGraphicElements are clickable. 

tspan elements should not, however derive from nsSVGGraphicElement as they are not transformable.

Since I rewrote GetComputedTextLength not to require a call to GetBBox, I revert tspan elements to being properly derived from nsSVGStylableElement but I then need to make them explicitly clickable.

Comment 11

[Robert Longson [:longsonr]]

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

asking for review again given explanation of tspan changes

Comment 12

[tor]

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

There are inconsistent error returns from content - sometimes a GetTextContextMetrics() results in NS_ERROR_FAILURE being returned from the parent function, sometimes NS_OK.  Should these all be the same?

Implementing the cairo text glue, we've found at least two platforms that behave badly (crash or invalidate the context) if an empty string is passed to cairo_text_extents.  It looks like GetComputedTextLength (and maybe others) could use some bulletproofing for this.  Ie, "if (test.IsEmpty()) ...".

GetCharNumAtPosition is generating the path and checking the fill extents.  I'd prefer that it generate the rectangle from the return of text_extents on the character and test on that, as generating/bounding a path is relatively expensive and nsSVGCairoGlyphGeometry::ContainsPoint() should agree with this (for example, a :hover effect used alongside this method).

Comment 13

[Robert Longson [:longsonr]]

Attachment: address review comments

> There are inconsistent error returns from content - sometimes a
> GetTextContextMetrics() results in NS_ERROR_FAILURE being returned from the
> parent function, sometimes NS_OK.  Should these all be the same?

GetTextContentMetrics would return NULL if the element was set to display: none. 
In the case of getNumberOfChars, getComputedTextLength and getSubStringLength
I think NS_OK is reasonable with a return value of 0.

getStartPositionOfChar, getEndPositionOfChar, getExtentOfChar, getRotationOfChar,
can't return anything reasonable as they need to return position/rotation data.
There isn't any hence the NS_ERROR_FAILURE value.

I have changed getCharNumAtPosition to return -1 and NS_OK in this patch if the
element is not rendered.

> 
> Implementing the cairo text glue, we've found at least two platforms that
> behave badly (crash or invalidate the context) if an empty string is passed to
> cairo_text_extents.  It looks like GetComputedTextLength (and maybe others)
> could use some bulletproofing for this.  Ie, "if (test.IsEmpty()) ...".

The callers of the functions check that there is at least one character where necessary.
E.g. nsSVGUtils::GetComputedTextLength only calls GetComputedTextLength if
fragment->GetNumberOfChars() > 0. I can add belt and braces checks to
the nsSVGCairoGlyphMetrics functions if you wish but I have not done so
in this patch.

> 
> GetCharNumAtPosition is generating the path and checking the fill extents.  I'd
> prefer that it generate the rectangle from the return of text_extents on the
> character and test on that, as generating/bounding a path is relatively
> expensive and nsSVGCairoGlyphGeometry::ContainsPoint() should agree with this
> (for example, a :hover effect used alongside this method).

I have changed the implementation of nsSVGCairoGlyphMetrics::GetExtentOfChar and
nsSVGCairoGlyphMetrics::GetCharNumAtPosition.

Comment 14

[Robert Longson [:longsonr]]

Attachment: update to tip

Integrate with bug 183156. This just changes UCS2 to UTF16 in nsSVGCairoGlyphMetrics.cpp

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 211741 [details] [diff] [review]
update to tip

- In nsSVGCairoGlyphMetrics::GetStartPositionOfChar():

+  nsSVGCharacterPosition *cp;
+
+  if (NS_FAILED(mSource->GetCharacterPosition(&cp)))
+    return NS_ERROR_FAILURE;
+
+  nsCOMPtr<nsIDOMSVGPoint> point = do_CreateInstance(NS_SVGPOINT_CONTRACTID);
+
+  NS_ASSERTION(point, "could not create point");
+  if (!point) {
+    delete [] cp;
+    return NS_ERROR_FAILURE;
+  }

If you'd create the point before you call GetCharcterPosition() above you wouldn't need the manual delete of [] cp...

sr=jst

Comment 16

[Robert Longson [:longsonr]]

Attachment: address jst's sr comments

Comment 17

[tor]

Attachment: final version checked in - previous contained fix from bug 327507

Comment 18

[tor]

Checked in for Robert.