1102584 - Implement extended TextMetrics object

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, task)

Description

[Florian Scholz (Open Web Docs)]

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

Comment 1

[John Daggett (:jtd)]

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.

Comment 2

[Richard Matheson]

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

Comment 3

[Richard Matheson]

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

Comment 4

[Richard Matheson]

Attachment: Implement extended TextMetrics object

Comment 5

[Jonathan Kew [: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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Timothy Nikkel (:tnikkel)]

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?

Comment 9

[Richard Matheson]

(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?

Comment 10

[Jonathan Kew [:jfkthame]]

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!

Comment 11

[Richard Matheson]

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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

And no need to re-ask review from me if the .webidl part stays the same :)
Just ask review from :jfkthame

Comment 13

[Jonathan Kew [: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.

Comment 14

[Richard Matheson]

Attachment: TextMetrics.diff

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.

Comment 15

[Jonathan Kew [: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

Comment 16

[Jonathan Kew [: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

Comment 17

[Richard Matheson]

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

Comment 18

[Richard Matheson]

Attachment: TextMetrics.diff

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.

Comment 19

[Jonathan Kew [: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.)

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: Implement the missing attributes for the TextMetrics object

For reference, here's the rebased patch that applies to current mozilla-central; I hope I didn't break anything in the process.

Comment 21

[Richard Matheson]

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

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

Assign to Richard since he is implementing. Thanks for the patch!

Comment 23

[Richard Matheson]

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.

Comment 24

[Timothy Nikkel (:tnikkel)]

ni for some feedback on comment 23.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[adrian]

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

Comment 27

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 28

[Fernando Serboncini]

Updated spec on this: https://github.com/whatwg/html/pull/3931

Comment 29

[Jeff Muizelaar [:jrmuizel]]

It looks like Chrome is going to ship this 77 so it might be worth trying to get done.

Comment 30

[Fernando Serboncini]

Btw,
Chrome has only shipped actualBoundingBox*, which I think sidesteps most of the issues brought up here.

Comment 31

[Jonathan Kew [:jfkthame]]

I guess we need to look at the latest spec, and at what Chrome is shipping, and if we're happy enough with it then update the patch here to align with them. We may still run into issue because of using different font metrics (for the same font) on different platform back-ends.

Are there web-platform-tests that we can rely on to check behavior here?

Comment 32

[Eyal]

Hi,
I am part of the Excel Online group in Microsoft, We are using this API to measure the text width and height, Currently FireFox is one of the only supported browser which did not implement it, since Edge is now chromium, and Safari also supported bounding box (left, right, height, width).
Unfortunately we might have to instruct customers to switch to other browsers to open office online until this issue is fixed.

Is it possible you will ship only the bounding box part of the API?
What would be the timeline?

Thanks,
Eyal.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Keeping an eye on this issue as it's recently active.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

With Fx73 going to Beta next week, this is going to have to go out with Fx74 at the earliest.

Comment 35

[Jonathan Kew [:jfkthame]]

I've been looking into updating the patch here, with a view to possibly shipping the actualBoundingBox attributes; current try run is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a62c5c847dcd0fe075849923e3f3c883f2d4f062.

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to Eyal from comment #32)

Hi,
I am part of the Excel Online group in Microsoft, We are using this API to measure the text width and height, Currently FireFox is one of the only supported browser which did not implement it, since Edge is now chromium, and Safari also supported bounding box (left, right, height, width).
Unfortunately we might have to instruct customers to switch to other browsers to open office online until this issue is fixed.

Is it possible you will ship only the bounding box part of the API?

Eyal, could you please confirm if it is specifically the four actualBoundingBox* attributes that you're looking for (the ones mentioned by Fernando in comment 30), or do you also need the emHeight* and/or fontBoundingBox* values?

It's unclear to me exactly what the state of standardization and implementation is here... I see these additional attributes in Chrome, which doesn't seem to match comment 30 above, and I also see an advances array, which IIRC was quite controversial when it was discussed; meanwhile in Safari I see several *Baseline attributes (but no advances).

If we implement just the actualBoundingBox* attributes, which seem to present the fewest standardization/compatibility issues, would this address your use case adequately?

Comment 37

[Eyal]

(In reply to Jonathan Kew (:jfkthame) from comment #36)

(In reply to Eyal from comment #32)

Hi,
I am part of the Excel Online group in Microsoft, We are using this API to measure the text width and height, Currently FireFox is one of the only supported browser which did not implement it, since Edge is now chromium, and Safari also supported bounding box (left, right, height, width).
Unfortunately we might have to instruct customers to switch to other browsers to open office online until this issue is fixed.

Is it possible you will ship only the bounding box part of the API?

Eyal, could you please confirm if it is specifically the four actualBoundingBox* attributes that you're looking for (the ones mentioned by Fernando in comment 30), or do you also need the emHeight* and/or fontBoundingBox* values?

It's unclear to me exactly what the state of standardization and implementation is here... I see these additional attributes in Chrome, which doesn't seem to match comment 30 above, and I also see an advances array, which IIRC was quite controversial when it was discussed; meanwhile in Safari I see several *Baseline attributes (but no advances).

If we implement just the actualBoundingBox* attributes, which seem to present the fewest standardization/compatibility issues, would this address your use case adequately?

Yes, We need the actual bounding box to measure the text width and height.

We also need the baseline to draw an underline,
We need the line gap (leading) for multiple lines of text in a cell (wrap text),
But the last 2 are not provided yet by chrome as well.
If you could add them it would be great,
Thanks,
Eyal.

Comment 38

[Jonathan Kew [:jfkthame]]

(In reply to Eyal from comment #37)

We also need the baseline to draw an underline,
We need the line gap (leading) for multiple lines of text in a cell (wrap text),
But the last 2 are not provided yet by chrome as well.

Looking at the current TextMetrics spec, I don't see any attributes relevant to leading. So if you want some line-spacing/leading metrics to be exposed, I think this needs to be brought up as a spec discussion first.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1102584 - patch 1 - Provide somewhat better defaults for hanging and ideographic baseline alignment of canvas text. r=lsalzman

This is extracted from the original patch by Richard Matheson; it's not strictly necessary
to implementing the additional TextMetrics attributes, but OTOH if we're going to potentially
expose baseline attributes, it seems sensible to make them somewhat more meaningful than the
fallbacks in the current code.

Eventually we should use baseline tables from the font, but (a) we don't have support for
that in the back-end font code, and (b) very few fonts provide the data anyway, so while this
isn't perfect, for now it's the best we can do.

Comment 40

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1102584 - patch 2 - Implement extended attributes of the TextMetrics object (preffed off by default). r=lsalzman,bzbarsky

This is the main work of Richard Matheson's original patch, updated to current trunk code
and with the new attributes put behind prefs. Because some of the attributes may be more
stable than others (there was a move by Google to change how baselines are represented,
but then this was retracted because Safari is already shipping per the existing spec; and
we have some differences in how we handle font metrics between platforms which may affect
the font ascent/descent values), I've split this into several prefs so that we have the
possibility of enabling just the more stable (and/or more urgently requested) attributes.

(Note that this echos Google's approach per comment 30 of initially shipping part of the API.)

Depends on D59678

Comment 41

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1102584 - patch 3 - Enable support for the actualBoundingBox* attributes of TextMetrics. r=lsalzman

This is unaffected by the differences in font metrics between platform back-ends,
so should be safe to ship without significant risk that it'll be unstable or need
to be revised in future.

Depends on D59679

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For what it's worth, I filed some github issues on the TextMetrics spec when reviewing it on behalf of the TAG.

Comment 43

[Jonathan Kew [:jfkthame]]

Yeah, there are questions regarding font metrics and baselines that I think need clarification (and probably more work in the implementation...). The actualBoundingBox metrics seem less troublesome, AFAICT.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59069d4514d0
patch 1 - Provide somewhat better defaults for hanging and ideographic baseline alignment of canvas text. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/967170ab891c
patch 2 - Implement extended attributes of the TextMetrics object (preffed off by default). r=lsalzman,bzbarsky
https://hg.mozilla.org/integration/autoland/rev/947829830dd5
patch 3 - Enable support for the actualBoundingBox* attributes of TextMetrics. r=lsalzman

Comment 45

[Andreea Pavel [:apavel]]

Backed out for failing wpt on OSX at 2d.text.measure.actualBoundingBox.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=285502009&resultStatus=testfailed%2Cbusted%2Cexception&revision=c81d3e9ee1581b9765f9c3ea998dd250364bc969

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285502009&repo=autoland&lineNumber=918

Backout: https://hg.mozilla.org/integration/autoland/rev/b3d7e55aed9b36bbaff5c16000bc769d867341f0

Comment 46

[Jonathan Kew [:jfkthame]]

Yeah, I saw this failed a test and was backed out - will be looking into it.

Comment 47

[Jonathan Kew [:jfkthame]]

This failed the test for the actualBoundingBoxLeft attribute because although the glyph in the test font nominally begins at the origin (and so the test expects actualBoundingBoxLeft == 0), we actually return -1 because the bounding box includes potential antialiasing-affected pixels.

As a comment in the test file already says, "Different platforms may render text slightly different", so I think we should just adjust the test to accept a close-to-zero value rather than requiring exactly zero here.

Comment 48

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1102584 - Allow a little variation in TextMetrics.actualBoundingBox test to account for rendering differences. r=lsalzman

Comment 49

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21273 for changes under testing/web-platform/tests

Comment 50

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/21273
* Community-TC (pull_request) (https://community-tc.services.mozilla.com/tasks/groups/N_aSwKXdSsShbpkGXtnemQ)

Comment 51

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1102584 followup: adjust tests2dtext.yaml to match changes to the generated test file.

Comment 52

[Jonathan Kew [:jfkthame]]

Argh, upstream checks complain if we don't update the original yaml file properly. This should fix things.

Comment 53

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 54

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/28a9c958e194
https://hg.mozilla.org/mozilla-central/rev/17f161af2406
https://hg.mozilla.org/mozilla-central/rev/a59075257290
https://hg.mozilla.org/mozilla-central/rev/e3be392e8132

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e6ef0abad23
followup: adjust tests2dtext.yaml to match changes to the generated test file. r=lsalzman

Comment 56

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4e6ef0abad23

Comment 57

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 58

[Pascal Chevrel:pascalc]

Hi Eyal,

Could you test our nightly builds and confirm that the change implemented in this ticket fixes the issue for the Excel team?

Thanks
Pascal
https://nightly.mozilla.org

Comment 59

[Eyal]

(In reply to Jonathan Kew (:jfkthame) from comment #36)

(In reply to Eyal from comment #32)

Hi,
I am part of the Excel Online group in Microsoft, We are using this API to measure the text width and height, Currently FireFox is one of the only supported browser which did not implement it, since Edge is now chromium, and Safari also supported bounding box (left, right, height, width).
Unfortunately we might have to instruct customers to switch to other browsers to open office online until this issue is fixed.

Is it possible you will ship only the bounding box part of the API?

Eyal, could you please confirm if it is specifically the four actualBoundingBox* attributes that you're looking for (the ones mentioned by Fernando in comment 30), or do you also need the emHeight* and/or fontBoundingBox* values?

It's unclear to me exactly what the state of standardization and implementation is here... I see these additional attributes in Chrome, which doesn't seem to match comment 30 above, and I also see an advances array, which IIRC was quite controversial when it was discussed; meanwhile in Safari I see several *Baseline attributes (but no advances).

If we implement just the actualBoundingBox* attributes, which seem to present the fewest standardization/compatibility issues, would this address your use case adequately?

Hi,
First of all I would like to start with a big thank you! It's amazing to see how responsive you are, which really helps building better apps and better browsers :)

I have checked it and it works very similar to chrome which suits our basics needs.
We also need the fontBoundingBox (currently we get it from the height of a div element with the requested font), chromium have it under a flag.
Is it possible to support it as well?
Thanks,
Eyal.

Comment 60

[Jonathan Kew [:jfkthame]]

We also have the fontBoundingBox metrics available behind a flag; you can try setting dom.textMetrics.fontBoundingBox.enabled to true in about:config, and experiment with it.

I'm not sure how portable/compatible this will be, as there are various ways of reading font metrics and they may give different results, so would be interested in any feedback.

Comment 61

[Florian Scholz (Open Web Docs)]

Updated/created docs:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/74#Canvas_and_WebGL

https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/actualBoundingBoxAscent
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/actualBoundingBoxDescent
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/actualBoundingBoxLeft
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/actualBoundingBoxRight
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/alphabeticBaseline
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/emHeightAscent
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/emHeightDescent
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/fontBoundingBoxAscent
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/fontBoundingBoxDescent
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/hangingBaseline
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/ideographicBaseline

Comment 62

[Calixte Denizet (:calixte)]

:jfkthame, is there any plan to set the pref to true by default ?
According to MDN, Firefox is the only browser to not support this feature:
https://developer.mozilla.org/en-US/docs/Web/API/TextMetrics/fontBoundingBoxAscent

Comment 63

[Jonathan Kew [:jfkthame]]

We could enable it, I guess, but my concern is that the results are not actually interoperable across browsers, so it's unclear how useful it would really be until that is resolved. See for example the comment at https://github.com/web-platform-tests/interop/issues/159#issuecomment-1303361603.

Comment 64

[Calixte Denizet (:calixte)]

In the pdf.js context, I want to use these properties to modify the top of the spans containing the text in the text layer in order to perfectly align them and the text drawn on the canvas.
Could we at least just enable the feature for pdf.js (we already did that in the past for other features, e.g. https://hg.mozilla.org/mozilla-central/rev/0ae8b1a8e1e9) ?

Comment 65

[Jonathan Kew [:jfkthame]]

I don't see a convenient way to do that for these attributes. Maybe we should start by enabling them for Nightly/early-Beta (or Nightly and Dev Edition?), and see what happens. I'm also curious to know how well they work for your use case, as I'm not sure exactly how the canvas metrics will relate to HTML text layout.... it may still be difficult to achieve entirely consistent results.

My suggestion: file a new bug requesting them to be enabled for pre-release channels, or something like that; and then if things go well, we can consider going all the way to release.