Font metrics, version 1

Font metrics, version 1

Spec : http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-textmetrics-width

BUG=277215
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168737

[sugoi1, 2014-01-28 21:24:41]

Hi bungeman,

  I need some help from a font expert with this cl. I wrote a first prototype of
the spec that can be found here :

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...

  The code is simple, but the spec doesn't seem to have any associated math to
explain each new TextMetrics parameter, so I'm unsure how to know if I got it
right and how to test if I got it right. I'm aware that what I wrote is most
likely wrong in some cases, but I'm now familiar with the Font code in Blink and
I'm having a hard time figuring out which parts are equivalent to what's
mentioned in the spec.

  Any help understanding this would be appreciated. Thanks.

[bungeman-skia, 2014-01-28 22:17:06]

I'll take a more in-depth look later, but in general nothing here stands out as
obviously wrong. It really looks like this is the sort of thing that needs a
layout test which draws all of these boxes and lines around and on some sample
text onto a canvas with various settings to make sure that everything is going
in the right direction and that things don't change in the future.

Note that font metrics are madness. The metrics here seem like they might be
stable though.

[sugoi1, 2014-01-31 21:56:04]

https://codereview.chromium.org/141433026/diff/20001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/141433026/diff/20001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2329: return
(fontMetrics.ascent() * 4) / 5;
I modified the hanging baseline from "ascent" to "0.8 * ascent", in accordance
with the standard.

[eseidel, 2014-01-31 22:03:30]

I'm glad we're looking at exposing more measurement primitives.

https://codereview.chromium.org/141433026/diff/20001/Source/core/html/TextMet...
File Source/core/html/TextMetrics.h (right):

https://codereview.chromium.org/141433026/diff/20001/Source/core/html/TextMet...
Source/core/html/TextMetrics.h:42: float actualBoundingBoxLeft() const { return
m_actualBoundingBoxLeft; }
So strange.  Don't we have a more sophisitcated object to return instead of
having 4 versions of the same method just for left right bottom top?

[eseidel, 2014-01-31 22:03:58]

Could you please update the CL descriptiong to include information about what
bug is tracking this work, where I might find the spec, etc?

[Rik, 2014-01-31 22:08:05]

On 2014/01/28 21:24:41, sugoi1 wrote:
> Hi bungeman,
> 
>   I need some help from a font expert with this cl. I wrote a first prototype
of
> the spec that can be found here :
> 
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...
> 

Chrome will be the first one to implement these APIs.
At the time these were proposed, people voiced concerns that this is not really
what is needed. 
Someone at the CSS F2F also mentioned that these APIs should be in a different
location since they are useful for non-canvas purposes.

[sugoi1, 2014-02-03 16:02:50]

On 2014/01/31 22:08:05, Rik wrote:
> On 2014/01/28 21:24:41, sugoi1 wrote:
> > Hi bungeman,
> > 
> >   I need some help from a font expert with this cl. I wrote a first
prototype of
> > the spec that can be found here :
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...
> 
> Chrome will be the first one to implement these APIs.
> At the time these were proposed, people voiced concerns that this is not
really what is needed. 
> Someone at the CSS F2F also mentioned that these APIs should be in a different
> location since they are useful for non-canvas purposes.

Ok... so what does this mean? After this quick prototype, I wanted to add my
"Intent to implement" on this feature (which would most likely spark more
discussion). Do you advise I proceed differently?

[sugoi1, 2014-02-13 21:46:19]

On 2014/02/03 16:02:50, sugoi1 wrote:
> On 2014/01/31 22:08:05, Rik wrote:
> > On 2014/01/28 21:24:41, sugoi1 wrote:
> > > Hi bungeman,
> > > 
> > >   I need some help from a font expert with this cl. I wrote a first
> prototype of
> > > the spec that can be found here :
> > >
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...
> > 
> > Chrome will be the first one to implement these APIs.
> > At the time these were proposed, people voiced concerns that this is not
> really what is needed. 
> > Someone at the CSS F2F also mentioned that these APIs should be in a
different
> > location since they are useful for non-canvas purposes.
> 
> Ok... so what does this mean? After this quick prototype, I wanted to add my
> "Intent to implement" on this feature (which would most likely spark more
> discussion). Do you advise I proceed differently?

