Implement extended TextMetrics object

NEW
Assigned to

Status

()

defect
5 years ago
9 months ago

People

(Reporter: fscholz, Assigned: richard, NeedInfo)

Tracking

(Blocks 1 bug, {dev-doc-needed, parity-chrome})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=Canvas], )

Attachments

(2 attachments, 2 obsolete attachments)

Currently, we are only implementing the width attribute of the TextMetrics object that is returned by ctx.measureText(). The canvas v5 API extended this object to some more measurements:

[Exposed=(Window,Worker)]
interface TextMetrics {
  // x-direction
  readonly attribute double width; // advance width
  readonly attribute double actualBoundingBoxLeft;
  readonly attribute double actualBoundingBoxRight;

  // y-direction
  readonly attribute double fontBoundingBoxAscent;
  readonly attribute double fontBoundingBoxDescent;
  readonly attribute double actualBoundingBoxAscent;
  readonly attribute double actualBoundingBoxDescent;
  readonly attribute double emHeightAscent;
  readonly attribute double emHeightDescent;
  readonly attribute double hangingBaseline;
  readonly attribute double alphabeticBaseline;
  readonly attribute double ideographicBaseline;
};

It's been implemented in Blink [1] but is behind a flag for now [2].

[1] https://code.google.com/p/chromium/issues/detail?id=277215
[2] https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/SgofW_bQ3ps/pVtmYmChQoEJ
WebKit: https://bugs.webkit.org/show_bug.cgi?id=82798

MDN: https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics
Reporter

Updated

4 years ago
Blocks: 1119470
One thing to note here is that this API probably needs to be updated to take into account vertical layout. As it stands now, it's basically horizontal only.
Assignee

Comment 2

3 years ago
I'm looking to implement the metrics. Anything I need to be aware of/anything I need to take into account?
Assignee

Comment 3

3 years ago
I'll be more specific as this is my first time working on firefox - should I put it behind a flag?
Assignee

Comment 4

3 years ago
Attachment #8739746 - Flags: review?(jfkthame)
Comment on attachment 8739746 [details] [diff] [review]
Implement extended TextMetrics object

Review of attachment 8739746 [details] [diff] [review]:
-----------------------------------------------------------------

Richard, thanks for working on this! Sorry you haven't had feedback sooner; for future reference, when you post a patch that you want to be considered, set the "review" (r?) flag on it, directed at an appropriate person for the code in question (you can look at previous bugs in the same area, and/or the mercurial history of the code, to see who might be a good reviewer). That should get it noticed.

:smaug, flagging you for r? because this touches a .webidl file; I'm not clear whether it strictly needs DOM peer review, as it's not "adding a _new_ interface" but just filling out currently-unimplemented attributes of an existing one, but I figured we should at least ask.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3322,2 @@
>    Optional<double> maxWidth;
> +  aError = DrawOrMeasureText(aRawText, 0, 0, maxWidth, TextDrawOperation::MEASURE, &metrics);

Now that the TextMetrics object is created within DrawOrMeasureText, I think the code will be easier to follow if you revise the signature of the DrawOrMeasureText helper such that it takes the ErrorResult as a reference param (just like its callers), and returns the TextMetrics* (or null, if not measuring) as its function result.

@@ +3807,5 @@
>  
>    if (currentFontStyle->GetStyle()->size == 0.0F) {
> +    //if (aWidth) {
> +    //  *aWidth = 0;
> +    //}

Don't we need to return a valid object here (even though its fields will all be zero), if aMetrics is non-null?

@@ +3844,5 @@
>    }
>    processor.mCtx = this;
>    processor.mOp = aOp;
>    processor.mBoundingBox = gfxRect(0, 0, 0, 0);
> +  processor.mDoMeasureBoundingBox = doCalculateBounds || !mIsEntireFrameInvalid || aOp==TextDrawOperation::MEASURE;

