Closed Bug 621918 Opened 9 years ago Closed 9 years ago
character clusters and ligatures vanish when rendering SVG and Canvas text-on-path
1.96 KB, image/svg+xml
3.57 KB, patch
|Details | Diff | Splinter Review|
1.13 KB, text/html
14.78 KB, patch
|Details | Diff | Splinter Review|
9.73 KB, patch
|Details | Diff | Splinter Review|
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.
(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.)
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.)
Assignee: nobody → 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.
+ // 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.
So, please just do that and simplify the code here.
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.
Does this patch not work? I don't see how it can fail to work :-)
(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.)
aDirtyRect really is supposed to be just a dirty rect. ClipPartialLigature is responsible for restricting the rect horizontally to contain just the partial ligature.
Attachment #500602 - Flags: review?(roc) → review+
(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.
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 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...
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.
Summary: character clusters vanish when rendering SVG text-on-path → character clusters and ligatures vanish when rendering SVG and Canvas text-on-path
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.
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.
Attachment #502463 - Flags: review?(roc) → review+
Simplified now that we don't need to pass a dirtyRect parameter.
Attachment #502464 - Flags: review?(roc) → review+
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.
> 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 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.
Attachment #502463 - Flags: approval2.0?
Attachment #502463 - Flags: approval2.0? → approval2.0+
Attachment #502464 - Flags: approval2.0? → approval2.0+
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)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.