Add SVG Text to support the CSS 'text-orientation' property

Add SVG Text to support the CSS 'text-orientation' property

This patch changes SVGTextLayoutEngine to rely on fonts to orient
glyphs in vertical flow.

As the result, this patch changes:

1. SVG text uses the CSS 'text-orientation' property to orient glyphs
   in vertical flow. This is defined in the CSS spec[1] and will give
   consistent authoring for CSS and SVG.
2. Vertical alternate forms of glyphs are enabled as defined in the
   CSS/SVG specs.
3. The SVG 'glyph-orientation-vertical/glyph-orientation-horizontal'
   properties are no longer honored. Removal of these properties will
   be in a following patch.

[1] https://drafts.csswg.org/css-writing-modes-3/#glyph-orientation

BUG=540782, 543397
Committed: https://crrev.com/a5703876c2ba95f3cd5684edeaf91314b0cbd627
Cr-Commit-Position: refs/heads/master@{#354970}

[kojii, 2015-10-16 07:52:12]

kojii@chromium.org changed reviewers:
+ fs@opera.com

[kojii, 2015-10-16 07:52:13]

PTAL.

bots failures are conflicting NeedsRebaseline as rebaseline bots are not working
for a few days, I'll probably need to update TestExpectations when rebaseline
bots returns. But the code is ready for review.

Turns out we have a test that includes vertical alternate forms:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

Small white circle changing the position is the one. This is a period (FULL
STOP) for CJK. When in vertical, it should move to the right-top corner. It
didn't before, but with this CL enables vertical alternate form, it does.

[fs, 2015-10-16 10:54:50]

This looks great! LGTM w/ a few nits and a question.

On 2015/10/16 at 07:52:13, kojii wrote:
> PTAL.
> 
> bots failures are conflicting NeedsRebaseline as rebaseline bots are not
working for a few days, I'll probably need to update TestExpectations when
rebaseline bots returns. But the code is ready for review.

Acknowledged.
 
> Turns out we have a test that includes vertical alternate forms:
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
> 
> Small white circle changing the position is the one. This is a period (FULL
STOP) for CJK. When in vertical, it should move to the right-top corner. It
didn't before, but with this CL enables vertical alternate form, it does.

That's good to hear. I CC'd you on a bug that may have been the same issue
(crbug.com/525043)

https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/text/resources/glyph-orientation-vertical.css
(right):

https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/text/resources/glyph-orientation-vertical.css:1:
/* Maps the SVG 'glyph-orientation-vertical' attribute to
What will happen to this when we remove the properties? Will it be dropped? If
so I wonder if it really makes sense to add it in the first place, and just add
the new baselines instead.

https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp (right):

https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:388: // but
does not expose today to avoid the API becomes too complex.
Nit: "but does not expose today" -> "but is not exposed today" (and "becomes" ->
"becoming" or just "avoid API complexity" or something maybe)

Also, should this be a TODO for the SVGTextMetricsBuilder (i.e. SVGTextMetrics
would store logical values instead of width7height as it does today)?

https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:396: float
yOrientationShift = 0;
These two are now only used for <textPath> related adjustments - could you move
them down to just before the m_inPathLayout check and maybe rename them to
textPath<something> (textPathGlyphMidpointX/Y maybe. I think I have a patch
somewhere that would be able to simplify that even more.)

[kojii, 2015-10-17 13:50:12]

Patchset #5 (id:80001) has been deleted

[kojii, 2015-10-17 13:51:02]

Patchset #4 (id:60001) has been deleted

[kojii, 2015-10-18 14:02:27]

On 2015/10/16 10:54:50, fs wrote:
> 
> That's good to hear. I CC'd you on a bug that may have been the same issue
> (crbug.com/525043)

Yeah, that's the one, I resolved it as dup.

>
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/svg/text/resources/glyph-orientation-vertical.css
> (right):
> 
>
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/svg/text/resources/glyph-orientation-vertical.css:1:
> /* Maps the SVG 'glyph-orientation-vertical' attribute to
> What will happen to this when we remove the properties? Will it be dropped? If
> so I wonder if it really makes sense to add it in the first place, and just
add
> the new baselines instead.

Not sure I understand your recommendation, let me try to explain and see if it
matches.

Since this CL already ignores computed values of these properties, removing
doesn't affect the baselines. It just changes unused properties to
unrecognized/unparsed properties.

Adding this CSS to tests helps us to ensure authors can map auto, 0, and 90 and
to ensure we produce the same results. However, reaselines are still needed
because 1) other values are not mapped, and 2) there are 1px diff due to
rounding differences.

We can chose not to use the mapping CSS. That gives us clearer picture of these
properties are no longer supported, but will lose benefits above.

I have mild preference to keep this, as it allows us to test when authors map,
but I'm ok to remove if you prefer. Does this answer? If so, can you give me
your preference?

>
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp
(right):
> 
>
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:388: // but
> does not expose today to avoid the API becomes too complex.
> Nit: "but does not expose today" -> "but is not exposed today" (and "becomes"
->
> "becoming" or just "avoid API complexity" or something maybe)

Thanks, that's better ;)

> Also, should this be a TODO for the SVGTextMetricsBuilder (i.e. SVGTextMetrics
> would store logical values instead of width7height as it does today)?

Added TODO, storing logical values is a good idea. I haven't looked at other
usages very well yet, but the current logic calls Font::width() for each single
character, and I'm wondering if I can change that, because width("A") +
width("V") != width("AV") when kerning is enabled, or joining, other font
features too.

I'll look into it in following patches.

>
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:396: float
> yOrientationShift = 0;
> These two are now only used for <textPath> related adjustments - could you
move
> them down to just before the m_inPathLayout check and maybe rename them to
> textPath<something> (textPathGlyphMidpointX/Y maybe. I think I have a patch
> somewhere that would be able to simplify that even more.)

Done, pls let me know if you prefer other names.

[kojii, 2015-10-18 14:03:45]

kojii@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[kojii, 2015-10-18 14:03:46]

drott@, eae@, PTAL for one file in platform/fonts.

[fs, 2015-10-18 20:46:02]

On 2015/10/18 at 14:02:27, kojii wrote:
> On 2015/10/16 10:54:50, fs wrote:
> > 
> > That's good to hear. I CC'd you on a bug that may have been the same issue
> > (crbug.com/525043)
> 
> Yeah, that's the one, I resolved it as dup.

Thanks.

> >
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Layo...
> > File
> >
third_party/WebKit/LayoutTests/svg/text/resources/glyph-orientation-vertical.css
> > (right):
> > 
> >
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Layo...
> >
third_party/WebKit/LayoutTests/svg/text/resources/glyph-orientation-vertical.css:1:
> > /* Maps the SVG 'glyph-orientation-vertical' attribute to
> > What will happen to this when we remove the properties? Will it be dropped?
If
> > so I wonder if it really makes sense to add it in the first place, and just
add
> > the new baselines instead.
> 
> Not sure I understand your recommendation, let me try to explain and see if it
matches.
> 
> Since this CL already ignores computed values of these properties, removing
doesn't affect the baselines. It just changes unused properties to
unrecognized/unparsed properties.

Understood.

> Adding this CSS to tests helps us to ensure authors can map auto, 0, and 90
and to ensure we produce the same results. However, reaselines are still needed
because 1) other values are not mapped, and 2) there are 1px diff due to
rounding differences.
> 
> We can chose not to use the mapping CSS. That gives us clearer picture of
these properties are no longer supported, but will lose benefits above.
> 
> I have mild preference to keep this, as it allows us to test when authors map,
but I'm ok to remove if you prefer. Does this answer? If so, can you give me
your preference?

Ok, so the only intention is to _test_ that presentation attributes are
"re-mappable" - no harm doing that I guess (not overly specific in itself
though, since it essentially only tests that attribute selectors work.)

So, I'm fine with having this as some form of testing. (One might argue that it
just be a single targetted test for the mapping though.)

> >
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp
(right):
> > 
> >
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:388: //
but
> > does not expose today to avoid the API becomes too complex.
> > Nit: "but does not expose today" -> "but is not exposed today" (and
"becomes" ->
> > "becoming" or just "avoid API complexity" or something maybe)
> 
> Thanks, that's better ;)
> 
> > Also, should this be a TODO for the SVGTextMetricsBuilder (i.e.
SVGTextMetrics
> > would store logical values instead of width7height as it does today)?
> 
> Added TODO, storing logical values is a good idea. I haven't looked at other
usages very well yet, but the current logic calls Font::width() for each single
character, and I'm wondering if I can change that, because width("A") +
width("V") != width("AV") when kerning is enabled, or joining, other font
features too.

Yeah, that's a problem - but it's not a new one. There are some provisions for
joining, but kerning is definitely an issue - especially since the chunks
haven't been determined yet at that point (AFAICR). Regardless though, I think
it would be a progression.

> I'll look into it in following patches.
> 
> >
https://codereview.chromium.org/1404853003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:396: float
> > yOrientationShift = 0;
> > These two are now only used for <textPath> related adjustments - could you
move
> > them down to just before the m_inPathLayout check and maybe rename them to
> > textPath<something> (textPathGlyphMidpointX/Y maybe. I think I have a patch
> > somewhere that would be able to simplify that even more.)
> 
> Done, pls let me know if you prefer other names.

New names WFM.

[drott, 2015-10-19 07:00:04]

LGTM for platform/fonts/* with just a naming suggestion and please file a bug
report for TODOs.

Nice work, great to see this unification of vertical handling.

https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h (right):

https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h:52: // TODO(kojii):
We should store logical width (advance) and height instead
Could you add a bug report and reference it from here?

https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontOrientation.h (right):

https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontOrientation.h:58: inline
FontOrientation resolveMixedFontOrientation(FontOrientation orientation, UChar32
character)
How about adjustOrientationForCharacterInMixedVertical()?

[kojii, 2015-10-20 03:15:04]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-10-20 03:15:05]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
drott@chromium.org
Link to the patchset: https://codereview.chromium.org/1404853003/#ps120001
(title: "drott review")

[commit-bot: I haz the power, 2015-10-20 03:15:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1404853003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1404853003/120001

[commit-bot: I haz the power, 2015-10-20 03:20:26]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-10-20 03:21:18]

Patchset 5 (id:??) landed as
https://crrev.com/a5703876c2ba95f3cd5684edeaf91314b0cbd627
Cr-Commit-Position: refs/heads/master@{#354970}

[kojii, 2015-10-20 03:43:56]

On 2015/10/19 07:00:04, drott wrote:
> LGTM for platform/fonts/* with just a naming suggestion and please file a bug
> report for TODOs.
> 
> Nice work, great to see this unification of vertical handling.
> 
>
https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h (right):
> 
>
https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h:52: // TODO(kojii):
> We should store logical width (advance) and height instead
> Could you add a bug report and reference it from here?

Done.

>
https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/fonts/FontOrientation.h (right):
> 
>
https://codereview.chromium.org/1404853003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/fonts/FontOrientation.h:58: inline
> FontOrientation resolveMixedFontOrientation(FontOrientation orientation,
UChar32
> character)
> How about adjustOrientationForCharacterInMixedVertical()?

Done, thank you for the review.