Bug 138213 - Text positioning is wrong in ComplexTextControllerCoreText if left-most glyph has non-zero glyph position offsets
Summary: Text positioning is wrong in ComplexTextControllerCoreText if left-most glyph...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on: 160892
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-30 10:12 PDT by Behdad Esfahbod
Modified: 2016-12-07 18:29 PST (History)
3 users (show)

See Also:


Attachments
Reduction (278 bytes, text/html)
2014-10-30 17:25 PDT, Myles C. Maxfield
no flags Details
Comparison of Safari 9 and Chrome 51 (51.84 KB, image/png)
2016-07-15 12:12 PDT, Dave Crossland
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Behdad Esfahbod 2014-10-30 10:12:14 PDT
As explained here:
https://code.google.com/p/chromium/issues/detail?id=352736

I said:

On the CoreText path, the issue still exists, AND it's both vertical and horizontal mispositioning.  The root of the problem is similar though: GlyphBuffer doesn't have the position of the leftmost glyph.  So fixing it would need wiring up a startPosition the way the HarfBuzz shaper does.

In fact, ComplexTextControllerCoreText.mm uses CTRunGetAdvancesPtr instead of CTRunGetPositionsPtr.  That itself has the bug.  From the CTRunGetAdvancesPtr docs:

"""Note that advances alone are not sufficient for correctly positioning glyphs in a line, as a run may have a non-identity matrix or the initial glyph in a line may have a non-zero origin; callers should consider using positions instead."""

Here's a test case, test on Safari and Firefox: http://behdad.org/urdu
Comment 1 Behdad Esfahbod 2014-10-30 10:30:44 PDT
Ned, since you are listening:

I noticed that if I have a, eg RLE, at the beginning of a run of text, CoreText outputs a glyph with id 65535 there.  I imagine this is to help with cluster mapping...

One way to fix this bug for Webkit and others at the same time is, if the first glyph in buffer out from CoreText has non-zero offsets, prepend one of those invisible glyphs there.
Comment 2 Myles C. Maxfield 2014-10-30 15:29:34 PDT
We already track the location of the run as a whole (using CTRunGetInitialAdvance). Seems that we could just add in the position of the first glyph (unless that is what CTRunGetInitialAdvance is already doing)
Comment 3 Behdad Esfahbod 2014-10-30 16:00:05 PDT
Correct.  That's similar to what we (Blink) did in our other controller:

  https://codereview.chromium.org/180893004

More recently, we changed GlyphBuffer API to carry both advances and offsets.
Comment 4 Myles C. Maxfield 2014-10-30 16:45:47 PDT
Reduction: لے آ
Comment 5 Myles C. Maxfield 2014-10-30 17:25:41 PDT
Created attachment 240714 [details]
Reduction
Comment 6 Myles C. Maxfield 2014-10-30 18:49:10 PDT
The problem here is that CoreText is encoding the additional horizontal space in the glyph that it is attributing to the space character. WebKit sees this and ignores the advance that CoreText supplies, and instead uses the spaceWidth() which we had cached at font creation time.
Comment 7 Myles C. Maxfield 2014-10-30 19:10:43 PDT
Selection doesn't work because ComplexTextController::advance() doesn't take the initial advance into consideration when computing widths
Comment 8 Behdad Esfahbod 2014-10-30 23:03:53 PDT
(In reply to comment #6)
> The problem here is that CoreText is encoding the additional horizontal
> space in the glyph that it is attributing to the space character.

Well, root of problem is that CTRunGetAdvancesPtr() (and even CTRunGetPositionsPtr) returns displacement from one glyph position to the next glyph position.  So it loses the difference between the advance of current glyph and next glyph's offset from its origin.


> WebKit
> sees this and ignores the advance that CoreText supplies, and instead uses
> the spaceWidth() which we had cached at font creation time.

Interesting!
Comment 9 Behdad Esfahbod 2014-10-30 23:31:18 PDT
Ok so it occurs to me that there are two separate problems here, resulting in the same wrong behavior:

  - Use of CTRunGetAdvancesPtr instead of CTRunGetPositionsPtr.  This loses the positioning of the left-most glyph in a run,

  - Replacement of glyph displacement from a space glyph to the glyph to its right, which loses the positioning of the glyph to the right of a space character.

Those are separate issue.

This normally doesn't show itself up because in left-to-right text, the leftmost glyph or the glyph to the right of space are typically base-characters and have a positioning of 0,0 relative to the glyph position.  In right-to-left text, when the last character in a word or run is a combining mark, and if the font has mark-to-base glyph positioning (GPOS feature 'mark'), then the left-most glyph or glyph to the right of space have non-trivial positioning that is lost.  We originally observed this with normal Arabic fonts.

With this Noto Nastaliq font, this happens more because the font decomposes Arabic letters with dots or other "diacritics" into multiple glyphs, the left-most one (in right-to-left order) acting like a combining mark.

Hope that helps.  Let me know if I can be of more help.
Comment 10 Myles C. Maxfield 2014-11-03 20:57:11 PST
The patch at https://bugs.webkit.org/show_bug.cgi?id=138348 fixes one of the problems here.
Comment 11 Dave Crossland 2016-07-15 12:12:12 PDT
Created attachment 283779 [details]
Comparison of Safari 9 and Chrome 51

This issue remains unresolved after several years :( 

Comparison of OS X 10.11.5, Safari Version 9.1.1 (11601.6.17), and Chrome Version 51.0.2704.103 (64-bit), rendering http://behdad.org/urdu/ (note the shaping on the right end of the last line...)
Comment 12 Myles C. Maxfield 2016-07-18 16:44:17 PDT
(In reply to comment #9)
> Ok so it occurs to me that there are two separate problems here, resulting
> in the same wrong behavior:
> 
>   - Use of CTRunGetAdvancesPtr instead of CTRunGetPositionsPtr.  This loses
> the positioning of the left-most glyph in a run,
> 
We use CTRunGetInitialAdvance() which gets this.
Comment 13 Myles C. Maxfield 2016-09-02 14:13:50 PDT
The overlapping glyphs problem showed in the screenshot is being fixed in https://bugs.webkit.org/show_bug.cgi?id=160892. After that patch lands, I'd appreciate it if you could take a look at a WebKit nightly build to see if this problem is fixed, or if there is more to do here.
Comment 14 Myles C. Maxfield 2016-09-24 15:57:57 PDT
(In reply to comment #13)
> The overlapping glyphs problem showed in the screenshot is being fixed in
> https://bugs.webkit.org/show_bug.cgi?id=160892. After that patch lands, I'd
> appreciate it if you could take a look at a WebKit nightly build to see if
> this problem is fixed, or if there is more to do here.

Any word?
Comment 15 Behdad Esfahbod 2016-12-07 18:29:14 PST
Sorry Myles, I dropped the ball on this.  Don't have a Mac nearby.  If http://behdad.org/urdu/ renders similar to what Firefox or Chrome does, it's good.

Thanks,