Style nit: spaces around the "==" operator, please. (Yes, I know there are some existing errors of this kind, but let's not add more.)

@@ +3898,5 @@
>  
>    switch (state.textBaseline)
>    {
>    case TextBaseline::HANGING:
> +      baselineAnchor = fontMetrics.emAscent * 0.8f;

Is there any clear basis for the 0.8 factor here -- e.g. a spec that recommends this as a fallback -- or it's just "seems like a reasonable guess"?

@@ +3910,5 @@
>    case TextBaseline::ALPHABETIC:
>      baselineAnchor = 0;
>      break;
> +  case TextBaseline::IDEOGRAPHIC:
> +    // fall through; best we can do with the information available

Is there a reason you've changed the behavior for IDEOGRAPHIC here (making it like BOTTOM instead of ALPHABETIC)? It's not clear to me which of these is likely to be closer to "correct",

@@ +3939,5 @@
> +                       -fontMetrics.emDescent - baselineAnchor);           /*ideographicBaseline*/
> +  }
> +
> +  // if only measuring, don't need to do any more work
> +  if (aOp==TextDrawOperation::MEASURE) {

While you're moving this line, please add the missing spaces around the "==".

::: dom/canvas/TextMetrics.h
@@ +126,5 @@
> +  float emHeightAscent;
> +  float emHeightDescent;
> +  float hangingBaseline;
> +  float alphabeticBaseline;
> +  float ideographicBaseline;

It seems odd to use 'float' for all the fields here, when the webidl declares the attributes as 'double', and the underlying gfx font code also works with double (gfxFloat) values.

I realize the existing width field is already declared as float, but if we're touching this object at all, perhaps we should fix that.

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ -323,5 @@
>    readonly attribute double width; // advance width
>  
> -  /*
> -   * NOT IMPLEMENTED YET
> -

This looks straightforward enough, but my understanding is that any patch that touches .webidl should be reviewed by a DOM peer. So I'll flag someone from DOM to check this.

::: testing/web-platform/tests/2dcontext/drawing-text-to-the-canvas/2d.text.measure.actualBoundingBoxAscent.basic.html
@@ +1,2 @@
> +<!DOCTYPE html>
> +<!-- DO NOT EDIT! This test has been generated by tools/gentest.py. -->

Are there modifications to gentest.py involved here, to generate the new test files? There presumably must be additions to the .yaml, at least. Those should be included in the patch, then, not only the generated files.
Attachment #8739746 - Flags: review?(jfkthame) → review?(bugs)
All the .webidl changes do need a dom peer review. The idea has been to get another pair of eyes to look at the APIs we expose to the web. (not necessarily to the API implementations). Historically we used to have the problem that some APIs got somewhat accidentally exposed to the web.
Comment on attachment 8739746 [details] [diff] [review]
Implement extended TextMetrics object

r+ for the webidl. Looks like blink uses still float for the types, but is otherwise compatible with the spec.

Someone needs to review the rest of the patch.
Attachment #8739746 - Flags: review?(bugs) → review+
Richard, I know it's been a long time since you posted the patch. Did you want to update it based on the review comments or should we get someone else to do that and get it landed?
Flags: needinfo?(richard)
Assignee

Comment 9

3 years ago
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Richard, I know it's been a long time since you posted the patch. Did you
> want to update it based on the review comments or should we get someone else
> to do that and get it landed?

Thanks Jonathan, Olli and Timothy, I'm happy to fix up things based on the review comments over the weekend. Do I just resubmit the same way once I've made the changes?
Flags: needinfo?(richard)
Yes, that's fine; just attach a new patch, and mark the older one as obsolete (there's a checkbox in the attachment details for that, or I think it may happen automatically if the filename is the same), and request review on the new version. Thanks!
Assignee

Comment 11

3 years ago
Jonathan, in reply to your comment on baseline changes, ideographic baseline (baseline for CJK characters) is supposed to be lower than alphabetic. I read somewhere about using the bottom as the ideographic baseline, but I can't remember where. I've also read about using 3/4 of the descent from alphabetic as a good substitute as well.

Hanging Baseline (specifically for Tibetan script) is supposed to be slightly lower than the top. 0.8 was the fudge factor recommended in the baseline information below. I believe blink uses 0.8 as well.

Sources I remember looking at include: https://www.w3.org/Graphics/SVG/WG/wiki/How_to_determine_dominant_baseline#Ideographic
and
https://www.microsoft.com/typography/otspec/BASE.htm

On the testing, I didn't modify gentest.py! Now that I know, I'll look it up and make the changes, hopefully without breaking everything.

The other changes seem very reasonable and I've already started on those.
And no need to re-ask review from me if the .webidl part stays the same :)
Just ask review from :jfkthame
(In reply to Richard Matheson from comment #11)
> Jonathan, in reply to your comment on baseline changes, ideographic baseline
> (baseline for CJK characters) is supposed to be lower than alphabetic. I
> read somewhere about using the bottom as the ideographic baseline, but I
> can't remember where.

I agree the ideographic baseline would typically be lower than alphabetic, but I suspect that in most cases it shouldn't really be as low as the bottom. See for example the illustration at https://developer.apple.com/library/iad/documentation/WebKit/Reference/CanvasRenderingContext2DClassRef/index.html#//apple_ref/javascript/instp/CanvasRenderingContext2D/textBaseline, where the ideographic baseline is only slightly below the alphabetic.

(The "ideal" value would be dependent on the font design, and OpenType provides the 'BASE' table to allow designers to specify it, but we don't currently have any support for using that.)

The same applies to the hanging baseline: ideally, we'd get it from the font, but we don't yet have any support for that. (And many fonts don't include full baseline tables anyway.) I guess 0.8 is as good a default as anything for now.

For both of these, it'd be good to define a constant such as

  const float kHangingBaselineDefault = 0.8;     // hanging baseline, as fraction of ascent
  const float kIdeographicBaselineDefault = 0.5; // ideographic baseline, as fraction of descent

or something along those lines, and then use these in the computations.
Assignee

Updated

3 years ago
Attachment #8739746 - Attachment is obsolete: true
Assignee

Comment 14

3 years ago
Posted patch TextMetrics.diff (obsolete) — Splinter Review
I've updated the function signature as per review, amended gentest.py and texts2dtext.yaml to generate html tests, and replaced magic numbers for hanging and ideographic baselines with named constants.
Attachment #8788691 - Flags: review?(jfkthame)
Comment on attachment 8788691 [details] [diff] [review]
TextMetrics.diff

Review of attachment 8788691 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update, Richard; I think this is pretty close to being ready to go forward. I've noted a few minor code-style "nits" where we should align better with Gecko coding style[1], and also some points where I think the management of what gets allocated/returned/etc can be tweaked. I think the suggestions are all fairly straightforward, but get back to me if anything doesn't seem to make sense.

(Clearing the r? flag for now, pending a further round of revisions. Once the TextMetrics allocation/management issues are fixed up, I'll send the next rev through tryserver to check for any surprises there...)

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3301,5 @@
>                                     double aY,
>                                     const Optional<double>& aMaxWidth,
>                                     ErrorResult& aError)
>  {
> +   DrawOrMeasureText(aText, aX, aY, aMaxWidth, TextDrawOperation::FILL, aError);

Hmm, on thinking about this, we should probably include an assertion that DrawOrMeasureText does NOT return a TextMetrics object when called for a drawing operation. (If it did, we'd be leaking it.)

So maybe we should write something like

  TextMetrics* metrics = ...
  MOZ_ASSERT(!metrics);

which should have no effect in production builds, but debug builds will have the chance to confirm that nothing gets returned here.

Oh, but that will probably result in a 'variable set but not used' warning in non-debug builds. So we should use DebugOnly to handle that:

  DebugOnly<TextMetrics*> metrics = ...
  MOZ_ASSERT(!metrics); // drawing calls should not return a metrics record

I believe that will keep the compiler happy.

@@ +3310,5 @@
>                                       double aY,
>                                       const Optional<double>& aMaxWidth,
>                                       ErrorResult& aError)
>  {
> +  DrawOrMeasureText(aText, aX, aY, aMaxWidth, TextDrawOperation::STROKE, aError);

As above.

@@ +3321,2 @@
>    Optional<double> maxWidth;
> +  TextMetrics *metrics = DrawOrMeasureText(aRawText, 0, 0, maxWidth, TextDrawOperation::MEASURE, aError);

style nit: preferred style is to put the * on the end of the type name.

@@ +3323,1 @@
>    if (aError.Failed()) {

And here, let's also MOZ_ASSERT(!metrics), on the basis that if DrawOrMeasureText returns a failure code then it should NOT also return a metrics object.

But wait... if aError.Failed(), then DrawOrMeasureText will have returned nullptr, so we can simply "return metrics" here as well. Which means we don't really need the test at all. We can simplify this to just

  return DrawOrMeasureText(...);

and the caller still gets the same result.

@@ +3740,5 @@
>    bool mDoMeasureBoundingBox;
>  };
>  
> +
> +TextMetrics *CanvasRenderingContext2D::DrawOrMeasureText(const nsAString& aRawText,

style nit: this should be written as

TextMetrics*
CanvasRendering....

with the return type (including the * attached to the type name) on its own line.

@@ +3747,5 @@
>                                              const Optional<double>& aMaxWidth,
>                                              TextDrawOperation aOp,
> +                                            ErrorResult &aError)
> +{
> +  const double kHangingBaselineDefault = 0.8;     // approximated hanging baseline, as fraction of ascent 

nit: lose the trailing space. Also, line length should be limited to 80 chars, by rearranging or rewording somehow. Maybe something like

  // Factors used to provide approximate values for baselines, as we don't
  // yet support reading true values from the font (even if available):
  const double kHangingBaselineDefault = 0.8; // as a fraction of font's ascent
  const double kIdeographicBaselineDefault = 0.5; // as a fraction of descent

@@ +3813,5 @@
>      SetUserFontSet(presShell->GetPresContext()->GetUserFontSet());
>  
>    if (currentFontStyle->GetStyle()->size == 0.0F) {
> +    aError = NS_OK;
> +    return new TextMetrics(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0);

This should only return a TextMetrics if the operation was MEASURE, otherwise return nullptr.

@@ +3818,5 @@
>    }
>  
>    if (!IsFinite(aX) || !IsFinite(aY)) {
> +    aError = NS_OK;
> +    return new TextMetrics(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // This may not be correct - what should TextMetrics contain in the case of infinite width or height?

As above, only return a TextMetrics if the operation was MEASURE. But wait... the MeasureText caller passes a constant 0 for aX and aY, so that can't occur.

So instead, let's simply do

  MOZ_ASSERT(aOp != TextDrawOperation::MEASURE);
  return nullptr;

here.

@@ +3901,5 @@
>  
>    switch (state.textBaseline)
>    {
>    case TextBaseline::HANGING:
> +      baselineAnchor = fontMetrics.emAscent * 0.8f;

Use the named constant rather than a literal 0.8 here.

@@ +3902,5 @@
>    switch (state.textBaseline)
>    {
>    case TextBaseline::HANGING:
> +      baselineAnchor = fontMetrics.emAscent * 0.8f;
> +      break;

fix indentation

@@ +3913,5 @@
>    case TextBaseline::ALPHABETIC:
>      baselineAnchor = 0;
>      break;
> +  case TextBaseline::IDEOGRAPHIC:
> +    // fall through; best we can do with the information available

Shouldn't we be applying the kIdeographicBaselineDefault factor here?

@@ +3925,5 @@
>    // so instead we check the flags that will be used to initialize it.
>    uint16_t runOrientation =
>      (processor.mTextRunFlags & gfxTextRunFactory::TEXT_ORIENT_MASK);
> +
> +  TextMetrics *aMetrics = new TextMetrics(

Don't use the 'a' prefix here; that's for function arguments. So just

  TextMetrics* metrics = ...

(also note the position of the *).

@@ +3937,5 @@
> +                     fontMetrics.emAscent - baselineAnchor,                                  /*emHeightAscent*/
> +                     -fontMetrics.emDescent- baselineAnchor,                                 /*emHeightDescent*/
> +                     fontMetrics.emAscent * kHangingBaselineDefault - baselineAnchor,        /*hangingBaseline*/
> +                     -baselineAnchor,                                                        /*alphabeticBaseline*/
> +                     -fontMetrics.emDescent * kIdeographicBaselineDefault - baselineAnchor); /*ideographicBaseline*/