Updated to ToT. If there's no viable alternative proposed other than
implementing the spec, I'll have to move forward with this on whatwg to see if
other parties have an opinion on the matter.

[eseidel, 2014-02-14 11:31:17]

This needs an intent to ship, or it needs to be behind a runtime flag.  not
lgtm.

Do we have feedback from a customer wanting to use this?  Would this allow Adobe
to make their regions polyfill better for instance?

[sugoi, 2014-02-14 13:16:10]

On 2014/02/14 11:31:17, eseidel wrote:
> This needs an intent to ship, or it needs to be behind a runtime flag.  not
lgtm.
> 
> Do we have feedback from a customer wanting to use this?  Would this allow
Adobe to make their regions polyfill better for instance?

No worries, I have no intention of committing this code until everyone agrees
that this is the way to go. I just wanted to know if I had to do something other
than flagging an "intent to implement and ship". I wasn't clear on what was my
next step following Rik's comment.

I'm not sure who originally requested this feature, this task was delegated to
me from junov@, so he might know more, I'll ask him for more info.

[sugoi1, 2014-02-14 15:01:25]

On 2014/02/14 13:16:10, sugoi wrote:
> On 2014/02/14 11:31:17, eseidel wrote:
> > This needs an intent to ship, or it needs to be behind a runtime flag.  not
> lgtm.
> > 
> > Do we have feedback from a customer wanting to use this?  Would this allow
> Adobe to make their regions polyfill better for instance?
> 
> No worries, I have no intention of committing this code until everyone agrees
> that this is the way to go. I just wanted to know if I had to do something
other
> than flagging an "intent to implement and ship". I wasn't clear on what was my
> next step following Rik's comment.
> 
> I'm not sure who originally requested this feature, this task was delegated to
> me from junov@, so he might know more, I'll ask him for more info.

So, there were 2 reasons to implement this originally:
1) It was a request from the Ritz team. They ended up using a workaround to
achieve their goal since the font metrics weren't available.
2) It was discussed (on whatwg, I believe) that this could be implemented
instead of text decorations, so that users could have the flexibility to create
any text decoration they wanted.

[eseidel, 2014-02-18 18:20:13]

I think we should totally implement this, we should just add these behind the
--experimental-web-platform features flag (just add a RuntimeEnabledFeature with
status=experimental) and get confirmation from customers that this is the right
API before being the first to ship this.
http://www.chromium.org/blink/runtime-enabled-features

If other vendors are already shipping this of course that's a different story.

Once we ship to the web, it's much more difficult to unship.

[sugoi, 2014-02-18 18:29:37]

On 2014/02/18 18:20:13, eseidel wrote:
> I think we should totally implement this, we should just add these behind the
> --experimental-web-platform features flag (just add a RuntimeEnabledFeature
with
> status=experimental) and get confirmation from customers that this is the
right
> API before being the first to ship this.
> http://www.chromium.org/blink/runtime-enabled-features
> 
> If other vendors are already shipping this of course that's a different story.
> 
> Once we ship to the web, it's much more difficult to unship.

Ok, I'll add the experimental flag as soon as I have some time to work on this.

[eseidel, 2014-02-18 18:33:11]

eae may be a useful reviewer for you once you add the runtime flag.

https://codereview.chromium.org/141433026/diff/110001/Source/core/html/TextMe...
File Source/core/html/TextMetrics.idl (right):

https://codereview.chromium.org/141433026/diff/110001/Source/core/html/TextMe...
Source/core/html/TextMetrics.idl:29: readonly attribute float
actualBoundingBoxLeft;
To make this runtime enabled, each of these just gets a
[RuntimeEnabled=TextMetrics] attribute.

[Stephen White, 2014-02-18 18:54:07]

