621918 - character clusters and ligatures vanish when rendering SVG and Canvas text-on-path

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: example of SVG textPath with character clusters

Character clusters (e.g. Indic reordered consonant+vowel combinations, Latin base letter+accent combinations, etc) are not painted when we render SVG <textPath> elements; instead, we fire "!!! ASSERTION: Cannot draw partial ligatures without a dirty rect."

The attached testcase shows lines of text that are simply rotated using a 45° transform, which works fine, and the same text rendered using <textPath>; a number of glyphs are missing from the <textPath> versions.

Comment 1

[Jonathan Kew [:jfkthame]]

(Testcase originally based on http://inkscape.org/doc/examples/i18n.svg, which illustrates that this is a particular problem for use of non-Latin text in SVG.)

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch v1 - make SVG text respect character clusters and render partial ligatures

This modifies the CharacterIterator in nsSVGGlyphFrame.cpp to iterate by clusters instead of individual characters, and provides an appropriate dirtyRect when painting a partial ligature in nsSVGGlyphFrame::FillCharacters.

(There may well be some additional places where we should pay more attention to clusters, but this appears to fix the most glaring issues - glyphs no longer disappear from the test case - and does not break any of our unit tests.)

Comment 3

[Jonathan Kew [:jfkthame]]

This will also fix bug 606022, as surrogate pairs are in effect just a particular subset of "character clusters" when iterating over the (utf-16) text.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    // if the cluster is a partial ligature, we need a bounding box to pass
+    // to mTextRun->Draw, otherwise glyphs will not be painted

I don't know why we didn't do this before, but we can eliminate that requirement by having mTextRun->Draw use aContext->GetClipExtents for the dirty rect if none was passed in.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So, please just do that and simplify the code here.

Comment 6

[Jonathan Kew [:jfkthame]]

Using aContext->GetClipExtents in gfxTextRun::Draw doesn't paint the ligatures correctly in the SVG text-on-path case. In fact, the behavior seems a bit erratic - what gets painted varies between refreshes.

I don't think the context's initial clipExtents correspond at all to the bounds of the partial ligature we're wanting to paint in this situation; rather, the bounds passed in to gfxTextRun::Draw are presumably used to *update* that clip prior to actually painting, so that we actually render the appropriate section of the ligature glyph.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix dirtyRect

Does this patch not work? I don't see how it can fail to work :-)

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to comment #7)
> Created attachment 500441 [details] [diff] [review]
> fix dirtyRect
> 
> Does this patch not work? I don't see how it can fail to work :-)

It doesn't seem to. I commented out the partial-ligature stuff in my patch, and applied this on top: the ligatures in the Indic text-on-paths in http://inkscape.org/doc/examples/i18n.svg are missing from the rendered image, and when I click around in the window they appear or disappear depending where I click - which presumably affects the context's clip when it is repainted, though it's not obvious how this relates to clipping the individual ligature glyphs on the path.

Actually, I don't see how this patch *would* help DrawPartialLigature. OK, it eliminates the assertions, but what we're really wanting here isn't just the context's "dirty rect" for redraw purposes - which may or may not bear any relation to the area of the ligature glyph we're painting - we actually want the bounding rect of the part of the ligature glyph that we're associating with the current character/cluster (or the intersection of this with the dirtyRect, strictly speaking). This patch doesn't do anything to ensure that the appropriate rect - based on the ligature glyph's bounds - is set up, does it?

(Arguably, the aDirtyRect parameter to DrawPartialLigature is misnamed; it might be better called aPartialLigatureBounds or something like that. And you can't derive that from the context's clip, AFAICS.)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

aDirtyRect really is supposed to be just a dirty rect. ClipPartialLigature is responsible for restricting the rect horizontally to contain just the partial ligature.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: reftests for clusters & ligatures in <textPath>

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to comment #9)
> aDirtyRect really is supposed to be just a dirty rect. ClipPartialLigature is
> responsible for restricting the rect horizontally to contain just the partial
> ligature.

Ah yes, I see... but it doesn't appear to work correctly for the <textPath> case. Though I don't think it's actually ClipPartialLigature that is failing, as I sometimes see *part* of the desired partial-ligature being rendered, with it being clipped incorrectly in the *vertical* direction - the horizontal clipping, which ClipPartialLigature is responsible for, seems to be correct when this happens. So it looks to me like the context's clipExtents are not properly set up (perhaps not taking into account the per-character <textPath> matrix changes?) at the time this is called.

Actually, on looking at your patch in attachment 500441 [details] [diff] [review] again, I notice that it eliminates the use of the aDirtyRect parameter to gfxTextRun::Draw()  altogether. It's passed on to DrawPartialLigature(), but not used within that method, and nothing else uses it either. Was that the intention? If so, we can go further and remove the parameter altogether.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That is the intent.

Maybe there is a bug in GetClipExtents. If so, we'd better fix it! It shouldn't be that hard to figure out. _cairo_gstate_clip_extents is responsible for transforming the device-space clip extents rectangle to user space.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 500441 [details] [diff] [review]
fix dirtyRect

Aha, got it - turns out there's a small but crucial mistake in the fix-dirtyrect patch:

>     aCtx->Rectangle(gfxRect(left/mAppUnitsPerDevUnit,
>-                            aDirtyRect->Y()/mAppUnitsPerDevUnit,
>+                            clipExtents.Y(),
>                             (right - left)/mAppUnitsPerDevUnit,
>-                            aDirtyRect->Height()/mAppUnitsPerDevUnit), PR_TRUE);
>+                            clipExtents.YMost()), PR_TRUE);
>     aCtx->Clip();

This clips away too much of the glyph we're trying to draw. Instead of clipExtents.YMost() there, I think you meant clipExtents.Height().

I'll also eliminate the now-redundant aDirtyRect parameter. Updated patches coming...

Comment 14

[Jonathan Kew [:jfkthame]]

BTW, note that <canvas> text on a path (mozTextAlongPath) suffers from the same issue - character clusters and ligatures disappear from the rendered text. Eliminating the need for a dirtyRect to be passed to gfxTextRun::Draw should fix this, too.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: testcase showing <canvas> glyph dropout problem

This example (based on attachment 273498 [details] in bug 339553) shows the canvas version of the ligatures-on-path problem: the "fi" ligature in Times Roman (at least on OS X) and most of the Devanagari glyphs fail to display.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - eliminate aDirtyRect parameter from gfxTextRun::Draw etc

This is based on your "fix dirtyRect", and extends it to eliminate the now-redundant parameter from gfxTextRun::Draw altogether. (Most callers were just passing NULL anyway.) It also ensures that DrawPartialLigature preserves the current path; without that, the patch caused breakage in canvas text-on-path.

We could go further with eliminating dirtyRect parameters from some places in nsTextFrameThebes as well (e.g. nsTextFrame::DrawText clearly doesn't need it any longer, and perhaps some of the higher-level functions only have it in order to pass down to this code), but I decided against further surgery there for now; the gfxTextRun interface seemed like a good place to limit the scope of the current patch.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - make SVG text respect character clusters

Simplified now that we don't need to pass a dirtyRect parameter.

Comment 18

[Jonathan Kew [:jfkthame]]

With these patches, the ligature glyph dropout problem in <canvas> mozTextAlongPath is also fixed. The canvas implementation still needs some character-cluster love, but I'll spin that off into a separate bug.

Comment 19

[Jonathan Kew [:jfkthame]]

> The canvas implementation still needs some
> character-cluster love, but I'll spin that off into a separate bug.

This is now bug 624359.

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 502463 [details] [diff] [review]
part 1 - eliminate aDirtyRect parameter from gfxTextRun::Draw etc

Requesting approval-2.0 for the patches here. Although this isn't a regression - we already shipped a product where clusters and ligatures in <textPath> are totally broken - it's a pretty glaring bug when it happens, and I think we should fix it for FF4.0.

Comment 21

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/8e1487c6e663 (part 1)
http://hg.mozilla.org/mozilla-central/rev/119ef8681a16 (part 2)
http://hg.mozilla.org/mozilla-central/rev/57eb3d440593 (reftests)