The ligature rendering is failed at GDI path. See the screenshot, you can see a square after "*profile" in the heading text. If I use customized build that is always using Uniscribe, the text is rendered correctly. This is a regression of bug 370588. Note that the font is new system fonts for Vista-ja. It's so important bug for us.

What does GetCharacterPlacement return in this case?

aLength == 8 and nGlyphs == 8 * p r o fi l e glyphs: 0x0D 0x53 0x55 0x52 0x72 0x4F 0x48 0x00 advance: 30 29 20 29 28 12 28 00

This fixes this bug. Even if we don't set GCP_LIGATE, the GetCharacterPlacement uses ligatures for Meiryo font that is a new Japanese system font. We should set GCP_LIGATE and if the text has ligature, we should fallback to uniscribe.

Comment on attachment 257510 [details] [diff] [review] Patch rv1.0 doesn't this have the potential for a huge performance hit? many fonts will have ligatures...

(In reply to comment #4) > (From update of attachment 257510 [details] [diff] [review]) > doesn't this have the potential for a huge performance hit? many fonts will > have ligatures... Don't we support the ligatures? And if the text doesn't have ligatures, the patch doesn't have the damage.

We can get the un-ligatured glyphs from |GetGlyphIndices|. I.e., the glyph array should be same as text length. So, we can compare the glyph arrays. Cannot we know the text-glyph mapping from it? (I think that we need to help only the simple cases, for other cases, we can fallback to uniscribe.)

I don't think we can figure out the text-glyph mapping just from two glyph index arrays, one with ligatures and one without. What about this example: Glyphs without ligatures: 1 2 3 4 5 Glyphs with ligatures: 6 7 We just don't know which glyphs combined to form which ligatures. We could maybe take a guess by looking at the advance widths (e.g. if the width of "1 2 3" is close to the width of "6" then 6 is probably a ligature for 1 2 3), but it would only be a guess. I wouldn't want to depend on that approach. So right now I don't see how we can handle ligatures without taking the Uniscribe path. I think we should stop using GetCharacterPlacement and use GetGlyphIndices/GetTextExtentExPointI for the fast path.

how did this work before roc's patch? can you write some test cases?

This patch is including the patch of bug 372636. My concept is that we handle the ligatured text in GDI path only for the simple cases. Other cases should go to Uniscribe. How about this approach? # if Stuart like this approach, I'll create testcases...

You said this was a regression from roc's patch. What changed? Why are we using GDI now when we used Uniscribe before?

And I think that we should support ligatures in all cases. Because if GDI path doesn't support it, some text will be rendering as different glyphs. E.g., p { font-family: westernfont, japanesefont; } <p>fix</p> <p>fix日本語</p> in this case, the "fi" of first paragraph will be rendered by GDI path, so it is *not* rendered as ligature. But in second paragraph, the text will be rendered by Uniscribe. Therefore, the "fi" will be rendered as ligature.

(In reply to comment #10) > You said this was a regression from roc's patch. the old textrun doesn't draw with text length, it used the glyph counts. but new textrun is not so. therefore, we have a regression.

(In reply to comment #11) > And I think that we should support ligatures in all cases. Because if GDI path > doesn't support it, some text will be rendering as different glyphs. > > E.g., > > p { > font-family: westernfont, japanesefont; > } > > <p>fix</p> > <p>fix日本語</p> > > in this case, the "fi" of first paragraph will be rendered by GDI path, so it > is *not* rendered as ligature. But in second paragraph, the text will be > rendered by Uniscribe. Therefore, the "fi" will be rendered as ligature. I mostly agree with this argument, but still, I think we should have a path where ligatures are completely disabled, so we can see what the cost of enabling them is.

I have a report in bugzilla-jp, it is a problem... http://www.w3.org/TR/CSS21/text.html#propdef-letter-spacing > When the resultant space between two characters is not the same as the default space, user agents should not use ligatures. This is out of this bug, but we need to think for this problem, we need shaping, but the shaping uses ligatures...

This patch stops to support the ligatures in both GDI path and Uniscribe path. But this also has ligatures support code.

fix some nits, and added a comment.

Comment on attachment 259342 [details] [diff] [review] Patch rv3.1 I think that we should take a roc's patch of bug 372631, first. But we need to change the uniscribe path after it, like this patch.

My profiling showed that GetCharacterPlacement is very slow, at least on some systems. It would be great if there was a way to tell ahead of time whether a font supports ligatures or not. As it is, you can expect this patch to regress Tp (after I've fixed it in bug 372631) unless ligatures are disabled. I think we should delay trying to enable ligatures until the new textframe is landed; then we can see what the cost is really going to be. GetAdditionalClusterLengths is quite tricky and I'm not convinced it's correct. How about this example: String: ffllop Simple glyphs: "f" "f" "l" "l" "o" "p" Ligated glyphs: "ffl" "l" "o" "p" I believe GetAdditionalClusterLengths is going to succeed. It decides that the text was ligated as "ff", "l", "lo", "p". How about this approach? Break down the string into space-separated words and process each word separately. Find the start of the first ligature and the end of the last ligature in the word. We can tell if there's only one ligature --- there's only one ligated glyph between those two points, and of course we can tell if there's zero ligatures. If there's two ligatures with some non-ligatures in between we can tell that too --- the ligated glyphs will look like X <...> Y and the simple glyphs will look like A B <...> C D with the same sequence inside. Basically if we can find the non-ligated glyphs inside the simple glyph string then we know the ligature boundaries. But I'm not really sure we want to go with this complexity.

roc: thank you for the comment. (In reply to comment #19) > My profiling showed that GetCharacterPlacement is very slow, at least on some > systems. It would be great if there was a way to tell ahead of time whether a > font supports ligatures or not. maybe, we can do it if we check the font file directly. I will try it after bug 366664 and bug 362093.

by removing GetCharacterPlacement, bug 347011 doesn't occur in GDI path. Roc said that the APIs don't have good performance, therefore, it may be good that we should support ligatures only on Uniscribe path.

The original bug report here is fixed right? We display the text OK, just without ligatures.

Yeah, but we should disable the ligatures in Uniscribe path, right?

Note that we need to Uniscribe path without ligatures support for spacing rendering, see comment 15. Now, we disable the ligatures in Uniscribe path too, we can test the code in nightly builds.

We do not want to disable ligatures in the Uniscribe path.

why? we have a bug as comment 13 until disabling the ligatures in Uniscribe path. And even if we support the ligatures in future build, we also need to disable the ligatures if the text has spacing (comment 15). But we don't have a code for disabling the ligatures in Uniscribe path.

(In reply to comment #19) > My profiling showed that GetCharacterPlacement is very slow, at least on some > systems. It would be great if there was a way to tell ahead of time whether a > font supports ligatures or not. GetFontLanguageInfo() should tell us whether a font supports ligatures at all, but I guess what we really want to know is whether the font supports ligatures for the current text, otherwise we will be taking the slow path much too often.

What's the status here? I'm not sure what still needs to be done, if anything. The issue with ligatures being used even when CSS letter-spacing is in effect can be fixed now; see DISABLE_OPTIONAL_LIGATURES which will be passed as a textrun construction flag when letter-spacing is active. If there's a way to tell Uniscribe to disable ligatures, it would be easy to hook up. http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxFont.h#486 But this probably deserves its own bug.

oops, sorry. I forgot this. we should track the issue for uniscribe in bug 387170. -> FIXED