Bug 82798 - Implement new TextMetrics, returned by canvas measureText()
Summary: Implement new TextMetrics, returned by canvas measureText()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on: 109067 109068 109066 109069 109070
Blocks: 157629
  Show dependency treegraph
 
Reported: 2012-03-30 15:58 PDT by Dean Jackson
Modified: 2019-05-02 16:20 PDT (History)
27 users (show)

See Also:


Attachments
wip patch (13.82 KB, patch)
2013-01-30 14:44 PST, arno.
no flags Details | Formatted Diff | Diff
updated patch (13.83 KB, patch)
2013-01-30 15:40 PST, arno.
no flags Details | Formatted Diff | Diff
updated patch: actualBoundingBoxRight/Left was not correct (10.94 KB, patch)
2013-01-30 17:56 PST, arno.
no flags Details | Formatted Diff | Diff
patch proposal (21.64 KB, patch)
2013-02-06 12:53 PST, arno.
no flags Details | Formatted Diff | Diff
fix chromium test failure (21.72 KB, patch)
2013-02-07 09:43 PST, arno.
no flags Details | Formatted Diff | Diff
patch updated against tot (21.67 KB, patch)
2013-02-07 10:58 PST, arno.
no flags Details | Formatted Diff | Diff
Patch (20.73 KB, patch)
2017-07-19 00:03 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.40 MB, application/zip)
2017-07-19 00:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.56 MB, application/zip)
2017-07-19 00:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (7.73 MB, application/zip)
2017-07-19 01:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (2.20 MB, application/zip)
2017-07-19 01:15 PDT, Build Bot
no flags Details
Patch (19.67 KB, patch)
2017-07-20 22:40 PDT, Fujii Hironori
dino: review+
Details | Formatted Diff | Diff
Patch (22.50 KB, patch)
2017-07-24 00:47 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
manual test case (4.00 KB, text/html)
2017-07-24 20:40 PDT, Fujii Hironori
no flags Details
Patch (60.60 KB, patch)
2017-07-25 19:19 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2012-03-30 15:58:05 PDT
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
Comment 1 Radar WebKit Bug Importer 2012-03-30 15:58:25 PDT
<rdar://problem/11159332>
Comment 2 arno. 2013-01-30 14:44:50 PST
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 ?
Comment 3 arno. 2013-01-30 15:40:42 PST
Created attachment 185598 [details]
updated patch
Comment 4 arno. 2013-01-30 17:56:02 PST
Created attachment 185639 [details]
updated patch: actualBoundingBoxRight/Left was not correct
Comment 5 arno. 2013-02-06 12:53:32 PST
Created attachment 186902 [details]
patch proposal
Comment 6 Adam Barth 2013-02-06 13:24:29 PST
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?
Comment 7 Adam Barth 2013-02-06 13:25:25 PST
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.
Comment 8 WebKit Review Bot 2013-02-06 14:19:06 PST
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
Comment 9 arno. 2013-02-07 09:43:32 PST
Created attachment 187128 [details]
fix chromium test failure
Comment 10 arno. 2013-02-07 10:58:54 PST
Created attachment 187137 [details]
patch updated against tot
Comment 11 Alexandru Chiculita 2013-04-29 23:16:01 PDT
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>
Comment 12 arno. 2013-05-01 13:05:25 PDT
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 ?
Comment 14 Fujii Hironori 2017-07-19 00:03:05 PDT
Created attachment 315905 [details]
Patch

Just rebased
Comment 15 Build Bot 2017-07-19 00:39:13 PDT
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
Comment 16 Build Bot 2017-07-19 00:39:15 PDT
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
Comment 17 Build Bot 2017-07-19 00:44:18 PDT
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
Comment 18 Build Bot 2017-07-19 00:44:20 PDT
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
Comment 19 Build Bot 2017-07-19 01:10:49 PDT
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
Comment 20 Build Bot 2017-07-19 01:10:51 PDT
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
Comment 21 Build Bot 2017-07-19 01:15:21 PDT
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
Comment 22 Build Bot 2017-07-19 01:15:23 PDT
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
Comment 23 Myles C. Maxfield 2017-07-19 13:50:55 PDT
Ah, yes! Fixing this bug would be awesome!!!

👍👍
Comment 24 Fujii Hironori 2017-07-20 00:16:38 PDT
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
Comment 25 Fujii Hironori 2017-07-20 22:40:44 PDT
Created attachment 316070 [details]
Patch
Comment 26 Dean Jackson 2017-07-21 15:11:00 PDT
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()) ?
Comment 27 Fujii Hironori 2017-07-23 17:45:48 PDT
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.
Comment 28 Fujii Hironori 2017-07-24 00:47:07 PDT
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
Comment 29 Fujii Hironori 2017-07-24 20:40:40 PDT
Created attachment 316345 [details]
manual test case
Comment 30 Fujii Hironori 2017-07-24 21:22:12 PDT
There is another spec:

  Font Metrics API Level 1
  https://drafts.css-houdini.org/font-metrics-api-1/
Comment 31 Simon Fraser (smfr) 2017-07-24 21:38:46 PDT
That Houdini spec is not stable. We should follow the WhatWG spec.
Comment 32 Fujii Hironori 2017-07-25 00:53:07 PDT
(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
Comment 33 Dean Jackson 2017-07-25 16:21:45 PDT
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"?
Comment 34 Fujii Hironori 2017-07-25 19:15:52 PDT
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.
Comment 35 Fujii Hironori 2017-07-25 19:19:27 PDT
Created attachment 316422 [details]
Patch
Comment 36 WebKit Commit Bot 2017-07-26 16:53:30 PDT
Comment on attachment 316422 [details]
Patch

Clearing flags on attachment: 316422

Committed r219970: <http://trac.webkit.org/changeset/219970>
Comment 37 WebKit Commit Bot 2017-07-26 16:53:33 PDT
All reviewed patches have been landed.  Closing bug.