On 2014/02/18 18:33:11, eseidel wrote:
> eae may be a useful reviewer for you once you add the runtime flag.
> 
>
https://codereview.chromium.org/141433026/diff/110001/Source/core/html/TextMe...
> File Source/core/html/TextMetrics.idl (right):
> 
>
https://codereview.chromium.org/141433026/diff/110001/Source/core/html/TextMe...
> Source/core/html/TextMetrics.idl:29: readonly attribute float
> actualBoundingBoxLeft;
> To make this runtime enabled, each of these just gets a
> [RuntimeEnabled=TextMetrics] attribute.

We usually use the ExperimentalCanvasFeatures for canvas-only features that have
yet to get an intent-to-ship. It's kind of like the experimental web platform
features, only more fine-grained (canvas-only). It'd be nice to keep them
grouped together, if possible.

[sugoi1, 2014-02-24 21:38:01]

Finally, the new patch is here.
All new TextMetrics members are hidden behind the
enable-experimental-canvas-features flag.
Also, I changed the tests, which were pixel test to text based tests to make
them faster, since all we care about is the value returned for each TextMetrics
measurement, not the actual rendering.

***IMPORTANT NOTE***:
I did make a modification to the code that will appear in chromium. The hanging
baseline did not have a constant meaning throughout chromium, so I made a small
change to make it uniform. Based on
http://wiki.apache.org/xmlgraphics-fop/LineLayout/AlignmentHandling, which is
already used as a reference in SVGTextLayoutEngineBaseline.cpp, the hanging
baseline should be put at 80% of the ascender height. Previously, the hanging
baseline in canvas was at ascender height, same as the "top" baseline. Visually,
the hanging baseline looks a lot better at 80% and is no longer useless, since
it is no longer redundant with the top baseline.

[sugoi1, 2014-03-04 15:12:36]

Ping.
Updated to ToT after a week of inactivity.

[Justin Novosad, 2014-03-04 15:32:51]

lgtm with nit

https://codereview.chromium.org/141433026/diff/320001/Source/core/html/TextMe...
File Source/core/html/TextMetrics.h (right):

https://codereview.chromium.org/141433026/diff/320001/Source/core/html/TextMe...
Source/core/html/TextMetrics.h:43: void setActualBoundingBoxLeft(float abbl) {
m_actualBoundingBoxLeft = abbl; }
Style Nit: variable names should be words. "abbl" -> "actualBoundingBoxLeft" or
"value"

[sugoi1, 2014-03-04 15:45:52]

https://codereview.chromium.org/141433026/diff/320001/Source/core/html/TextMe...
File Source/core/html/TextMetrics.h (right):

https://codereview.chromium.org/141433026/diff/320001/Source/core/html/TextMe...
Source/core/html/TextMetrics.h:43: void setActualBoundingBoxLeft(float abbl) {
m_actualBoundingBoxLeft = abbl; }
On 2014/03/04 15:32:51, junov wrote:
> Style Nit: variable names should be words. "abbl" -> "actualBoundingBoxLeft"
or
> "value"

Done.

[sugoi1, 2014-03-04 16:32:29]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-03-04 16:33:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/141433026/340001

[sugoi1, 2014-03-05 16:01:21]

The CQ bit was unchecked by sugoi@chromium.org

[sugoi1, 2014-03-05 16:03:00]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-03-05 16:04:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/141433026/360001

[commit-bot: I haz the power, 2014-03-05 20:59:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/141433026/360001

[commit-bot: I haz the power, 2014-03-06 02:24:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 02:24:24]

Try jobs failed on following builders: win_blink_rel

[sugoi, 2014-03-06 13:02:50]

The CQ bit was checked by sugoi@google.com

[commit-bot: I haz the power, 2014-03-06 13:03:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/141433026/360001

[commit-bot: I haz the power, 2014-03-06 13:22:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 13:22:22]

Try jobs failed on following builders: win_blink_rel

[sugoi, 2014-03-07 13:48:22]

The CQ bit was checked by sugoi@google.com

[commit-bot: I haz the power, 2014-03-07 13:48:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sugoi@chromium.org/141433026/360001

[commit-bot: I haz the power, 2014-03-07 14:41:30]

Change committed as 168737