These lines all look awfully long, partly because they're indented so far. I'd probably try wrapping the initial assignment statement so as to pull everything back to the left somewhat:

  TextMetrics* metrics =
    new TextMetrics(totalWidth,
                    ...);

That might buy enough space to be able to include the comments (which are useful) without lines getting excessively long.

@@ +3943,5 @@
> +  // if only measuring, don't need to do any more work
> +  if (aOp == TextDrawOperation::MEASURE) {
> +    aError = NS_OK;
> +    return aMetrics;
> +  }

The whole allocation/initialization of the TextMetrics should go inside this conditional, actually. Because if the operation is FILL or STROKE, we do NOT want to allocate and return this object -- the callers would just leak it.

So, I think the code here should be more like

  if (aOp == TextDrawOperation::MEASURE) {
    aError = NS_OK;
    return new TextMetrics(...);
  }

and then the later exit points (for drawing operations) will just "return nullptr;" rather than a TextMetrics object.

@@ +3997,5 @@
> +                                        nsBidiPresUtils::MODE_DRAW,
> +                                        nullptr,
> +                                        0,
> +                                        nullptr,
> +                                        &bidiEngine);

Shouldn't we check aError for failure here, and return if it didn't succeed? I see the existing code doesn't check rv, but that's presumably just a bug.

