314627 - implement SVGTSpanElement.getExtentOfChar

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.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

getExtentOfChar is not implemented for tspan elements. It is however implemented for text elements.

Reproducible: Always

Steps to Reproduce:
see testcase
Actual Results:  
not implemented exception in Javascript console

Comment 1

[Robert Longson [:longsonr]]

Attachment: testcase

For this testcase to work properly under cairo the patch for bug #313897 is required as well.

Comment 2

[Robert Longson [:longsonr]]

Attachment: implement SVGTSpanElement.getExtentOfChar

Comment 3

[tor]

Comment on attachment 201529 [details] [diff] [review]
implement SVGTSpanElement.getExtentOfChar

>+// nsISVGTextContentMetrics
>+NS_IMETHODIMP
>+nsSVGTSpanFrame::GetExtentOfChar(PRUint32 charnum, nsIDOMSVGRect **_retval)
>+{
>+  nsISVGGlyphFragmentNode* node = GetFirstGlyphFragmentChildNode();
>+  if (!node) return NS_ERROR_FAILURE; // xxx return some index-out-of-range error;
>+  return nsSVGUtils::GetExtentOfChar(node, charnum, _retval);
>+}

NS_ERROR_DOM_INDEX_SIZE_ERR (likewise nsSVGTextFrame::GetExtentOfChar and nsSVGUtils::GetExtentOfChar)

>+  /*
>+   * Returns the glyph fragment containing a particular character
>+   */
>+  static nsISVGGlyphFragmentLeaf *
>+  GetGlyphFragmentAtCharNum(nsISVGGlyphFragmentNode* node,
>+                            PRUint32 charnum);
>+
>+  /*

Unless you have plans for using this method outside of nsSVGUtils, it should be private or file static.

Note that neither this patch or your other one will return the right answer for textPath, but that can be addressed in another bug.

Comment 4

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 5

[Robert Longson [:longsonr]]

Attachment: integrate with bug 313968

The previous patch does not compile now that Bug 313968 has been checked in.
This patch only changes nsSVGTSpanElement::GetTextContentMetrics from the implementation in the previous patch.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 202365 [details] [diff] [review]
integrate with bug 313968

- In nsSVGTSpanElement::GetTextContentMetrics():

+  nsIFrame* frame = presShell->GetPrimaryFrameFor(this);
+
+  if (!frame) {
+    NS_ERROR("no frame");

We'd hit this error if someone was to set one of these elements to display: none. I'd say loose the NS_ERROR() here as this doesn't seem like an error.

+    return nsnull;
+  }
+  
+  nsISVGTextContentMetrics* metrics;
+  CallQueryInterface(frame, &metrics);
+  NS_ASSERTION(metrics, "wrong frame type");

What if someone changes the display type of one of these elements to something completely different that causes us to create a frame that doesn't implement this interface? No need to assert (or even warn) here, the code will do the right thing either way, right?

sr=jst with that fixed.

Comment 7

[Robert Longson [:longsonr]]

Attachment: address superreview comments

Comment 8

[tor]

Checked in for Robert.