New issue
Advanced search Search tips
Starred by 40 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug

Blocked on:
issue 774502


Show other hotlists

Hotlists containing this issue:
Top-Starred-Bugs


Sign in to add a comment

Awkward letter spacing on BlinkMacSystemFont

Project Member Reported by paulir...@chromium.org, Jul 12 2016 Back to list

Issue description

Github.com just rolled out a change to their fonts, placing -apple-system and BlinkMacSystemFont at the top of their font stack.

On Mac, things look awkward. 
The attached screenshot clearly shows the text looking sensible in Safari and difficult in Chrome.

In addition to being used on Github, DevTools uses the systemfont internally, and we've noticed some odd spacing/kerning/tracking in our text as well.

 
blinkmacsystemfont differences.png
89.0 KB View Download
Cc: erikc...@chromium.org
It appears our different character widths is due to a hardcoding in LayoutTextControlMultiLine::getAvgCharWidth. See attached screenshot.

This special behavior originates to 2009 in some Windows input/textarea width compatibility issues: https://bugs.webkit.org/show_bug.cgi?id=25566
(Interestingly, it appears this as for form fields exclusively, but the current behavior affects body text.)

Today, Safari does not have the same behavior as us and the original getAvgCharWidth workaround remains for "Lucida Grande": https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderTextControlSingleLine.cpp#L275

+cc erikchen who implemented BlinkMacSystemFont originally in https://codereview.chromium.org/1292593003 

It's looking like the getAvgCharWidth hacks should revert back to Lucida Grande, but I'm eager to hear what the layout & text experts think!
Screen Shot 2016-07-12 at 2.33.49 PM.png
214 KB View Download
Cc: tkent@chromium.org keishi@chromium.org thakis@chromium.org
I am, alas, not a layout or text expert. Adding some others.

Comment 3 by tkent@chromium.org, Jul 12 2016

Cc: -tkent@chromium.org -keishi@chromium.org drott@chromium.org kojii@chromium.org e...@chromium.org
Components: -Blink>Forms>Text Blink>Fonts
LayoutTextControlMultiLine::getAvgCharWidth should be unrelated.  It is only for <input> and <textarea>.

Aye, looking some more, it appears LayoutTextControlMultiLine::getAvgCharWidth is indeed unrelated. Apologies. 

Any other ideas on why our rendering is so wonky in comparison to Safari's?

Comment 5 by kojii@chromium.org, Jul 13 2016

Components: -Blink>Layout
Labels: OS-Mac
BTW, WebKit still has the hack, they just moved it down to Font (which looks sensible to me):
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/FontCascade.cpp#L420

It looks like either kerning is turned off, or subpixel is disabled, probably the latter.

Are you on non-Retina? I'm seeing on my Retina mac, see a slight difference but not as much as the screenshot.
I posted the original screenshot on Twitter. I'm on a 2013 MacBook Air, so non-Retina. I have no idea about the Blink or Webkit internals, but my initial thoughts were a problem with both kerning and subpixel rendering. Still seems likely (at least kerning is definitely different, and I'd argue wrong).

Comment 7 by e...@chromium.org, Jul 13 2016

Labels: Needs-Feedback
Do you have a URL that demonstrates the problem? Ideally the one from your screenshot?

https://github.com/ uses the webfont Roboto exclusively for me on both Mac, Windows and Linux.

Regardless, LayoutTextControlMultiLine::getAvgCharWidth is unrelated as it's only used for form controls.

Comment 8 by o...@github.com, Jul 13 2016

Hi, I work at Github! You'll want to visit a repository page to see the font stack in full effect. We use Roboto in our marketing pages.

See https://github.com/github/linguist as an example.

Comment 9 by e...@chromium.org, Jul 13 2016

Re: #8, thanks Otto. 

On a retina mac the rendering looks almost identical between Safari and Chrome. Both have kerning enable and respect the fonts kerning information.

I suspect that Safari uses optimzeSpeed (which disables kerning) on non-retina but don't have a non-retina mac to test on at the moment.

Could you please try to either turn on kerning on Safari (using text-rendering: optimizeLegability) or disable kerning on Chrome (using font-kerning: none)?

I believe Safari (9.1.1 on macOS 10.11.5) is already using "text-rendering: optimizeLegability". Setting it has no effect. Setting "text-rendering: optimizeSpeed" spaces out the letters (increases the kerning) very slightly, but not to the same extent that Chrome is doing by default. None of the other "text-rendering" options has any effect.

On Chrome (51.0.2704.106), setting "font-kerning: none" increases the kerning more than it already is (though only slightly more), making it even worse. Setting any "text-rendering" options has no effect. Neither did adjust the "LCD text antialiasing" flag.

Comment 11 by o...@github.com, Jul 14 2016

I made a short screen recording to demonstrate the differences I see on my end (a retina 13" MBP in max screen resolution). See http://f.cl.ly/items/3Z2W1M0c0j0i1b1v201D/safari-chrome-sf-rendering.mov (or let me know if that doesn't work).

Interestingly enough, I noticed that the sidebar labels (font-weight:600;) aren't as affected as the normal weight headings and comment content.

I don't see it mentioned here, but for reference, the San Francisco automatically switches between SF Text and SF Display variants at 19px. I hesitate to say that's the cause because all font-sizes appear affected here, but wanted to at least mention it.

Comment 12 by e...@chromium.org, Jul 14 2016

Thanks joshua.jabbour and otto.
I'm fairly sure it's the fact that hb-coretext currently always renders at the same point size and linearly scales the results, whereas San Francisco uses the 'trak' table to adjust spacing at various sizes.  IIRC Skia also has such an assumption baked in... But we should be able to fix this in HarfBuzz without changing in Skia.  I'll think about how to fix this.  We probably need new API in harfbuzz.

Comment 14 by drott@chromium.org, Jul 18 2016

Cc: bunge...@chromium.org
Ben, is Behdad's assumption about the linear scaling in Skia still correct? I seem to remember there were some changes in Skia around this. Thanks for commenting.
Way down inside, Skia calls CTFontGetAdvancesForGlyphs to get the glyph advance. When Skia sets up the CTFont it does so with the exact size (extracted from the current matrix). However, it does so by making an 'exact' copy; it avoids the automatic switch between 'SF Text' and 'SF Display'. So if Blink calls SkCreateTypefaceFromCTFont to create a typeface, that exact font is always used (the font will not change because of size / matrix changes). If we didn't do this then Blink goes crazy, if text scales all of a sudden the glyphs are wrong (because a different font with different glyph mappings is suddenly used just because the matrix changed).

I even wrote a comment about the 'trak' table at one point... the issue with 'trak' was probably working with https://codereview.chromium.org/752183002/ but then broken with https://codereview.chromium.org/770383002/ because asking CG for the requested size (and supplying the matrix for the rest) doesn't work with color bitmaps.
This is something we (drott@, bungeman@, and I) should look into together, perhaps at the next Chrome text work week.
Status: Available
Sounds good to me. Happy to come out. Ben, would you be up for meeting in US-CHAP for example and work on this together? Behdad@, I have collected another couple of things for such a week that we could look into.
Would love to join if you all are ok ;-)
Of course!

Comment 20 by fer...@gmail.com, Sep 22 2016

BlinkMacSystemFont looks a lot worse since updating to macOS 10.12 Sierra. The letter spacing got a lot wider, and it seems that [contextual kerning](https://en.wikipedia.org/wiki/Kerning#Contextual_kerning) isn't working anymore.

Here are screenshots demonstrating the difference:

macOS 10.11 El Capitan: https://imgur.com/a/bdQ2q
macOS 10.12 Sierra: https://imgur.com/a/MdG52

Is this change in macOS 10.12 related to this bug, or something different?

Comment 21 by drott@chromium.org, Sep 22 2016

feross@, if I identify your screenshots correctly, they affect the title bar and would be a Chrome UI issue. Do you have any screenshots of where this affects the content?

Comment 22 by fer...@gmail.com, Sep 22 2016

drott@, My screenshots are from a web page, not the title bar. They do look a bit misleading ;-).

Here is a clearer example:

macOS 10.11 El Capitan: https://imgur.com/a/61xPj
macOS 10.12 Sierra: https://imgur.com/a/pZd5K

Source code: https://codepen.io/anon/pen/WGRaka

You can see on 10.11 that the letters around the 'T' in 'Torrent' are a lot tighter and legible. On 10.12, the space around the 'T' is too wide.

Comment 23 by drott@chromium.org, Sep 23 2016

Cc: behdad@chromium.org
Labels: -Needs-Feedback
Owner: drott@chromium.org
Status: Assigned
Behdad, we should prioritize the trak table metrics on Mac.

Comment 24 by drott@chromium.org, Sep 23 2016

 Issue 578799  has been merged into this issue.

Comment 25 by fer...@gmail.com, Oct 3 2016

Has there been any progress on this? Does anyone known of a workaround?

Comment 26 by behdad@google.com, Oct 5 2016

No progress.  Dominik, should we look into this together end of month?
Yes, gladly.

Comment 28 by kojii@chromium.org, Oct 17 2016

Further analysis on FontPlatformData/SimpleFontData indicated that my concern on it being one of the causes for this bug was wrong, so hopefully supporting trak alone can fix this. Sorry for the noise.
Rather as notes to self, there are several aspects to this:

1) Our Mac system font matching would need to simulate the switching between SF Text and SF Display, the result of this is an NSFont/CTFont that we use to instantiate a new FontPlatformData and inside this, an SkTypeface, using SkCreateTypefaceFromCTFont. 
2) Trak table support is needed for the case where we stay inside the same font face, i.e. as long as we stay within SF Display and SF Text, we need trak support. For this, HarfBuzz needs to shape the request at the requested point size, and we need to way/API to specify this to HarfBuzz.
3) We either need to synchronize Skia and HarfBuzz to both switch the face for one CTFont on different sizes, or keep the exact same one. If the exact same one is kept, we need to keep two different SkTypefaces for this case in Blink.


Looking at the github project readme pages, Chrome 55 already gets different fonts for the body text and the headlines. SF Text for the text, and SF Display for the headlines, this part preliminarily seems working.

Behdad suggests diagnostics:
Inside hb-coretext, create a new CTFont each time, with an approximated point size, based on the scale factor that came in. The assumption would be, that if we create a new CTFont each time, this would bring default support for trak at the right approximated PT size.

If this approach works, then we need an API on HarfBuzz to set the pt size for what we want to get shaped, and HarfBuzz would internally create a new CTFont at requested point size.


Yeah, I should have written #28 better. As far as I checked, we create one  SKTypeface (and thus CTFont) for each size, only for Mac. The SKTypeface sharing is enabled only on other platforms.
and checked with tzik@ we do so.
Oh I see, so we don't use the CTFont in HB. Understood the problem.

Comment 34 by o...@github.com, Jan 23 2017

Any update on this?
Status: ExternalDependency
Depends on https://github.com/behdad/harfbuzz/issues/360 and a brief update in that bug.

Comment 36 by drott@chromium.org, Feb 22 2017

I've asked Behdad for an update in the github issue. We would need HarfBuzz to pass the correct pt size to CoreText and instantiate a CTFont at this size for shaping so that the CoreText backend can take 'trak' table data into account.
Will finish it this quarter for sure; hopefully next week.
Cc: js...@chromium.org
Project Member

Comment 39 by bugdroid1@chromium.org, Oct 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32f8cd34973f66a76c6b33d501609d5e73ef1765

commit 32f8cd34973f66a76c6b33d501609d5e73ef1765
Author: Dominik Röttsches <drott@chromium.org>
Date: Wed Oct 11 12:40:19 2017

Move fixed resolution units to platform/

And rename CSSHelper to CSSResolutionUnits.

Preparation for fixing  issue 627609  since HarfBuzz will need to know a
points size at which the hb_font is going to be rasterized.

No functional change.

Bug:  627609 
Change-Id: I46431b7195346de7528fc44f59541d431a0ddf5d
Reviewed-on: https://chromium-review.googlesource.com/712154
Reviewed-by: Morten Stenshorne <mstensho@opera.com>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507959}
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/animation/SVGLengthInterpolationType.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp
[rename] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/CSSResolutionUnits.h
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/CSSToLengthConversionData.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/MediaValues.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/MediaValuesDynamic.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/cssom/CSSUnitValue.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/svg/SVGLengthContext.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/core/svg/SVGSVGElement.cpp
[modify] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/platform/BUILD.gn
[copy] https://crrev.com/32f8cd34973f66a76c6b33d501609d5e73ef1765/third_party/WebKit/Source/platform/ResolutionUnits.h

Status: Started
Behdad and I have a local fix for this issue, based on HarfBuzz' fix to https://github.com/behdad/harfbuzz/issues/360 .

It turns out that tracking for the '.SF NS Display' and '.SF NS Text' fonts can only be activated through CTFontCreateUIFontForSize. Other than this call there is no CoreText API or FontDescriptor attributes that would allow us to control tracking.

So the nature of the HarfBuzz fix is detecting when we come into HarfBuzz with one of those system fonts, and recreate a CTFont at the right size using this API call, in order to activate tracking.

It is important to note that a CoreText "point" unit is a device independent size, defined similar to a CSS pixel, it is not a typographic point size unit. 

At the moment, the API for telling HarfBuzz about the actual visual font size to be used on the screen is to convert from CSS Pixel to typographic points. HarfBuzz then converts this to what CoreText expects, so converts its to CoreText "points".

We're going to roll HarfBuzz after the Chrome branch and upload a small change on the Blink side to pass the point size to HarfBuzz.






Blockedon: 774502
Unfortunately this is not fully fixed if the font is in italics. It still shows the wide letter spacing.

Chrome vs Safari

https://i.gyazo.com/0fa111dcd4c638d7ae3a93e0b14c6595.png
Right. We don't match the Italic.

Sign in to add a comment