Bug 157629 - Implement more of the canvas TextMetrics API
Summary: Implement more of the canvas TextMetrics API
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, WebExposed
Depends on: 82798
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-12 10:49 PDT by Simon Fraser (smfr)
Modified: 2018-09-13 14:07 PDT (History)
10 users (show)

See Also:


Attachments
Screenshot 1 of the patch vs. chrome (1012.26 KB, image/png)
2018-09-13 11:14 PDT, Justin Michaud
no flags Details
Patch (210.28 KB, patch)
2018-09-13 11:21 PDT, Justin Michaud
mmaxfield: review-
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Manual test case (updated from last patch on this issue) (5.29 KB, text/html)
2018-09-13 11:22 PDT, Justin Michaud
no flags Details
Archive of layout-test-results from ews202 for win-future (12.89 MB, application/zip)
2018-09-13 14:03 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.60 MB, application/zip)
2018-09-13 14:06 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-05-12 10:49:10 PDT
We should implement more of https://html.spec.whatwg.org/multipage/scripting.html#textmetrics

We currently just have 'width'.
Comment 1 Fujii Hironori 2017-07-19 00:04:50 PDT
Arno's WIP patch is in Bug 82798.
Comment 2 Fernando Serboncini 2018-08-30 10:46:18 PDT
New spec: https://github.com/whatwg/html/pull/3931
Comment 3 Radar WebKit Bug Importer 2018-09-04 10:17:06 PDT
<rdar://problem/44101655>
Comment 4 Justin Michaud 2018-09-13 11:13:18 PDT
https://github.com/whatwg/html/pull/3931 adds advances, and moves the baselines into a dictionary

https://github.com/whatwg/html/issues/3995 has some more discussion about Safari's existing implementation

https://github.com/whatwg/html/issues/4026 has some interesting discussion about a number of edge cases. 

This patch adds advances and moves the baselines. When multiple code points are shaped into one glyph, it outputs the same "advance" for both (note that "advances" in the spec are actually distances from the left of the text box to the left of the glyph). If one code point becomes multiple glyphs, it breaks completely. If a code point does not get shaped into a glyph, it will have the same "advance" as the previous character, but I think it will break if the first character in the text does not become a glyph. I am going to need some help to understand how implement these edge cases.

With this patch, it passes http://w3c-test.org/2dcontext/drawing-text-to-the-canvas/2d.text.measure.advances.html (although you need to run it locally. I think it has something to do with the fonts). This test isn't very thorough though.

Chromium has this feature behind a flag. To enable it, run 
```
/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --enable-blink-features=ExtendedTextMetrics
```

Note that Chromium's implementation is broken for rtl text. 

I have attached a manual test case to play around with. I also attached a screenshot of it in action ("Screenshot 1").

One of the advances is missing in this implementation compared to chrome, as seen in the image. I am not sure what the correct behavior is.
Comment 5 Justin Michaud 2018-09-13 11:14:24 PDT
Created attachment 349683 [details]
Screenshot 1 of the patch vs. chrome
Comment 6 Justin Michaud 2018-09-13 11:21:38 PDT
Created attachment 349684 [details]
Patch
Comment 7 Justin Michaud 2018-09-13 11:22:29 PDT
Created attachment 349685 [details]
Manual test case (updated from last patch on this issue)
Comment 8 EWS Watchlist 2018-09-13 11:23:41 PDT
Attachment 349684 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:12:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Justin Michaud 2018-09-13 11:24:46 PDT
I think there should be a separate issue for handling baselines correctly. Right now, we don't consider the font at all. You can see this in the screenshot above as well.
Comment 10 EWS Watchlist 2018-09-13 14:03:14 PDT
Comment on attachment 349684 [details]
Patch

Attachment 349684 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9205634

New failing tests:
imported/blink/fast/canvas/canvas-measure-text-rtl.html
imported/blink/fast/canvas/canvas-normalize-string.html
imported/blink/fast/canvas/canvas-measure-bidi-text.html
fast/canvas/canvas-measureText-2.html
Comment 11 EWS Watchlist 2018-09-13 14:03:26 PDT
Created attachment 349699 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 12 EWS Watchlist 2018-09-13 14:06:19 PDT
Comment on attachment 349684 [details]
Patch

Attachment 349684 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9205557

New failing tests:
imported/w3c/web-platform-tests/workers/semantics/interface-objects/002.worker.html
Comment 13 EWS Watchlist 2018-09-13 14:06:21 PDT
Created attachment 349702 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 14 Myles C. Maxfield 2018-09-13 14:07:24 PDT
Comment on attachment 349684 [details]
Patch

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

I'm not sure this API is mature enough to implement. I don't quite understand how other browsers have implemented it given how many standardization discussions we're having about it.

> Source/WebCore/ChangeLog:12
> +        No new tests (OOPS!).

Please update this to say the name of the test that you updated.

> Source/WebCore/platform/graphics/FontCascade.cpp:294
> +    advances.append(glyphBuffer.initialAdvance().width());

The Web API right now should probably ignore the initial advance. https://github.com/whatwg/html/issues/4030#issuecomment-420945138

> Source/WebCore/platform/graphics/FontCascade.cpp:329
> +    while (nextGlyph < glyphBuffer.size()) {
> +        const Font* nextFontData = glyphBuffer.fontAt(nextGlyph);
> +
> +        if (nextFontData != fontData) {
> +            fontData = nextFontData;
> +            currentOffset = lastOffset + 1;
> +        }
> +
> +        lastOffset = glyphBuffer.offsetInString(nextGlyph) + currentOffset;
> +        offsets.append(lastOffset);
> +        nextGlyph++;
> +    }

We have logic in ComplexTextController::offsetForPosition() that does this. We should investigate pulling that code out into a shared function that we can call here instead.

Specifically, look for // FIXME: Instead of dividing the glyph's advance equally between the characters, this
Comment 15 Myles C. Maxfield 2018-09-13 14:07:53 PDT
I think this API is too nascent to implement right now.