::: dom/canvas/TextMetrics.h
@@ +14,5 @@
>  
>  class TextMetrics final : public NonRefcountedDOMObject
>  {
>  public:
> +  explicit TextMetrics(double aWidth, 

nit: trailing space

@@ +25,5 @@
> +                       double aEmHeightAscent,
> +                       double aEmHeightDescent,
> +                       double aHangingBaseline,
> +                       double aAlphabeticBaseline,
> +                       double aIdeographicBaseline) 

trailing space
Attachment #8788691 - Flags: review?(jfkthame)
Oops, I forgot to add a reference to the Gecko code style page[1], for guidance on things like naming, line length, wrapping conventions, etc.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
Assignee

Comment 17

3 years ago
Thanks Jonathan, that's useful. I should be able to finish it off this weekend.
Assignee

Comment 18

3 years ago
I had coke zero, pizza and a night alone, so done!

1. Given we are only returning a TextMetric on a measure operation, I have moved the Text metric definition (@@ +3925) inside the if block (@@ +3943) which means we don't need to delete it if we don't return it. 

2. Other places where we return a TextMetric, I've put an if (aOp == TextDrawOperation::MEASURE) around it

3. I've split the metric expressions into different variables as a way round the extremely long line length.

4. Added the missing return value check for Process text.

5. Fixed ideographic and hanging baseline in the switch statement.

6. Fixed the trailing spaces, pointer spacing and 
 line lengths

Think that's everything.
Attachment #8788691 - Attachment is obsolete: true
Attachment #8789955 - Flags: review?(jfkthame)
Sorry for the slow response here. Looking good, I think.... I took this patch and rebased it to apply on current mozilla-central trunk code (you must be working on a somewhat old checkout), and pushed to tryserver to see how things go. (I also removed the existing "fail" annotations for the DOM tests that will now pass because all the expected attributes are available.)

So here's how it went ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=f50434b66094

Two points this brings up:

(1) On Linux only, we're getting a bunch of cases of failures along the lines of 

TEST-UNEXPECTED-FAIL | /2dcontext/drawing-text-to-the-canvas/2d.text.measure.actualBoundingBoxAscent.basic.html | Canvas test: 2d.text.measure.actualBoundingBoxAscent.basic - assert_equals: ctx.measureText('A').actualBoundingBoxAscent === 37.5 (got 38[number], expected 37.5[number]) expected 37.5 but got 38

in the tests for the new attributes. Some of them are just off by a fraction of a pixel, which could easily just be a rounding issue that perhaps is ignorable; but others are off by several pixels (e.g. actualBoundingBoxAscent ... expected 28.4 +/- 0.2 but got 25.5). So I think we need to figure out why we're not getting the expected values there.

This may not really be a problem in this patch itself, but rather an issue in how our Linux font code derives the metrics that are then used as the basis for the TextMetrics values. But we need to understand what's happening here, so that we can either fix it now, or annotate the tests as failing and file an appropriate followup bug to address the underlying issue.

Richard, if you're able to look into this at all, that would be great; otherwise, I'll try to investigate further, but it may be some time before I get to it.


(2) TEST-UNEXPECTED-PASS | /2dcontext/text-styles/2d.text.draw.baseline.hanging.html

This is an existing test that no longer fails, due to the change in how the baseline is computed. So we can just remove the "fail" annotation and it'll be happy. (Though only because our default for the hanging baseline now matches what the test expects, not because we really support reading the hanging baseline from the font!)

However, this exposes a shortcoming of both the existing drawing tests and the new TextMetrics api tests. To fully implement the various textBaseline values, we should be relying on the OpenType 'BASE' table (when available), and only using fixed ratios as a fallback for fonts that don't provide the data. But the tests don't check this. We could pass all the tests by tweaking our fixed ratios to match what the tests expect, but that's cheating... really, there should be a couple of test fonts with very _different_ baselines defined, to check that implementations respect the specific font's values.

(That can be a followup, we don't need to deal with it here and now; I just wanted to note it before I forget.)
For reference, here's the rebased patch that applies to current mozilla-central; I hope I didn't break anything in the process.
Assignee

Comment 21

3 years ago
That's Jonathan, I've just set up a Linux box, so I'll continue to look at it and let you know what I find
Assign to Richard since he is implementing. Thanks for the patch!
Assignee: nobody → richard
Assignee

Comment 23

3 years ago
I've looked into this - the issue is that linux uses different opentype metrics for ascender/descender in certain cases than DirectWrite(Windows) and OS x. 

Windows and OS X
----------------
if use typographic metric flag is set in os2.fSelection:
 emAscent = os2.sTypoAscender
 emDescent = -os2.sTypoDescender
else
 emAscent = hhead.ascender / (hhead.ascender + hhead.descender) * emHeight
 emDescent = -hhead.descender / (hhead.ascender + hhead.descender) * emHeight

Linux
-----
if (os2.sTypoAscender)
 emAscent = os2.sTypoAscender
 emDescent = -os2.sTypoDescender
...

Judging from the opentype documentation, it looks like the windows behaviour is correct. I've verified that this is how directwrite on windows behaves, alas I haven't had time to set up a test on os x. It's easy to change if required, and I'm happy to do it, but should probably do as part of a new bug and should definitely be run past someone with more knowledge of font rendering than me.

There is also 1 px actual bounding box difference between linux and the other platforms - it's not a measuring difference as it's precisely 1 px at all font sizes, but I don't think that is particularly important, and happy to amend the tests to take that into account.
ni for some feedback on comment 23.
Flags: needinfo?(jfkthame)
One other thought is that the new parts should perhaps be behind a preference (maybe even initially landing with the pref off).  There are a bunch of sites from which we've heard feedback that *might* be addressed by this API, and it would be good to be able to show them something and find out if it actually works, before we ship it and are stuck with it.

Comment 26

a year ago
Bump, would really like to see this implemented.  Trying to get decent text rendering in webgl(especially things like font icons and arabic) is very difficult without such apis
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [parity-blink][DocArea=Canvas] → [DocArea=Canvas]
You need to log in before you can comment on or make changes to this bug.