Fixing superscript and subscript baseline for tiny fonts in SVG

Fixing superscript and subscript baseline for tiny fonts in SVG

When the font size is small (<1), superscript and subscript tags
(baseline-shift="super" and baseline-shift="sub") do not make any
difference in the baseline.

BUG=656638

[zakerinasab, 2016-10-17 19:01:37]

Description was changed from

==========
Patch set 1.

BUG=656638
==========

to

==========
Fixing superscript and subscript baseline for tiny fonts in SVG

When the font size is small (<1), superscript and subscript tags
(baseline-shift="super" and baseline-shift="sub") do not make any
difference in the baseline.

BUG=656638
==========

[zakerinasab, 2016-10-17 19:01:37]

zakerinasab@chromium.org changed reviewers:
+ eae@chromium.org

[zakerinasab, 2016-10-17 19:02:01]

New patch submitted.

[eae, 2016-10-17 20:14:23]

Thanks for your patience, this code is suprt old and quite messy :(

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp (right):

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp:405:
scaledFont.getFontDescription().setSubpixelAscentDescent(true);
It's not clear that updateScaledFont is guaranteed to run before m_scaledFont is
used, perhaps you could set a breakpoint there or add an assert to see if that
is the case.

There is also SVGTextLayoutEngine::layoutTextOnLineOrPath that access the font
directly which might need updating.

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/FontDescription.h:20: * along with this
library; see the file COPYING.LIB. If not, write to
?

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/FontDescription.h:420:
other2.setSubpixelAscentDescent(m_fields.m_subpixelAscentDescent);
This shouldn't be needed, the fields are compared by comparing the
fieldsAsUnsigned masks. The fields should also be included in the hash
(styleHashWithoutFamilyList) as it uses the full width of the bit fields.

[zakerinasab, 2016-10-18 15:51:16]

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp (right):

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp:405:
scaledFont.getFontDescription().setSubpixelAscentDescent(true);
On 2016/10/17 20:14:23, eae wrote:
> It's not clear that updateScaledFont is guaranteed to run before m_scaledFont
is
> used, perhaps you could set a breakpoint there or add an assert to see if that
> is the case.

Correct. The problem is that the rounding at the beginning of platformInit()
affects the calculation of other metrics as well. The current call to
setSubpixelAscentDescent() does not update the previous calculations, so you
have to do that before the font gets initiated, which brings you to the change
needed in LayoutText.cpp which is outside SVG code and affects others too (like
Canvas). We can either go with something universal, or to remain more cautious,
we can change setSubpixelAscentDescent() to update the previous calculations if
needed. WDYT?


> There is also SVGTextLayoutEngine::layoutTextOnLineOrPath that access the font
> directly which might need updating.

No benefit there as the font is already created there, unless we update the
previous metrics in setSubpixelAscentDescent(). In this case IMO this will
complete the CL.

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/FontDescription.h:20: * along with this
library; see the file COPYING.LIB. If not, write to
On 2016/10/17 20:14:23, eae wrote:
> ?

It seems to has been replaced by someone by mistake previously.

https://codereview.chromium.org/2427773002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/fonts/FontDescription.h:420:
other2.setSubpixelAscentDescent(m_fields.m_subpixelAscentDescent);
On 2016/10/17 20:14:23, eae wrote:
> This shouldn't be needed, the fields are compared by comparing the
> fieldsAsUnsigned masks. The fields should also be included in the hash
> (styleHashWithoutFamilyList) as it uses the full width of the bit fields.
> 

Acknowledged.

[zakerinasab, 2016-10-18 19:15:15]

New patch submitted. PTAL. IMHO this one is better as it is focused on baseline
and calculateBaselineShift()
is always called in SVGTextLayoutEngine::layoutTextOnLineOrPath() so I think it
provides the coverage too. If you were okay with the code I'll go ahead to
prepare the new layout test results by trying rebaseline-cl again.

[zakerinasab, 2016-10-18 19:17:57]

Patchset #3 (id:40001) has been deleted

[eae, 2016-10-18 22:12:04]

This looks great, thank you!

https://codereview.chromium.org/2427773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontDescription.h (right):

https://codereview.chromium.org/2427773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontDescription.h:20: * along with this
library; see the file COPYING.LIB. If not, write to
Please fix this to remove the "LIB." string.

[zakerinasab, 2016-10-19 14:14:41]

zakerinasab@chromium.org changed reviewers:
+ junov@chromium.org

[zakerinasab, 2016-10-19 14:14:41]

New patch uploaded after rebaseline-cl.
- It seems that this CL has affected other layout tests. Please advise on
keeping the updated TestExpectations or revert to the older version.
- CL works fine on Linux and Mac but it doesn't work on Mac Retina. Needs more
investigation. Any ideas?
- CL does not seem to work on Windows. Looking into that.

[zakerinasab, 2016-10-19 14:45:43]

On my Windows desktop the patch works fine, still doesn't generate the proper
output on Windows try-bots.

[zakerinasab, 2016-10-19 15:18:36]

The CL also works fine on my Mac Retina.

[zakerinasab, 2016-10-19 23:31:03]

Patchset #6 (id:120001) has been deleted

[zakerinasab, 2016-10-20 14:38:58]

Patchset #5 (id:100001) has been deleted

[zakerinasab, 2016-10-20 14:42:55]

Patchset #5 (id:140001) has been deleted

[zakerinasab, 2016-10-25 14:04:11]

Patchset #7 (id:200001) has been deleted

[zakerinasab, 2016-10-25 14:04:19]

Patchset #7 (id:220001) has been deleted

[zakerinasab, 2016-10-25 14:04:29]

Patchset #7 (id:240001) has been deleted

[zakerinasab, 2016-10-25 14:04:39]

Patchset #7 (id:260001) has been deleted

[zakerinasab, 2016-10-25 20:25:18]

Patchset #9 (id:320001) has been deleted

[zakerinasab, 2016-10-25 20:25:27]

Patchset #9 (id:340001) has been deleted

[zakerinasab, 2016-10-26 15:31:57]

Patchset #9 (id:360001) has been deleted

[zakerinasab, 2016-10-26 17:40:22]

Patchset #8 (id:300001) has been deleted

[zakerinasab, 2016-10-26 17:43:42]

Patchset #8 (id:370011) has been deleted

[zakerinasab, 2016-10-26 20:40:37]

Patchset #8 (id:390001) has been deleted

[zakerinasab, 2016-10-26 20:45:41]

Patchset #9 (id:420001) has been deleted

[zakerinasab, 2016-10-31 15:42:10]

Patchset #8 (id:370012) has been deleted

[zakerinasab, 2016-10-31 16:47:46]

Patchset #9 (id:460001) has been deleted

[zakerinasab, 2016-10-31 16:57:05]

New patch submitted. The issue with Mac Retina try-bots is resolved. The issue
with Windows try-bots however needs to turn on subpixel text rendering for small
fonts.

@eae, PTAL.

[eae, 2016-10-31 17:04:14]

LGTM

[zakerinasab, 2016-10-31 17:57:31]

New patch submitted. Cleaning up FontPlatformDataWin.cpp after removing the
check for subpixel text rendering.
@eae PTAL.

[eae, 2016-10-31 18:01:42]

On 2016/10/31 17:57:31, zakerinasab wrote:
> New patch submitted. Cleaning up FontPlatformDataWin.cpp after removing the
> check for subpixel text rendering.
> @eae PTAL.

NOT LGTM

Why did you remove the check for subpixel text rendering? That has nothing to do
with subscript and is there to make certain legacy fonts legible at smaller
sizes on windows.

[zakerinasab, 2016-10-31 18:16:12]

On 2016/10/31 18:01:42, eae wrote:
> On 2016/10/31 17:57:31, zakerinasab wrote:
> > New patch submitted. Cleaning up FontPlatformDataWin.cpp after removing the
> > check for subpixel text rendering.
> > @eae PTAL.
> 
> NOT LGTM
> 
> Why did you remove the check for subpixel text rendering? That has nothing to
do
> with subscript and is there to make certain legacy fonts legible at smaller
> sizes on windows.

This code has the same effect as the code in the previous patch (patch set 9)
which you LGTM. It just sets the SkPaint::kSubpixelText_Flag by default. If it
is not acceptable, we need to plumb a variable from SimpleFontData::platformInit
to FontPlatformData::setupPaint to specify that we have a sub-/superscript here.
I'll do that and upload another patch. However, I suspect this might not solve
the issue with Windows try-bots. The problem here is that the Windows try-bots
do not use/assume a high DPI screen, so the 0.3 px font size falls below the
threshold and does not receive subpixel text rendering. Turning on subpixel
rendering for sub/super in these fonts does not make sense when the original
baseline does not have it.

[eae, 2016-10-31 18:23:01]

You are removing a set of functionality that is unrelated to your change. If you
want to change how we enable subpixel rendering for all text on windows then
please do that as a separate change so that we can have a discussion about it
instead of doing it as a part of a seemingly unrelated change to svg baselines.

[zakerinasab, 2016-10-31 19:06:04]

On 2016/10/31 18:23:01, eae wrote:
> You are removing a set of functionality that is unrelated to your change. If
you
> want to change how we enable subpixel rendering for all text on windows then
> please do that as a separate change so that we can have a discussion about it
> instead of doing it as a part of a seemingly unrelated change to svg
baselines.

To clarify, as long as subpixel text rendering for these small fonts is off, the
subscript and superscript will not work properly on Windows try-bots for very
small fonts. So these changes are related. The CL works fine on my local Windows
machine because the display is high DPI, which is not the case on try-bots. It
makes sense to keep this for another bug, so I will revert the changes of
FontPlatformDataWin.cpp and will submit another patch. If that was okay, I will
rebaseline and commit.

[eae, 2016-10-31 19:11:27]

On 2016/10/31 19:06:04, zakerinasab wrote:
> On 2016/10/31 18:23:01, eae wrote:
> > You are removing a set of functionality that is unrelated to your change. If
> you
> > want to change how we enable subpixel rendering for all text on windows then
> > please do that as a separate change so that we can have a discussion about
it
> > instead of doing it as a part of a seemingly unrelated change to svg
> baselines.
> 
> To clarify, as long as subpixel text rendering for these small fonts is off,
the
> subscript and superscript will not work properly on Windows try-bots for very
> small fonts. So these changes are related. The CL works fine on my local
Windows
> machine because the display is high DPI, which is not the case on try-bots. It
> makes sense to keep this for another bug, so I will revert the changes of
> FontPlatformDataWin.cpp and will submit another patch. If that was okay, I
will
> rebaseline and commit.

That might be but you aren't changing this just for sub/sup for SVG, you are
changing it for all text. We've spent *years* working on making sure our text
rendering is legible across scripts, fonts and operating systems. This code is
there for a reason.  You can't fundamentally change how we do text rendering on
a whim to fix one edge case. 

I'm sorry but this needs to go back to the drawing board.

[zakerinasab, 2016-10-31 19:28:20]

On 2016/10/31 19:11:27, eae wrote:
> On 2016/10/31 19:06:04, zakerinasab wrote:
> > On 2016/10/31 18:23:01, eae wrote:
> > > You are removing a set of functionality that is unrelated to your change.
If
> > you
> > > want to change how we enable subpixel rendering for all text on windows
then
> > > please do that as a separate change so that we can have a discussion about
> it
> > > instead of doing it as a part of a seemingly unrelated change to svg
> > baselines.
> > 
> > To clarify, as long as subpixel text rendering for these small fonts is off,
> the
> > subscript and superscript will not work properly on Windows try-bots for
very
> > small fonts. So these changes are related. The CL works fine on my local
> Windows
> > machine because the display is high DPI, which is not the case on try-bots.
It
> > makes sense to keep this for another bug, so I will revert the changes of
> > FontPlatformDataWin.cpp and will submit another patch. If that was okay, I
> will
> > rebaseline and commit.
> 
> That might be but you aren't changing this just for sub/sup for SVG, you are
> changing it for all text. We've spent *years* working on making sure our text
> rendering is legible across scripts, fonts and operating systems. This code is
> there for a reason.  You can't fundamentally change how we do text rendering
on
> a whim to fix one edge case. 
> 
> I'm sorry but this needs to go back to the drawing board.

Right. I'll revert and rebaseline. If it was okay I'll file another bug for the
sub/superscript issue with small fonts on Windows.

[zakerinasab, 2016-10-31 20:00:05]

Patchset #11 (id:520001) has been deleted

[zakerinasab, 2016-11-01 15:22:29]

Patchset #12 (id:560001) has been deleted

[zakerinasab, 2016-11-01 15:52:39]

New patch submitted. Changes in FontPlatformDataWin.cpp reverted. This fix
doesn't solve the problem for tiny fonts on Windows machines with low DPI
screens (deviceScaleFactor <= 1.5). New bug filed: crbug.com/661178
@eae PTAL. Thanks.

[eae, 2016-11-04 18:25:28]

Much better, thank you.

LGTM w/suggestions

https://codereview.chromium.org/2427773002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2427773002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:89: void
SimpleFontData::platformInit(bool subpixelAscentDescent) {
bool subpixelAscentDescentForSmallSizes would be a better name given it only
triggers for acsent < 3.

https://codereview.chromium.org/2427773002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:136:
subpixelAscentDescent =
Please use a different name here to avoid the appearance of masking the
parameter subpixelAscentDescent.

[zakerinasab, 2016-11-07 16:07:29]

Patchset #13 (id:600001) has been deleted

[zakerinasab, 2016-11-07 17:09:43]

Patchset #13 (id:620001) has been deleted

[zakerinasab, 2016-11-07 18:26:14]

Patchset #13 (id:640001) has been deleted

[zakerinasab, 2016-11-07 18:40:26]

New patch submitted. Comments addressed. I'll try to land the CL if try-bots
were green.

https://codereview.chromium.org/2427773002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp (right):

https://codereview.chromium.org/2427773002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:89: void
SimpleFontData::platformInit(bool subpixelAscentDescent) {
On 2016/11/04 18:25:28, eae wrote:
> bool subpixelAscentDescentForSmallSizes would be a better name given it only
> triggers for acsent < 3.

Done.

https://codereview.chromium.org/2427773002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp:136:
subpixelAscentDescent =
On 2016/11/04 18:25:28, eae wrote:
> Please use a different name here to avoid the appearance of masking the
> parameter subpixelAscentDescent.

Done.

[zakerinasab, 2016-11-07 22:13:08]

Patchset #13 (id:660001) has been deleted

[zakerinasab, 2016-11-08 14:05:38]

Patchset #13 (id:680001) has been deleted

[zakerinasab, 2016-11-08 18:09:08]

Patchset #14 (id:720001) has been deleted

[zakerinasab, 2016-11-08 19:43:38]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2016-11-08 19:43:39]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2427773002/#ps740001
(title: "Rebaselining layout tests.")

[commit-bot: I haz the power, 2016-11-08 19:44:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 21:46:53]

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

[commit-bot: I haz the power, 2016-11-08 21:46:55]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zakerinasab, 2016-11-09 14:41:15]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2016-11-09 14:41:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 16:03:29]

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

[commit-bot: I haz the power, 2016-11-09 16:03:31]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zakerinasab1, 2016-12-21 18:15:29]

The CQ bit was checked by zakerinasab@google.com

[zakerinasab1, 2016-12-21 18:15:30]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2427773002/#ps760001
(title: "Rebaseline")

[commit-bot: I haz the power, 2016-12-21 18:15:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 19:44:39]

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

[commit-bot: I haz the power, 2016-12-21 19:44:41]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)