[HarfBuzz] Should MarkToBase attachment zero the mark advance?

[Behdad Esfahbod]

Pushed this one out.  Thanks.

b

On 09/24/10 12:15, Jonathan Kew wrote:
> On 23 Sep 2010, at 01:06, Behdad Esfahbod wrote:
> 
>> Hi Jonathan,
>>
>> I'm investigating this.  Can you send me the exact version of DejaVuSansMono
>> and the sample text reproducing this please.
>>
> 
> The attached version of DejaVu Sans Mono should serve.
> 
> Testcase:
> 
> data:text/html,<div style="font-family:DejaVu Sans Mono">foobar<br/>fo&#xf3;bar<br/>foo&#x301;bar
> 
> The third line demonstrates the issue (though not in current Firefox beta, as we've patched it locally).
> 
> Jonathan
> 
> 
> 
> 
> 
> 
> 
> 
>> Thanks,
>> behdad
>>
>> On 08/23/10 13:46, Jonathan Kew wrote:
>>> Hi Behdad,
>>>
>>> I notice that the code in MarkArray::apply() includes the following:
>>>
>>>    hb_internal_glyph_position_t &o = c->buffer->pos[c->buffer->i];
>>>    o.x_advance = 0;
>>>    o.y_advance = 0;
>>>    o.x_offset  = base_x - mark_x;
>>>    o.y_offset  = base_y - mark_y;
>>>    o.back      = c->buffer->i - glyph_pos;
>>>
>>> i.e. in addition to setting x_offset and y_offset so as to position the mark glyph, it also explicitly overrides any existing x_advance and y_advance values for the glyph, settings them to zero. In many cases, this is harmless (though redundant), as mark glyphs are typically designed with zero advance anyway.
>>>
>>> However, I'm seeing problems in several monospaced fonts as a result of this; an example is DejaVuSansMono.ttf. Here, the mark glyphs have the same (non-zero) advance as the rest of the glyphs -- logical, I suppose, for a fixed-width font. There is a GPOS 'mark' feature that positions diacritics such as the U+03xx range. The trouble is that this feature actually executes TWO lookups for these glyphs: first, it does a MarkToBase Attachment (type 4), to place the diacritic over the base glyph, AND THEN it does a Single Adjustment that modifies the advance of the diacritic glyph by the negative of its original advance. This is clearly intended to make it become zero-width; but because harfbuzz has already zeroed the advance, it now ends up with a NEGATIVE advance, and the result is that the next glyph completely overprints the accented character.
>>>
>>> So unless you know of specific reasons why it is necessary to zero the x_advance and y_advance values here (are there examples of fonts where the rendering is incorrect without this?), I'd suggest removing those two lines, as in the attached patch.
>>>
>>> With that change, I'm getting the expected rendering with DejaVuSansMono. (The same issue occurs with the Consolas font on Windows7, for example.)
>>>
>>> JK
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>