Implement various text, tspan and textPath DOM functions

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 204717 [details] [diff] [review]
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.
(Assignee)

Comment 2

13 years ago
Created attachment 205067 [details] [diff] [review]
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.
Attachment #204717 - Attachment is obsolete: true
Attachment #205067 - Flags: review?(tor)
(Assignee)

Updated

13 years ago
Assignee: general → longsonr
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

12 years ago
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.
Attachment #205067 - Flags: review?(tor) → review-
(Assignee)

Updated

12 years ago
Depends on: 323907
(Assignee)

Comment 4

12 years ago
Created attachment 209824 [details] [diff] [review]
address
(Assignee)

Comment 5

12 years ago
Created attachment 209825 [details] [diff] [review]
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.
Attachment #205067 - Attachment is obsolete: true
Attachment #209824 - Attachment is obsolete: true
Attachment #209825 - Flags: review?(tor)
(Assignee)

Comment 6

12 years ago
Created attachment 209826 [details]
testcase for various text DOM functions

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

Comment 7

12 years ago
Comment on attachment 209825 [details] [diff] [review]
address review comments

It looks like you accidentally attached the previous version of the patch again.
Attachment #209825 - Flags: review?(tor)
(Assignee)

Comment 8

12 years ago
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

12 years ago
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);
>+}
(Assignee)

Comment 10

12 years ago
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.
(Assignee)

Comment 11

12 years ago
Comment on attachment 209825 [details] [diff] [review]
address review comments

asking for review again given explanation of tspan changes
Attachment #209825 - Flags: review?(tor)

Comment 12

12 years ago
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).
(Assignee)

Comment 13

12 years ago
Created attachment 210226 [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?

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.
Attachment #209825 - Attachment is obsolete: true
Attachment #210226 - Flags: review?(tor)
Attachment #209825 - Flags: review?(tor)

Updated

12 years ago
Attachment #210226 - Flags: review?(tor) → review+
(Assignee)

Updated

12 years ago
Attachment #210226 - Flags: superreview?(jst)
(Assignee)

Comment 14

12 years ago
Created attachment 211741 [details] [diff] [review]
update to tip

Integrate with bug 183156. This just changes UCS2 to UTF16 in nsSVGCairoGlyphMetrics.cpp
Attachment #210226 - Attachment is obsolete: true
Attachment #211741 - Flags: superreview?(jst)
Attachment #211741 - Flags: review?(tor)
Attachment #210226 - Flags: superreview?(jst)

Updated

12 years ago
Attachment #211741 - Flags: review?(tor) → review+
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
Attachment #211741 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 16

12 years ago
Created attachment 212204 [details] [diff] [review]
address jst's sr comments

Comment 17

12 years ago
Created attachment 212250 [details] [diff] [review]
final version checked in - previous contained fix from bug 327507
Attachment #212204 - Attachment is obsolete: true

Comment 18

12 years ago
Checked in for Robert.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Depends on: 311031, 313897
(Assignee)

Updated

12 years ago
Blocks: 339039
You need to log in before you can comment on or make changes to this bug.