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
<rdar://problem/11159332>
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 ?
Created attachment 185598 [details] updated patch
Created attachment 185639 [details] updated patch: actualBoundingBoxRight/Left was not correct
Created attachment 186902 [details] patch proposal
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 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 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
Created attachment 187128 [details] fix chromium test failure
Created attachment 187137 [details] patch updated against tot
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>
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 ?
Created attachment 315905 [details] Patch Just rebased
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
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 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
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 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
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 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
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
Ah, yes! Fixing this bug would be awesome!!! 👍👍
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
Created attachment 316070 [details] Patch
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 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.
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
Created attachment 316345 [details] manual test case
There is another spec: Font Metrics API Level 1 https://drafts.css-houdini.org/font-metrics-api-1/
That Houdini spec is not stable. We should follow the WhatWG spec.
(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 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 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.
Created attachment 316422 [details] Patch
Comment on attachment 316422 [details] Patch Clearing flags on attachment: 316422 Committed r219970: <http://trac.webkit.org/changeset/219970>
All reviewed patches have been landed. Closing bug.