Bug 82798 – Implement new TextMetrics, returned by canvas measureText()

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Dean Jackson]

Canvas v5 has expanded the results given by measureText
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-measuretext

 - many more metrics from measureText()
     var metrics = context.measureText('Hello World');
     var bL = metrics.actualBoundingBoxLeft;
     var bR = metrics.actualBoundingBoxRight;
     var bU = metrics.actualBoundingBoxAscent;
     var bD = metrics.actualBoundingBoxDescent;
     context.fillStyle = 'black';
     context.fillRect(x-bL, y-bU, bL+bR, bU+bD);
     context.fillStyle = 'white';
     context.fillText(x, y, 'Hello World');
     // there are also values for all the baselines

[Radar WebKit Bug Importer]

<rdar://problem/11159332>

[arno.]

Created attachment 185584 [details]
wip patch

Here is a wip patch.
I encountered a problem:
If I understand correctly, we can use the glyphOverflow parameter (in Font::width) to get the actual font ascent and descent, and also the left and right overflow relative to the advance box.
But a lot of ports do not fill this structure in the complex path.
So, what is the best way to deal with that ?
Is is to ok to just add the test as skipped in all the platform which do not support glyphOverflow with complex path ?

[arno.]

Created attachment 185598 [details]
updated patch

[arno.]

Created attachment 185639 [details]
updated patch: actualBoundingBoxRight/Left was not correct

[arno.]

Created attachment 186902 [details]
patch proposal

[Adam Barth]

Comment on attachment 186902 [details]
patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=186902&action=review

> Source/WebCore/html/TextMetrics.idl:39
> +    readonly attribute float actualBoundingBoxLeft;
> +    readonly attribute float actualBoundingBoxRight;
> +    readonly attribute float fontBoundingBoxAscent;
> +    readonly attribute float fontBoundingBoxDescent;
> +    readonly attribute float actualBoundingBoxAscent;
> +    readonly attribute float actualBoundingBoxDescent;
> +    readonly attribute float emHeightAscent;
> +    readonly attribute float emHeightDescent;
> +    readonly attribute float hangingBaseline;
> +    readonly attribute float alphabeticBaseline;
> +    readonly attribute float ideographicBaseline;

Are these attributes in a spec?  Would you be willing to link to the spec in the ChangeLog?

[Adam Barth]

Comment on attachment 186902 [details]
patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=186902&action=review

>> Source/WebCore/html/TextMetrics.idl:39
>> +    readonly attribute float ideographicBaseline;
> 
> Are these attributes in a spec?  Would you be willing to link to the spec in the ChangeLog?

It looks like the spec is here:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#textmetrics

IMHO, we should link to the spec in the ChangeLog.

[WebKit Review Bot]

Comment on attachment 186902 [details]
patch proposal

Attachment 186902 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16397345

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-measureText-2.html

[arno.]

Created attachment 187128 [details]
fix chromium test failure

[arno.]

Created attachment 187137 [details]
patch updated against tot

[Alexandru Chiculita]

Comment on attachment 187137 [details]
patch updated against tot

View in context: https://bugs.webkit.org/attachment.cgi?id=187137&action=review

Thanks for working on this patch. I've been long looking for the text metrics on canvas. I have a few nits below.

