Closed Bug 1102584 Opened 10 years ago Closed 5 years ago

Implement extended TextMetrics object

Categories

(Core :: Graphics: Canvas2D, task)

Product:
Core
Component:
Graphics: Canvas2D
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla74
Tracking Flags:
Tracking Status
firefox73 - wontfix
firefox74 + fixed

People

(Reporter: fs, Assigned: richard)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, parity-chrome, Whiteboard: [DocArea=Canvas][layout:backlog], [wptsync upstream])

Attachments

(7 files, 2 obsolete files)

TextMetrics.diff
(deleted), patch
Details | Diff | Splinter Review
Implement the missing attributes for the TextMetrics object
(deleted), patch
Details | Diff | Splinter Review
Bug 1102584 - patch 1 - Provide somewhat better defaults for hanging and ideographic baseline alignment of canvas text. r=lsalzman
(deleted), text/x-phabricator-request
Details
Bug 1102584 - patch 2 - Implement extended attributes of the TextMetrics object (preffed off by default). r=lsalzman,bzbarsky
(deleted), text/x-phabricator-request
Details
Bug 1102584 - patch 3 - Enable support for the actualBoundingBox* attributes of TextMetrics. r=lsalzman
(deleted), text/x-phabricator-request
Details
Bug 1102584 - Allow a little variation in TextMetrics.actualBoundingBox test to account for rendering differences. r=lsalzman
(deleted), text/x-phabricator-request
Details
Bug 1102584 followup: adjust tests2dtext.yaml to match changes to the generated test file.
(deleted), text/x-phabricator-request
Details
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
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.
I'm looking to implement the metrics. Anything I need to be aware of/anything I need to take into account?
I'll be more specific as this is my first time working on firefox - should I put it behind a flag?
Attached patch Implement extended TextMetrics object (obsolete) (deleted) — — Splinter Review
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)
(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!
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.
Attachment #8739746 - Attachment is obsolete: true
Attached patch TextMetrics.diff (obsolete) (deleted) — — 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
Thanks Jonathan, that's useful. I should be able to finish it off this weekend.
Attached patch TextMetrics.diff (deleted) — — Splinter Review
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.
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
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.
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]

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

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

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?

Flags: needinfo?(jfkthame)

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.

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

status-firefox73: --- → affected
tracking-firefox73: --- → ?
Type: defect → task

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

status-firefox73: affected → wontfix
tracking-firefox73: ? → -

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.

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

Flags: needinfo?(eyalg)
Whiteboard: [DocArea=Canvas] → [DocArea=Canvas][layout:backlog]

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

Flags: needinfo?(eyalg)

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

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.

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

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

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

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.

Pushed by jkew@mozilla.com: 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
Flags: needinfo?(richard) → needinfo?(jfkthame)

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

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.

Flags: needinfo?(jfkthame)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21273 for changes under testing/web-platform/tests
Whiteboard: [DocArea=Canvas][layout:backlog] → [DocArea=Canvas][layout:backlog], [wptsync upstream]
Whiteboard: [DocArea=Canvas][layout:backlog], [wptsync upstream] → [DocArea=Canvas][layout:backlog], [wptsync upstream error]

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

Flags: needinfo?(jkew)
Attachment #9120421 - Attachment description: Bug 1102584 - patch 2 - Implement extended attributes of the TextMetrics object (preffed off by default). r=lsalzman → Bug 1102584 - patch 2 - Implement extended attributes of the TextMetrics object (preffed off by default). r=lsalzman,bzbarsky
Flags: needinfo?(jkew)
Whiteboard: [DocArea=Canvas][layout:backlog], [wptsync upstream error] → [DocArea=Canvas][layout:backlog], [wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Flags: needinfo?(jkew)
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e6ef0abad23 followup: adjust tests2dtext.yaml to match changes to the generated test file. r=lsalzman
Regressions: 1610432
Upstream PR merged by moz-wptsync-bot

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

tracking-firefox74: --- → +
Flags: needinfo?(eyalg)

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

Flags: needinfo?(eyalg)

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.

: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

Flags: needinfo?(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.

Flags: needinfo?(jfkthame)

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

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.

Attachment #8789955 - Flags: review?(jfkthame)
Blocks: 1801198
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: