New issue
Advanced search Search tips

Issue 774034 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocking:
issue 722462



Sign in to add a comment

zwsp is missing in ShapeResult on some Macs

Project Member Reported by kojii@chromium.org, Oct 12 2017

Issue description

Shape the following string with kRtl:
UChar string[] = {kZeroWidthSpaceCharacter,
                    kZeroWidthSpaceCharacter,
                    0x0627,
                    0x0631,
                    0x062F,
                    0x0648,
                    kZeroWidthSpaceCharacter,
                    kZeroWidthSpaceCharacter};

1. mac_chromium_rel_ng (10.9, it seems)
#chars=8, #glyphs=4, dir=1,
runs[1]{
  0:{start=2, #chars=6, dir=5, glyphs[4]{
    0:{char=3, glyph=356}
    1:{char=2, glyph=268}
    2:{char=1, glyph=272}
    3:{char=0, glyph=240}}}}

2. my MacBook (10.12.6)
#chars=8, #glyphs=5, dir=1,
runs[2]{
  0:{start=2, #chars=6, dir=5, glyphs[4]{
    0:{char=3, glyph=356}
    1:{char=2, glyph=268}
    2:{char=1, glyph=272}
    3:{char=0, glyph=240}}}
  1:{start=0, #chars=2, dir=5, glyphs[1]{
    0:{char=0, glyph=65535}}}}

3. Linux
#chars=8, #glyphs=8, dir=1,
runs[1]{
  0:{start=0, #chars=8, dir=5, glyphs[8]{
    0:{char=7, glyph=3}
    1:{char=6, glyph=3}
    2:{char=5, glyph=1392}
    3:{char=4, glyph=1372}
    4:{char=3, glyph=1374}
    5:{char=2, glyph=1364}
    6:{char=1, glyph=3}
    7:{char=0, glyph=3}}}}

Differences:
A. On mac bots, we're missing glyph_data for the zwsp. We rely on start_index of the last run (in RTL) to know the start range, and this is blocking issue 722462 (and will affect LayoutNG line breaker.)
B. On my local MacBook, start_index is 0, but character index 1 is missing.
 

Comment 1 by kojii@chromium.org, Oct 12 2017

Blocking: 722462
Components: Blink>Fonts
drott@, do you have any ideas why this is happening? Is this likely in HB, or in our code?
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 12 2017

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

commit 7ad22ea599918d0cec377261b385374e65bb02d5
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Oct 12 20:18:16 2017

Fix ShapeResult::StartIndexForResult on Mac

On Mac, ShapeResult may not contain RunInfo/HarfBuzzGlyphData
for Zero Width Space at the logical start in RTL.

This patch changes StartIndexForResult to compute from the first
RunInfo, so that it is not affected by the logical start character
in RTL.

Bug: 722462,  774034 
Change-Id: I10051467e9915cfe28d0cd8ebb474263dd775a00
Reviewed-on: https://chromium-review.googlesource.com/714739
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508436}
[modify] https://crrev.com/7ad22ea599918d0cec377261b385374e65bb02d5/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp
[modify] https://crrev.com/7ad22ea599918d0cec377261b385374e65bb02d5/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp
[modify] https://crrev.com/7ad22ea599918d0cec377261b385374e65bb02d5/third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.cpp
[modify] https://crrev.com/7ad22ea599918d0cec377261b385374e65bb02d5/third_party/WebKit/Source/platform/fonts/shaping/ShapeResult.h

Comment 4 by e...@chromium.org, Oct 12 2017

Status: Fixed (was: Assigned)

Comment 5 by drott@chromium.org, Oct 13 2017

Thanks for fixing this, Koji. I am assuming that the results on Mac and Linux are different due to different fonts, and only the font on Mac exposes this problem.

Comment 6 by kojii@chromium.org, Oct 13 2017

Yeah, or by the underlying APIs. Glyph id 3 is usually .notdef, according to Adobe guideline, so Linux handles as notdef glyph, while Core Graphics may return the glyph is missing, or Apple fonts don't have them in CMAP.

The CL fixes StartIndexForResult but does not change how we build GlyphData. I guess you're ok to have the differences?

Comment 7 by drott@chromium.org, Oct 13 2017

Yes, LGTM.
The still disabled tests should now work after  issue 894354  has been addressed.

Sign in to add a comment