Also, not sure if you need a runtime flag for this feature. Did you ask on the dev list about it? Also, exposing more of the text-metrics and "font" details might potentially introduce a new leak that might need to be investigated.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2160
> +    switch (state().m_textBaseline) {

Is the drawTextInternal doing the same alignment math? Can you extract a function from CanvasRenderingContext2D::drawTextInternal and reuse it here?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2177
> +    TextAlign align = physicalAlignment();

ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2417
> +    TextDirection direction = computedStyle ? computedStyle->direction() : LTR;

Looks like the spec now has a "direction" attribute on the context.

> LayoutTests/fast/canvas/canvas-measureText-2.html:9
> +    var texts = ['Some simple text', 'à½à½à½´à¼à½à½ºà½à¼']; // tibetan script triggers complex path

The spec has a nice visual representation of the baselines. I think this test is good, but might be better to also have a visual test with all the boxes/baselines. Is there a way to make it a ref-test? For example the ref-test might be using DIV blocks to compare that the baselines are consistent.

> LayoutTests/fast/canvas/canvas-measureText-2.html:31
> +                        testFailed('metrics.actualBoundingBoxLeft + metrics.actualBoundingBoxRight must be about the same as width')

nit: Might be useful to include the current values of the "for" statements (ie. text, baseline, alignment).

> LayoutTests/fast/canvas/canvas-measureText-2.html:40
> +                    if (baseline === 'top')
> +                        if (metrics.emHeightAscent !== 0)

nit: combine the nested ifs.

> LayoutTests/fast/canvas/canvas-measureText-2.html:88
> +        </script>

nit: <script> is not aligned with </script>

[arno.]

spec states

>  If doing these measurements requires using a font that has an origin that is not the same as that of the Document object that owns the canvas element (even if "using a font" means just checking if that font has a particular glyph in it before falling back to another font), then the method must throw a SecurityError exception.

Does anyone familiar with Font code known if it's possible to get the origin information from a Font object. Otherwise, what would it take to be able to retrieve that information ?

[Fujii Hironori]

Created attachment 315905 [details]
Patch

Just rebased

[Build Bot]

Comment on attachment 315905 [details]
Patch

Attachment 315905 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4146431

New failing tests:
fast/canvas/canvas-measureText-2.html

[Build Bot]

Created attachment 315909 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315905 [details]
Patch

Attachment 315905 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4146434

New failing tests:
fast/canvas/canvas-measureText-2.html

[Build Bot]

Created attachment 315910 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 315905 [details]
Patch

Attachment 315905 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4146547

New failing tests:
fast/canvas/canvas-measureText-2.html

[Build Bot]

Created attachment 315911 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 315905 [details]
Patch

Attachment 315905 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4146474

New failing tests:
fast/canvas/canvas-measureText-2.html

[Build Bot]

Created attachment 315912 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

Ah, yes! Fixing this bug would be awesome!!!

👍👍

[Fujii Hironori]

Chrome 59 implements it under a flag.

chrome://flags/#enable-experimental-canvas-features
https://crbug.com/277215

Firefox doesn't support yet.

https://bugzilla.mozilla.org/show_bug.cgi?id=1102584

[Fujii Hironori]

Created attachment 316070 [details]
Patch

[Dean Jackson]

Comment on attachment 316070 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316070&action=review

Looks good. Just needs a few small changes.

> Source/WebCore/html/TextMetrics.h:86
> +        , m_actualBoundingBoxLeft(0)
> +        , m_actualBoundingBoxRight(0)
> +        , m_fontBoundingBoxAscent(0)
> +        , m_fontBoundingBoxDescent(0)
> +        , m_actualBoundingBoxAscent(0)
> +        , m_actualBoundingBoxDescent(0)
> +        , m_emHeightAscent(0)
> +        , m_emHeightDescent(0)
> +        , m_hangingBaseline(0)
> +        , m_alphabeticBaseline(0)
> +        , m_ideographicBaseline(0)

Get rid of all these and....

> Source/WebCore/html/TextMetrics.h:100
> +    float m_actualBoundingBoxLeft;
> +    float m_actualBoundingBoxRight;
> +    float m_fontBoundingBoxAscent;
> +    float m_fontBoundingBoxDescent;
> +    float m_actualBoundingBoxAscent;
> +    float m_actualBoundingBoxDescent;
> +    float m_emHeightAscent;
> +    float m_emHeightDescent;
> +    float m_hangingBaseline;
> +    float m_alphabeticBaseline;
> +    float m_ideographicBaseline;

... put them here instead. e.g. float m_width { 0 };
then you won't need the TextMetrics constructor at all.

In fact, why not just make this a struct with public member variables.

class TextMetrics {
  float width { 0 };
  float actualBoundingBoxLeft { 0 };
...
}

> Source/WebCore/html/TextMetrics.idl:39
> +    readonly attribute unrestricted float actualBoundingBoxLeft;
> +    readonly attribute unrestricted float actualBoundingBoxRight;
> +    readonly attribute unrestricted float fontBoundingBoxAscent;
> +    readonly attribute unrestricted float fontBoundingBoxDescent;
> +    readonly attribute unrestricted float actualBoundingBoxAscent;
> +    readonly attribute unrestricted float actualBoundingBoxDescent;
> +    readonly attribute unrestricted float emHeightAscent;
> +    readonly attribute unrestricted float emHeightDescent;
> +    readonly attribute unrestricted float hangingBaseline;
> +    readonly attribute unrestricted float alphabeticBaseline;
> +    readonly attribute unrestricted float ideographicBaseline;

Oh. I guess that our bindings code always requires a getter/setter pair, rather than direct access into variables.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2335
> +    const FontProxy& font = fontProxy();

Use auto.

auto& font = fontProxy();

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2336
> +    const FontMetrics& fontMetrics = font.fontMetrics();

Ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2364
> +    TextAlign align = physicalAlignment();
> +
> +    float xoffset = 0;
> +    switch (align) {

You don't use align other than this, so just switch(physicalAlignment()) ?

[Fujii Hironori]

Comment on attachment 316070 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316070&action=review

Thank you for the review.

>> Source/WebCore/html/TextMetrics.idl:39
>> +    readonly attribute unrestricted float ideographicBaseline;
> 
> Oh. I guess that our bindings code always requires a getter/setter pair, rather than direct access into variables.

I think so. For example, it should have the getter actualBoundingBoxLeft(float value). All setters can be removed because generated binding code doesn't call setters for readonly attributes.

[Fujii Hironori]

Created attachment 316272 [details]
Patch

* Resolved review points
* New method textOffset() to calculate text offset for drawTextInternal() and measureText().
* Added a dummy implementation of calculating GlyphOverflow for HarfBuzz port

* Needs to fix the bug that actualBoundingBoxAscent and actualBoundingBoxDescent are zero in Windows port

[Fujii Hironori]

Created attachment 316345 [details]
manual test case

[Fujii Hironori]

There is another spec:

  Font Metrics API Level 1
  https://drafts.css-houdini.org/font-metrics-api-1/

[Simon Fraser (smfr)]

That Houdini spec is not stable. We should follow the WhatWG spec.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #28)
> * Needs to fix the bug that actualBoundingBoxAscent and
> actualBoundingBoxDescent are zero in Windows port

Filed.

  Bug 174813 – [WinCairo] Implement Font::platformBoundsForGlyph

AppleWin port looks nice.
https://photos.app.goo.gl/Knhlb4sTaw4DQLc63

[Dean Jackson]

Comment on attachment 316272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316272&action=review

Looks good. Just tiny changes (that I'm sorry I didn't notice last time) - I'll commit when a new patch is uploaded.

> LayoutTests/fast/canvas/canvas-measureText-2.html:31
> +                            if ((metrics.actualBoundingBoxLeft + metrics.actualBoundingBoxRight) < (metrics.width - 1) ||
> +                                (metrics.actualBoundingBoxLeft + metrics.actualBoundingBoxRight) > (metrics.width + 1))
> +                                testFailed('metrics.actualBoundingBoxLeft + metrics.actualBoundingBoxRight must be about the same as width' + condition)

I think for each of these it would be nice to also have PASS/testPassed results, so that we can confirm changes to the test and the expected results at the same time.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2384
> +        // Do nothing.

No need for this comment.

> Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:68
> +        // FIXME: Calculate.

What does this mean? Do you mean "Calculate the actual values rather than just the font's ascent and descent"?

[Fujii Hironori]

Comment on attachment 316272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316272&action=review

>> LayoutTests/fast/canvas/canvas-measureText-2.html:31
>> +                                testFailed('metrics.actualBoundingBoxLeft + metrics.actualBoundingBoxRight must be about the same as width' + condition)
> 
> I think for each of these it would be nice to also have PASS/testPassed results, so that we can confirm changes to the test and the expected results at the same time.

Agreed. I'll recreate a test case by using shouldBe*.

>> Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:68
>> +        // FIXME: Calculate.
> 
> What does this mean? Do you mean "Calculate the actual values rather than just the font's ascent and descent"?

That's right.

[Fujii Hironori]

Created attachment 316422 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316422 [details]
Patch

Clearing flags on attachment: 316422

Committed r219970: <http://trac.webkit.org/changeset/219970>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.