Closed Bug 436904 Opened 16 years ago Closed 16 years ago

implementing Canvas text spec

Categories

(Core :: Graphics: Canvas2D, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ebutler, Assigned: ebutler)

References

(Depends on 1 open bug, )

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0
Build Identifier: Trunk

Implementing the HTML 5 specification for text rendering on Canvas. Covers the attributes font, textAlign, and textBaseline and the functions fillText, strokeText, and measureText.

Reproducible: Always
Assignee: nobody → ebutler
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Attached patch version 1.0 (obsolete) — Splinter Review
This patch implements most of the spec for canvas text rendering and fixes a few bugs with the old Mozilla-specific canvas text rendering code.

The only notable part of the spec not implemented is support for ideographic and hanging baselines. If 'textBaseline' is set to either value, the alphabetic baseline will be silently used instead.

The existing moz text code was used to implement this, so 'mozTextStyle' and 'font' are actually the same value, and setting one will modify the other. A few tweaks and bug fixes were made to 'mozTextStyle' to have it conform with the spec for 'font,' such as changing the default value. RTL text should display correctly with all of the functions from the spec, though it does not work with any of the Mozilla-specific functions.
Attachment #323449 - Flags: review?(vladimir)
This is a test case that demonstrates nearly all the features of the new text functions. It shows off the alignment and baseline anchoring, filling and stroking, changing the font, measureText, and several small cases covered in the spec, such as replacing the tab character with a space.
Status: NEW → ASSIGNED
Comment on attachment 323449 [details] [diff] [review]
version 1.0

>diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>@@ -275,6 +275,51 @@
> NS_INTERFACE_MAP_END
> 
> /**
>+ ** nsTextMetrics
>+ **/
>+#define NS_TEXTMETRICS_PRIVATE_IID \
>+    { 0xc5b1c2f9, 0xcb4f, 0x4394, { 0xaf, 0xe0, 0xc6, 0x59, 0x33, 0x80, 0x8b, 0xf3 } }
>+class nsTextMetrics : public nsIDOMTextMetrics
>+{
>+public:
>+    nsTextMetrics(float w)
>+        : width(w)
>+    {
>+
>+    }

Nit: { } on just one line

>+    virtual ~nsTextMetrics()
>+    {
>+
>+    }

Same here, except { } can just go after the (), so it's all on one line.

>+    NS_DECLARE_STATIC_IID_ACCESSOR(NS_TEXTMETRICS_PRIVATE_IID)
>+
>+    NS_IMETHOD GetWidth(float* w)
>+    {

Nit: { on previous line


>+    /*
>+     * Implementation of the fillText and strokeText functions with the
>+     * operation abstracted to a flag. Follows the HTML 5 spec for rendering
>+     * text. Will query for the fourth, optional argument.
>+     */
>+    nsresult drawText(const nsAString& text,
>+                      float x,
>+                      float y,
>+                      TextDrawOperation op
>+                      );

More style nits: the hanging ); is weird; just close the function normally after op.

> nsCanvasRenderingContext2D::nsCanvasRenderingContext2D()
>     : mValid(PR_FALSE), mCanvasElement(nsnull),
>-      mSaveCount(0), mCairo(nsnull), mSurface(nsnull), mStyleStack(20)
>+      mSaveCount(0), mCairo(nsnull), mSurface(nsnull), mStyleStack(20),
>+      mTextAlign(TEXT_ALIGN_START), mTextBaseline(TEXT_BASELINE_ALPHABETIC)
> {
> }

textAlign/textBaseline is part of the current drawing state; it needs to be added to the
ContextState and saved/restored appropriately.

> 
>@@ -1456,6 +1539,366 @@
> //
> // text
> //
>+
>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::SetFont(const nsAString& font)
>+{
>+    // font and mozTextStyle are the same value
>+    return SetMozTextStyle(font);
>+}
>+
>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::GetFont(nsAString& font)
>+{
>+    // font and mozTextStyle are the same value
>+    return GetMozTextStyle(font);
>+}

Do this the other way around -- move the impls of mozTextStyle here, and implement mozTextSTyle in terms of SetFont/GetFont.  We want to be able to easily get rid of moz* stuff later on.

>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::SetTextAlign(const nsAString& ta)
>+{


>+    else
>+        return NS_ERROR_FAILURE;

I think this is supposed to return NS_ERROR_DOM_SYNTAX_ERR?  Check the spec to see what setting these to invalid values should throw.

(The various setters/getters also need to modify the current state element so that they can be pushed/popped, as mentioned before.)

>+
>+nsresult nsCanvasRenderingContext2D::drawText(const nsAString& rawText,
>+                                              float x,
>+                                              float y,
>+                                              TextDrawOperation op
>+                                              )

Weird hanging ) again.

>+{
>+    // get js context to parse extra arg
>+    nsresult rv;
>+
>+    if (!mCanvasElement) {
>+        return NS_ERROR_FAILURE;
>+    }

No need for {}'s; you also want a check for !mValid || !mCanvasElement here, I think.

>+    nsAXPCNativeCallContext* ncc = nsnull;
>+    rv = nsContentUtils::XPConnect()->
>+        GetCurrentNativeCallContext(&ncc);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (!ncc) {
>+        return NS_ERROR_FAILURE;
>+    }

{}'s again

>+
>+    JSContext* ctx = nsnull;
>+
>+    rv = ncc->GetJSContext(&ctx);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    PRUint32 argc;
>+    jsval* argv = nsnull;
>+
>+    ncc->GetArgc(&argc);
>+    ncc->GetArgvPtr(&argv);
>+
>+    double maxWidth = 0;
>+
>+    // get the optional fourth argument
>+    if (argc > 3) {
>+        if (!ConvertJSValToDouble(&maxWidth, ctx, argv[3])) {
>+            return NS_ERROR_INVALID_ARG;
>+        }
>+        // spec is undefined for when a value <= 0 is passed,
>+        // so return an error
>+        if (maxWidth <= 0) {
>+            return NS_ERROR_INVALID_ARG;
>+        }

Lots of extra unnecessary {}'s... won't point them out, but in general if it's just if() with a single statement, don't use the {}'s... especially if it's a return.

>+    }
>+
>+    gfxFontGroup* fontgrp = GetCurrentFontStyle();
>+    NS_ASSERTION(fontgrp, "font group is null");
>+
>+    // replace all the whitespace characters with U+0020 SPACE
>+    nsAutoString textToDraw(rawText);
>+    TextReplaceWhitespaceCharacters(textToDraw);
>+
>+    const PRUnichar* textData;
>+    textToDraw.GetData(&textData);
>+
>+    // get direction property from the frame
>+    nsIFrame* frame;
>+    if (mCanvasElement->GetPrimaryCanvasFrame(&frame)!=NS_OK) {
>+        return NS_ERROR_FAILURE;
>+    }

{} again, also spaces around the "!=" in the clause.  This type of stuff tends to be written as:

nsresult rv;
rv = mCanvasElement->GetPrimaryCanvasFrame(&frame);
if (NS_FAILED(rv))
  return rv;

so that the function call is clearer and not hidden in an if clause.

>+    PRBool isRTL = frame->GetStyleVisibility()->mDirection ==
>+        NS_STYLE_DIRECTION_RTL;
>+
>+    PRUint32 textrunflags = isRTL ? gfxTextRunFactory::TEXT_IS_RTL : 0;
>+
>+    // app units conversion factor
>+    PRUint32 aupdp;
>+    GetAppUnitsValues(&aupdp, NULL);
>+
>+    gfxTextRunCache::AutoTextRun textRun;
>+    textRun = gfxTextRunCache::MakeTextRun(textData,
>+                                           textToDraw.Length(),
>+                                           fontgrp,
>+                                           mThebesContext,
>+                                           aupdp,
>+                                           textrunflags);
>+
>+    if (!textRun.get()) {
>+        return NS_ERROR_FAILURE;
>+    }
>+
>+    gfxPoint pt(x, y);
>+    
>+    // get the text width
>+    PRBool tightBoundingBox = PR_FALSE;
>+    gfxTextRun::Metrics textRunMetrics = textRun->MeasureText(/* offset = */ 0,
>+                                                       textToDraw.Length(),
>+                                                       tightBoundingBox,
>+                                                       mThebesContext,
>+                                                       nsnull);
>+    gfxFloat textWidth = textRunMetrics.mAdvanceWidth/gfxFloat(aupdp);
>+
>+
>+    // offset pt x based on text align
>+    gfxFloat anchorX;
>+
>+    if (mTextAlign==TEXT_ALIGN_CENTER) {
>+        anchorX = .5;
>+    }
>+    else if (mTextAlign==TEXT_ALIGN_LEFT ||
>+        (!isRTL && mTextAlign==TEXT_ALIGN_START) ||
>+        (isRTL && mTextAlign==TEXT_ALIGN_END)) {
>+        anchorX = 0;
>+    }
>+    else {

style nits: else right after } please, spaces around =='s (spaces around comparison operators everywhere, I won't point them out).  Would be good to align the continuations of the if clause as well to right after the (, but that's minor.

>+        anchorX = 1;
>+    }
>+
>+    if (isRTL) {
>+
>+        pt.x += (1 - anchorX) * textWidth;
>+    }
>+    else {
>+        pt.x -= anchorX * textWidth;
>+    }
>+
>+    // offset pt y based on text baseline
>+    NS_ASSERTION(fontgrp->FontListLength()>0, "font group contains no fonts");
>+    const gfxFont::Metrics& fontMetrics = fontgrp->GetFontAt(0)->GetMetrics();
>+
>+    gfxFloat anchorY;
>+
>+    switch (mTextBaseline)
>+    {
>+    case TEXT_BASELINE_TOP:
>+        anchorY = fontMetrics.emAscent;
>+        break;
>+    case TEXT_BASELINE_HANGING:
>+        anchorY = 0; // currently unavailable
>+        break;
>+    case TEXT_BASELINE_MIDDLE:
>+        anchorY = (fontMetrics.emAscent-fontMetrics.emDescent)*.5f;
>+        break;
>+    case TEXT_BASELINE_ALPHABETIC:
>+        anchorY = 0;
>+        break;
>+    case TEXT_BASELINE_IDEOGRAPHIC:
>+        anchorY = 0; // currently unvailable
>+        break;
>+    case TEXT_BASELINE_BOTTOM:
>+        anchorY = -fontMetrics.emDescent;
>+        break;
>+    default:
>+        NS_ASSERTION(0, "mTextBaseline holds invalid value");
>+        return NS_ERROR_FAILURE;
>+    }
>+
>+    pt.y += anchorY;
>+
>+    // if text is over maxWidth, then scale the text horizonally such that its
>+    // width is precisely maxWidth
>+    if (maxWidth>0 && textWidth > maxWidth) {
>+        Save();
>+        // translate the anchor point to 0, then scale and translate back
>+        Translate(x, 0);
>+        Scale((float)(maxWidth/textWidth), 1);
>+        Translate(-x, 0);

Don't use the 2D context's methods here, make the cairo calls directly.  Save/Translate/Scale do a bunch of extra work that you shouldn't need to do.  This goes for code below this point as well.

>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::MeasureText(const nsAString& rawText,
>+                                        nsIDOMTextMetrics** _retval)
>+{
>+    // replace all the whitespace characters with U+0020 SPACE
>+    nsAutoString textToDraw(rawText);
>+    TextReplaceWhitespaceCharacters(textToDraw);
>+
>+    float textWidth;
>+    if (MozMeasureText(textToDraw, &textWidth)!=NS_OK) {
>+        return NS_ERROR_FAILURE;
>+    }

Same thing as earlier; move the implementation here, and have MozMeasureText call it.  It's ok if MozMeasureText does the space munging that it didn't do before.

> NS_IMETHODIMP
> nsCanvasRenderingContext2D::SetMozTextStyle(const nsAString& textStyle)
> {
>@@ -1508,7 +1951,16 @@
> 
>     nsStyleSet *styleSet = presShell->StyleSet();
> 
>-    nsRefPtr<nsStyleContext> sc = styleSet->ResolveStyleForRules(nsnull,rules);
>+    // get the frame's style context to use as the parent
>+    if (!mCanvasElement) {
>+        return NS_ERROR_FAILURE;
>+    }
>+    nsIFrame* frame;
>+    if (mCanvasElement->GetPrimaryCanvasFrame(&frame)!=NS_OK) {
>+        return NS_ERROR_FAILURE;
>+    }

Same thing with rv, {}, etc.


>+  void fillText(in DOMString text, in float x, in float y);
>+/*
>+  void fillText(in DOMString text, in float x, in float y, in float maxWidth);
>+*/
>+  void strokeText(in DOMString text, in float x, in float y);
>+/*
>+  void strokeText(in DOMString text, in float x, in float y, in float maxWidth);
>+*/

Hrm, so there's a new [optional] IDL attribute that I think you should use here (it's new since Gecko 1.9).  But I don't know what value you'd get for an optional float arg or how to figure out the number of args actually passed -- you should be able to get that info, though.  This would simplify a bunch of the implementation code, since you wouldn't need to reach in to JS.

If the [optional] stuff works well, you could fix up the other methods here that have optional params to use it as well.

>   attribute DOMString mozTextStyle; 
>   void mozDrawText(in DOMString textToDraw);
>   float mozMeasureText(in DOMString textToMeasure);
>diff --git a/dom/public/nsDOMClassInfoID.h b/dom/public/nsDOMClassInfoID.h
>--- a/dom/public/nsDOMClassInfoID.h
>+++ b/dom/public/nsDOMClassInfoID.h
>@@ -353,6 +353,7 @@
>   eDOMClassInfo_CanvasRenderingContext2D_id,
>   eDOMClassInfo_CanvasGradient_id,
>   eDOMClassInfo_CanvasPattern_id,
>+  eDOMClassInfo_TextMetrics_id,

Hrm, did that really get called TextMetrics in the spec?  I guess that's ok, sounds scarily generic.. hope we don't get collisions.

In general the patch looks good though; other than the nits, the major issues are:

- text attributes need to be part of the context state
- implement old moz* methods in terms of new API, as opposed to the other way around.

Also, needs a bunch of tests in content/canvas/tests -- you don't necessarily have to follow the exact way those existing tests are written (in fact, please don't -- the pixel comparison function is horrible, and shouldn't need 5 copies of the same args passed to it), and reftests might make more sense.
Attachment #323449 - Flags: review?(vladimir) → review-
Attached patch veraion 1.1 (obsolete) — Splinter Review
corrected initial patch by:
- making the attributes part of the context state
- adding several reftests and mochitests
- fixing all the style nits
Attachment #323449 - Attachment is obsolete: true
Attachment #323770 - Flags: review?(vladimir)
Comment on attachment 323770 [details] [diff] [review]
veraion 1.1

Looks great!  Code looks good, thanks for making all the changes.  Tests look great as well.

A few small style nits:


>+nsresult nsCanvasRenderingContext2D::drawText(const nsAString& rawText,
>+                                              float x,
>+                                              float y,
>+                                              float maxWidth,
>+                                              TextDrawOperation op)

nsresult on separate line from method name

>@@ -1620,30 +2058,15 @@
> NS_IMETHODIMP
> nsCanvasRenderingContext2D::MozMeasureText(const nsAString& textToMeasure, float *retVal)
> {
>-    const PRUnichar* textdata;
>-    textToMeasure.GetData(&textdata);
>+    nsIDOMTextMetrics* metrics;
>+    nsresult rv;
>+    rv = MeasureText(textToMeasure, &metrics);
>+    if (NS_FAILED(rv))
>+        return rv;
>+    rv = metrics->GetWidth(retVal);
>+    NS_RELEASE(metrics);

use nsCOMPtr<nsIDOMTextMetrics> metrics; -- bare pointers and manual NS_RELEASE are :(
Attachment #323770 - Flags: review?(vladimir) → review+
Attached patch version 1.2Splinter Review
identical to version 1.1 except with the two nits fixed.
Attachment #323770 - Attachment is obsolete: true
Pushed changeset 963ced95e2d1 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 1534566
You need to log in before you can comment on or make changes to this bug.