Closed Bug 445087 Opened 16 years ago Closed 16 years ago

FF3 cuts off the end of the last letter of a link

Categories

(Core :: Graphics, defect, P2)

Product:
Core
Component:
Graphics
Platform:
x86
Windows XP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: sh, Assigned: jfkthame)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(17 files, 5 obsolete files)

Example of text with last letter incomplete
16.54 KB, image/gif
Details
Comparison of fonts between Windows
99.40 KB, image/gif
Details
numbers cut off
150.36 KB, image/jpeg
Details
sample file that illustrates the glyph clipping problem
1.32 KB, text/html
Details
the sample file displayed without using ClearType
24.91 KB, image/png
Details
same sample file displayed with ClearType enabled
71.15 KB, image/png
Details
test program for cairo-win32 to demonstrate ClearType glyph clipping issue
5.39 KB, text/plain
Details
enlarged screenshot of the cairo test program's output
77.78 KB, image/png
Details
output of the cairo test program with patch applied to cairo-win32-font.c
78.15 KB, image/png
Details
the sample HTML file displayed by Firefox using patched Cairo
71.50 KB, image/png
Details
reftest 355548-3 screenshot (with unpatched Cairo)
20.08 KB, image/png
Details
reftest 355548-3 screenshot (with patch)
20.35 KB, image/png
Details
example of MathML rendering without padded glyph extents
15.79 KB, image/png
Details
the same MathML with Cairo patched to pad the glyph ink extents
15.87 KB, image/png
Details
example of ClearType antialiasing
51.01 KB, image/png
Details
revised as per comment #71 [Checkin: Comment 74+75]
5.35 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
patch for 1.9.1
4.38 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0 FF3 seems to cut off the last letter of links in certain circumstances. Please see reference URL, take a look at the session times section in the grey box. You should see that the 'm' in 'am' and 'pm' have the ends of the 'm's cut off. Since this is ajax content, firebug can be used to view the source. Using default setup/fonts. Font is defined "Arial, Helvetica, sans-serif". Reproducible: Always Steps to Reproduce: 1. Browse to http://www.readingcinemas.com.au/audev/cinemas/chirnsidepark.asp 2. Take a look at the grey box about 1/3 of the way down, with the session times in it. 3. Observe that the last letter of links has been cut off Actual Results: The last letter (m) of each link is cut off. Expected Results: The full 'm' should show.
Seems to WFM. Can you try this in safe-mode first? If the problem persists, can you please post a screenshot of what you are seeing?
Yep, I can still see the whole letter on OS-X, XP and Ubuntu. Did you try safe-mode yet? http://support.mozilla.com/en-US/kb/Safe+Mode
Yes, the screenshot was taken in safe mode. This was originally reported by someone running FF3, and I can see it. I have just tested on a VM with a fresh XP install with FF3, but cannot reproduce. It looks as though it could be a font anit-aliasing setting in Windows that makes these fonts look a bit 'smoother', but causes this problem in FF3. I'll attach a screenshot.
I have found that this is an issue when "ClearType" is enabled on the XP machine. To reproduce: 1. Go to Control Panel - > Display 2. In the dialog that opens, click the 'Appearance' tab. 3. Click the 'Effects...' button 4. Check 'Use the following method to smooth edges of screen fonts' 5. Select 'ClearType' from the drop down. This causes the font problem mentioned on: http://www.readingcinemas.com.au/audev/cinemas/chirnsidepark.asp when using FF3.
Attached image numbers cut off —
Another show case; it is win 2003 with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1 When I turn ClearType off everything looks ok and when turned on it seems that last pixel of number is missing.
Here is how to replicate on FF 3.0.1 I found that opacity plays role there. <html> <body> <table border='1' style="font-family: arial; font-size: 8pt;" > <tr> <td style="opacity: 0.95;">1.611</td> <td>1.611</td> </tr> </table> </body> </html>
I also have this problem in the latest nightly FF build on WinXP with ClearType on. Here is my code to reproduce: <html> <head> <style> body { opacity: 0.999; font-family: sans-serif; font-size:11px; } </style> </head> <body> I<br /> P<br /> S<br /> 1<br /> l </body> </html>
Is not this related to bug #397423
This is probably not related to bug 397423. Whether Cleartype is in "natural widths" mode or not, it should be returning metrics that don't cut off the end of the 'm'.
I tried this on Vista both with and without ClearType "natural widths", and saw problems in both cases. As predicted by roc, enabling that mode does not resolve the issue, although it does alter the particular instances where it shows up most obviously.
WFM Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 Can anyone provide additional URLs where this occurs, or possible upload some testcases? Both of the testcases above WFM.
This file displays the uppercase alphabet in sans-serif 11px (Arial by default, I believe), with varying color/opacity settings. The last line illustrates the ClearType clipping issue; see screenshots to follow.
Assignee: nobody → jfkthame
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 359252 [details] same sample file displayed with ClearType enabled The sample shows the alphabet in several ways: (1) normal black (for reference); (2, 3) gray, and black with opacity 0.5 (with visually indistinguishable results); (4) opacity 0.999 (visually indistinguishable from opaque black); (5) alternating black and gray glyphs; (6) alternating opacity 0.999 and 0.5. The last two lines should look virtually identical, but with ClearType enabled, many glyphs in the last line get clipped on the right-hand edge. This is especially noticeable with "I", which disappears almost completely, and with "P", which loses the right edge of its bowl, but many other glyphs show some minor damage as well when compared to the versions in the previous line.
The problem arises when the Cairo win32 backend decides that it cannot draw the glyphs directly using ExtTextOut and ends up drawing via _cairo_surface_fallback_show_glyphs. This function clips the area being drawn, based on glyph rectangles accessed through _cairo_scaled_font_glyph_device_extents. This "fallback" path is used whenever transparency is involved (among other things). The glyph extents used for clipping are obtained from GDI in the function _cairo_win32_scaled_font_init_glyph_metrics. This uses GetGlyphOutlineW, which returns a GLYPHMETRICS record that includes the height and width of the glyph's "black box" (described as "the smallest rectangle that completely encloses the glyph"). The glyph-painting done by the Cairo surface gets clipped to the containing rectangle of all the glyph "black boxes" for the run being painted. The problem arises because the "black box" returned by GetGlyphOutlineW is the "ideal" box that should contain the glyph, but does not take account of the fact that antialiasing, especially with ClearType, may want to make some use of pixels that are outside this "ideal" black box. In particular, the ClearType rendering often seems to "bleed" into the following pixel beyond the nominal bounding box. This doesn't usually show up in running text, as the clipping is applied to the overall bounding box of the glyph run, but it may affect the last character of the run (as in the original bug report). The sample file uses alternating levels of opacity in order to break up the runs and make the issue more obvious.
To demonstrate that this is an internal Cairo/Windows problem, not related to how we interact with Cairo, I have written a small test program that draws glyphs in various ways. Screenshots of the output will be attached. The program draws glyphs from Arial at 11px size, in black, gray, and black with alpha 0.5 (appears as gray). Each time, it draws the glyphs twice, once as a single run and the second time as individual glyphs.
Screenshot of the cairo test program, running on XP SP3 with ClearType enabled. The last line, showing glyphs drawn individually with 50% alpha, demonstrates the clipping problem. This line should look identical to the preceding one, which shows the same glyph array, drawn with the same cairo source settings (color/alpha); the only difference is that the last line is drawn one glyph at a time rather than using a single cairo_show_glyphs call for the whole line.
To avoid (or at least greatly reduce) the risk of clipping antialiasing pixels that fall beyond the nominal "black box" of the glyph, I propose that when _cairo_win32_scaled_font_init_glyph_metrics uses GetGlyphOutlineW to get the glyph metrics, it should add one pixel of "slop" to the gmBlackBoxX value that is returned. This will mean that if ClearType does cause the glyph image to "bleed" beyond its nominal edge, the pixels will still be drawn.
This screenshot shows the sample file (attachment 359250 [details]) displayed with the "slop" pixel added to the glyph black box width in cairo-win32-font, showing that this resolves the ClearType glyph clipping issue when opacity is involved.
Attachment #359260 - Flags: review?(roc)
Do we need to adjust gmptGlyphOrigin leftward to take into account antialiased pixels on the left edge of the glyph?
I considered that (see the comments in the code), but have not seen any instances where this is an issue in practice, so I thought it better to leave it as is for now, at least. I don't think there'd be any harm in such an adjustment (actually, I included it in the patch at one point, and didn't see any ill effects), but every pixel we add increases (slightly) the amount of work we're doing whenever we draw. In theory I guess the same applies at the top and bottom, too. But it doesn't seem to be an issue in actual practice.
Another question is whether we should make the same adjustment to the returned metrics in every case where cairo-win32-font calls GetGlyphOutlineW(). There are about such 4 places in the code, IIRC, and I have only patched in one place at the moment, as this is the one that underlies the problems we've seen (both in Firefox and in the cairo test program I wrote). I suppose we could try to analyze the circumstances where the other call sites would be used, and figure out whether similar visual problems could arise; there may be similar latent glitches just waiting for the exact right combination of factors to use those code paths. I haven't attempted this (yet) but it's something to keep in mind.
I'm nominating this as a potential blocker for FF 3.1 because I think the "cut-off" text seen in the screenshots (see first three attachments) is not acceptable for the release. It's a problem affecting real websites, and every user on Windows with ClearType enabled has the potential to encounter it.
Flags: blocking-firefox3.1?
Update to comment #25: I've been running a bunch more reftests, and have now seen a handful of cases where adding a "slop" pixel on the left as well (i.e., subtracting 1 from metrics.gmptGlyphOrigin.x and then adding 2 to metrics.gmBlackBoxX) resolves reftest failures with ClearType enabled. So I think we should include that in the patch. This helps with a number of the reftest problems Ehsan has encountered recently. I'll refresh the patch in a while, still running more tests here.
--> Core::Gfx
Assignee: jfkthame → nobody
Component: General → GFX: Thebes
Flags: blocking-firefox3.1?
Product: Firefox → Core
QA Contact: general → thebes
Yeah, let's fix this for 1.9.1.
Flags: blocking1.9.1+
Here's one thing I don't understand. For performance reasons we try to avoid getting the glyph metrics when the font size (in device pixels) is below browser.display.auto_quality_min_font_size --- we just assume the glyphs don't spill out of the font bounding box. So I don't understand how your patch actually fixes the bug!
The clipping doesn't happen because of our use of glyph metrics; it's entirely internal to Cairo, when it is drawing to an ARGB canvas rather than directly to the window with GDI; this is triggered by the opacity setting.
(for ARGB please read RGBA, of course!)
Oh, yikes. I see. I wonder how slow that is.
OK, looking forward to a new patch. I think Cleartype never does antialiasing in the vertical direction so we shouldn't need vertical slop. We will need to dress the patch up like your other cairo patches.
I'm now adding a pixel on both left and right of the black box from GetGlyphOutlineW, as there are a few reftests that show a difference on the left side as well. Added patch file to the Cairo dir, and entry to gfx/cairo/README, so this should be ready to go. I was going to add a reftest, but realized that's pointless; there are already a number of reftests in layout/reftest/text (e.g., the soft-hyphen ones) that demonstrate this issue if ClearType is enabled; and if ClearType isn't enabled then any new test wouldn't fail either.
Assignee: nobody → jfkthame
Attachment #359260 - Attachment is obsolete: true
Attachment #359411 - Flags: review?(roc)
Attachment #359260 - Flags: review?(roc)
Attachment #359411 - Flags: review?(roc) → review?(vladimir)
Comment on attachment 359411 [details] [diff] [review] updated to add slop on both sides of the glyph; added patch file to gfx/cairo directory Probably better for Vlad to review this.
BTW, note that this patch is written to land *after* the patch for bug 475092 (recently updated for re-landing, following test failure the first time). Both patches modify the same place in gfx/cairo/README, so they won't apply cleanly out of order.
Comment on attachment 359411 [details] [diff] [review] updated to add slop on both sides of the glyph; added patch file to gfx/cairo directory Yeh, this looks fine
Attachment #359411 - Flags: review?(vladimir) → review+
The test at bug 469208 showed that ClearType antialiasing can "bleed" even into the second pixel beyond the nominal black box (see comment #7 there). I have therefore updated this patch to add two pixels on the RHS of the box returned by Windows. With this update, that test finally passes. Marking for re-review as this is a further adjustment to the actual code.
Attachment #359411 - Attachment is obsolete: true
Attachment #359559 - Flags: review?(vladimir)
Pushed http://hg.mozilla.org/mozilla-central/rev/d679ac3a8de0
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Backed out due to test failures: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox /1233822698.1233828629.21623.gz Some of those, certainly the crashtest failures, are random, but the SVG mochitest text failures and the reftest failure seem like real regressions from this patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
Whiteboard: [needs 191 landing] → [needs new patch]
Hmmm. A number of existing reftests will fail if ClearType is enabled (this is not new), but that one seems to fail even without ClearType. Wonder why I didn't run into that before. Will re-test on various systems, and look at the SVG ones as well, but I'm guessing it may not be ready for re-landing until after FOSDEM.
Unfortunately I backed out the wrong changeset. Fortunately peterv backed out the right changeset later.
Regarding the SVG mochitest failures, I think many (if not all) of these reflect assumptions in the test that may break down in a ClearType-smoothed world. For example, we have is(text1.getExtentOfChar(0).width, text1.getSubStringLength(0, 1), "text1 char 0 extent width"); which seems to be saying that the "extent width" of a glyph should be the same as its advance. This is not true in the general case. It happened to be true for the test (until now) because the document uses the monospace font, so swashes etc that extend beyond the origin/advance are unlikely. However, with ClearType it suddenly becomes much more likely that a glyph - even a simple monospaced one - will paint beyond its advance width, and this patch adds "padding" to the glyph extent in order to allow for this and avoid clipping during painting. The same issue explains why things like p.x = 5 + 3*charWidth + 0.1; is(text1.getCharNumAtPosition(p), -1, "text1 finding no char on right"); now fail; the getCharNumAtPosition() function is using the glyph extent to determine what character is at the given position, and our glyph extents are now slightly larger than previously expected. To some extent, we could try to resolve this by not "padding" the actual glyph-extent metrics that Cairo is storing; instead we'd add those pixels whenever the extents are used to paint/clip/invalidate/etc, but not when hit-testing, for example. However, this is problematic when we expose the glyph extents to SVG or JavaScript, as we don't know whether they are going to be used for a purpose where the "padding" is necessary to avoid ClearType clipping or visual artifacts, or a purpose where "nominal" glyph bounds that ignore antialiasing pixels would be preferred. We could, I suppose, make the padding of glyph extents conditional on whether ClearType is currently enabled, but this would add an extra test in a potentially inner-loop place, and doesn't really resolve anything, it would only defer the test failures until the unit tests start running on machines with ClearType active. On balance, therefore, I'm inclined to either mark the test as a known-fail on Windows, or else make the conditions that are being tested a little less strict. WDYT?
(In reply to comment #45) > Regarding the SVG mochitest failures, I think many (if not all) of these > reflect assumptions in the test that may break down in a ClearType-smoothed > world. > > For example, we have > > is(text1.getExtentOfChar(0).width, text1.getSubStringLength(0, 1), "text1 > char 0 extent width"); yeah, that is clearly a bug in the test. What if we used a custom font, say Ahem? I guess it could still be antialiased even if the glyph is a block which is a whole number of device pixels on each side. Could we at least assert that the extent width is >= the substring length? > The same issue explains why things like > > p.x = 5 + 3*charWidth + 0.1; > is(text1.getCharNumAtPosition(p), -1, "text1 finding no char on right"); > > now fail; the getCharNumAtPosition() function is using the glyph extent to > determine what character is at the given position, and our glyph extents are > now slightly larger than previously expected. I think getCharNumAtPosition should use advance widths instead of glyph extents.
(In reply to comment #46) > yeah, that is clearly a bug in the test. What if we used a custom font, say > Ahem? I guess it could still be antialiased even if the glyph is a block which > is a whole number of device pixels on each side. Could we at least assert that > the extent width is >= the substring length? That depends what "extent" means here... if it's supposed to refer to the bounding box of the glyph's "ink", then it could equally well be smaller than the advance. If we use a specific custom font, then we can probably assert something reasonable about it, but not necessarily with pixel-level precision that will apply to all platforms, as the platforms do not rasterize identically. > I think getCharNumAtPosition should use advance widths instead of glyph > extents. That seems reasonable to me. However, that's really a separate issue from this bug. There's still a reftest I need to look at tomorrow, I think, but once that's resolved I'd suggest we mark this aspect of the mochitest as "todo", re-land this patch, and open a new bug about getCharNumAtPosition.
(In reply to comment #47) > (In reply to comment #46) > > > yeah, that is clearly a bug in the test. What if we used a custom font, say > > Ahem? I guess it could still be antialiased even if the glyph is a block which > > is a whole number of device pixels on each side. Could we at least assert that > > the extent width is >= the substring length? > > That depends what "extent" means here... if it's supposed to refer to the > bounding box of the glyph's "ink", It is. > then it could equally well be smaller than the advance. Not with Ahem, though. > If we use a specific custom font, then we can probably assert something > reasonable about it, but not necessarily with pixel-level precision that will > apply to all platforms, as the platforms do not rasterize identically. That's OK. I think we could assert the extend width >= the substring width. > > I think getCharNumAtPosition should use advance widths instead of glyph > > extents. > > That seems reasonable to me. However, that's really a separate issue from this > bug. There's still a reftest I need to look at tomorrow, I think, but once > that's resolved I'd suggest we mark this aspect of the mochitest as "todo", > re-land this patch, and open a new bug about getCharNumAtPosition. We can't mark it as "todo" because it will work on some platforms and todo will flag that success as an error. Better to comment out the incorrect test. Please do file that bug. You could even fix it as a gentle introduction to SVG text code :-).
The other test that fails (layout/reftests/bugs/355548-3.xml) is a mathml test, and the failure happens because the mathml code is using glyph extents (rather than advances) in certain situations for glyph spacing. Therefore, adding "padding" to the glyph extent in order to allow for ClearType pixels leads to changes in the math spacing. Simply changing the mathml code to use glyph advances may not be a good option, as the use of glyph extents is probably a deliberate choice to cope better with things like "f(x)", where the elements would otherwise look very crowded together. Accepting the changed spacing as a result of the padded glyph metrics from Cairo is also not a good option, IMO, as this will significantly affect the math spacing, especially at small sizes (as the amount of padding is constant, not proportional to glyph size). So I think this means that we'll have to modify the approach in this patch, so that when Cairo is asked for glyph extents, it still returns the original "unpadded" area. To resolve the original clipping problem, then, we'll have to add the "padding" pixels on left and right at the time when the glyph extents are used in painting (or clipping). Unfortunately, this means that we'll need a Windows-specific patch in the platform-independent Cairo "fallback" surface code, as that's where the drawing (and clipping) concerned is happening. We may well run into other places, too, where we'll have to pad the glyph extents returned on Windows. (Patching this issue "at source" in the metrics that Cairo caches was so much easier!)
(In reply to comment #49) > So I think this means that we'll have to modify the approach in this patch, so > that when Cairo is asked for glyph extents, it still returns the original > "unpadded" area. To resolve the original clipping problem, then, we'll have to > add the "padding" pixels on left and right at the time when the glyph extents > are used in painting (or clipping). But it's not correct to lie about the ink rect, that means we won't repaint enough in some circumstances. Does MathML actually look bad with the padded extents? Can you post a screenshot?
(In reply to comment #50) > Does MathML actually look bad with the padded extents? Can you post a > screenshot? I've posted screenshots from the reftest before and after applying this patch. Notice how the patched version has significantly looser spacing in a number of places. I don't think this would matter at large ("display") sizes, but it would be a problem for small ("in-line") math, where in many cases we'd get up to 3 extra pixels of space between adjacent glyphs. See the beginning of the 3rd line of the screenshot. Sorry, I can't do a nice "real math" sample right now as my Windows builds are all broken! :(
I think we should just not care about this MathML spacing issue, to be honest. It's good enough.
(In reply to comment #50) > But it's not correct to lie about the ink rect, that means we won't repaint > enough in some circumstances. I know. See bug 475968 for a nice example of what can happen in such cases (although that one is caused at a different level, not within Cairo itself like this issue). The trouble is that there are really two meanings of "ink rect" that we want, for different purposes. One is the "theoretical" extent of the glyph's ink, according to the (unhinted?) shape of the outline; this is useful for mathml spacing, for example, so that "f" doesn't crash into a following delimiter. For this purpose, the outline bounds that GDI (and unpatched cairo) gives us is the right thing to use. This might also be useful for hit-testing, in some cases where simply using origin & advance is not the best choice (e.g., trying to select zero-width diacritics), although I'm not sure we can currently do that nicely. However, for actually painting (or invalidating) the screen, we need an "ink rect" that also includes neighboring pixels that are touched (perhaps only very faintly) by the antialiasing. These pixels may be entirely outside the outline -- especially the original unhinted outline, which I think is what we're getting -- and they should not be considered for glyph spacing purposes. Patching Cairo so that it uses and returns this "extended" ink rect in all cases was an attractive idea in that it should resolve the issue for all operations that draw/clip/erase/invalidate based on the ink rects. But it will inevitably affect spacing/positioning in cases (like mathml -- not sure if we currently have others) where the ink rect is used for this, and that's an undesirable side-effect. The alternative is to allow for ClearType "pixel spread" wherever we actually make use of glyph extents to paint or invalidate. (We don't have to do this on a per-glyph basis; wherever a combined bounding box is being built for a whole glyph run, we'll just need to pad the result, not each individual glyph box.) It's not yet clear exactly how many places we may need to handle this, but I suspect that in practice it is very few -- after all, we are seeing problems in just a few specific situations, we're not seeing clipping or redraw artifacts throughout the application. This current bug can be fixed easily (the problem is entirely within cairo, not in our higher-level code; see comment 18 etc and test program). I'm trying to get a good build after merging with an updated trunk, and then I'll post the patch.
(In reply to comment #54) > I think we should just not care about this MathML spacing issue, to be honest. > It's good enough. Hmmm. I'd like to do a comparison with real math content, but need a working build first. If we do go that way, then the existing patch is correct as far as the code is concerned, and we just need to fix up or disable the problematic tests. OTOH, I can also offer a patch that fixes the problem and does not affect mathml spacing or break the svg tests. The only downside is that we might eventually find some additional places where a similar fix is needed.
(In reply to comment #55) > The trouble is that there are really two meanings of "ink rect" that we want, > for different purposes. One is the "theoretical" extent of the glyph's ink, > according to the (unhinted?) shape of the outline; this is useful for mathml > spacing, for example, so that "f" doesn't crash into a following delimiter. > For this purpose, the outline bounds that GDI (and unpatched cairo) gives us > is the right thing to use. Is that true? If we could get the precise ink rect covering the pixels rendered by the glyph (as opposed to the estimate you're producing by adding slop to every glyph), wouldn't that be adequate for MathML spacing? I'm just a little bit skeptical that we really need this version of the ink rect. I don't see why you'd ever really want to use the extents of the unhinted outline, since that's not what the true shape of the glyph is. > Patching Cairo so that it uses and returns this "extended" ink rect in all > cases was an attractive idea in that it should resolve the issue for all > operations that draw/clip/erase/invalidate based on the ink rects. But it will > inevitably affect spacing/positioning in cases (like mathml -- not sure if we > currently have others) where the ink rect is used for this, and that's an > undesirable side-effect. MathML is the only place this happens. > The alternative is to allow for ClearType "pixel spread" wherever we actually > make use of glyph extents to paint or invalidate. (We don't have to do this on > a per-glyph basis; wherever a combined bounding box is being built for a whole > glyph run, we'll just need to pad the result, not each individual glyph box.) > It's not yet clear exactly how many places we may need to handle this, but I > suspect that in practice it is very few -- after all, we are seeing problems > in just a few specific situations, we're not seeing clipping or redraw > artifacts throughout the application. I think we need to handle this in cairo because it's target-surface-specific. If we really need two different kinds of glyph extents, then we should get cairo to add API for that. Right now I'm not convinced. And if you convince me, you'll still have to convince Behdad to get it added to cairo :-).
> I think we need to handle this in cairo because it's target-surface-specific. I guess I meant font-system specific, but it's still platform-dependent and so we'd like it to live in cairo. (In reply to comment #56) > (In reply to comment #54) > > > I think we should just not care about this MathML spacing issue, to be honest. > > It's good enough. > > Hmmm. I'd like to do a comparison with real math content, but need a working > build first. Honestly, small cosmetic changes in MathML rendering are not important enough to delay a blocking1.9.1 fix. > If we do go that way, then the existing patch is correct as far as the code is > concerned, and we just need to fix up or disable the problematic tests. Let's do that, thanks.
(In reply to comment #58) > > I think we need to handle this in cairo because it's target-surface-specific. > > I guess I meant font-system specific, but it's still platform-dependent and so > we'd like it to live in cairo. Either of my current patches is within cairo; the question is whether to patch the windows font backend to pad the glyph extents it returns, or to patch cairo so that it adds the padding when it uses the extents to clip before painting the glyphs onto a surface. > Honestly, small cosmetic changes in MathML rendering are not important enough > to delay a blocking1.9.1 fix. The attachments 361625 and 361626 show that the effect on MathML is rather more than a "small cosmetic change", IMO; I think the spread-out rendering that results from the patch is not good enough. Note also that this would become a serious discrepancy between MathML rendering on the different platforms, even when using exactly the same fonts. > > > If we do go that way, then the existing patch is correct as far as the code is > > concerned, and we just need to fix up or disable the problematic tests. > > Let's do that, thanks. I'm currently running reftests, and will post both versions of the patch so you can make a choice. Personally, I prefer the "new" approach that avoids altering the glyph extents, as I think it is less likely to be disruptive and surprising for other cairo clients. Note that in any case we may have to handle similar issues outside cairo as well; this patch (whichever way we do it) does not resolve bug 475968, for example.
(In reply to comment #57) > Is that true? If we could get the precise ink rect covering the pixels rendered > by the glyph (as opposed to the estimate you're producing by adding slop to > every glyph), wouldn't that be adequate for MathML spacing? > > I'm just a little bit skeptical that we really need this version of the ink > rect. I don't see why you'd ever really want to use the extents of the unhinted > outline, since that's not what the true shape of the glyph is. I don't think the "precise ink rect covering the pixels rendered by the glyph" would be appropriate for MathML spacing purposes. The "true shape of the glyph" is a rather fuzzy concept in the antialiased world, and considering every touched pixel will artificially enlarge things. The screenshot here is the word "file:" from the Firefox address bar, with ClearType enabled. The origin of the text is indicated by the orange cross. If we're considering glyph spacing and positioning, it seems to me that the "true shape" of the "f", for example, is 4 pixels wide. However, there is clearly a 5th pixel painted, to the left of the origin. If we're using the glyph extent as a basis for spacing, I don't think this faint yellow pixel should be considered part of the glyph's "ink area". What is less clear is that there is actually a 6th pixel column that is touched by the "f"; it is contributing a slight amount of blueness to the yellowish pixel on the left of the dot of the "i". (This can be confirmed by inserting a space; then the faint blue pixel can be seen against the white background.) So what is the appropriate "ink extent" of the "f" here... is it 4 pixels wide, as cairo would currently tell us (based on what GDI tells it), or is it 6 pixels? That's a 50% increase, which makes a very substantial difference (as shown in the MathML samples above). We need the 6-pixel extent for painting and invalidation purposes, but as far as human perception is concerned (which is what matters for spacing), it's only 4.
(In reply to comment #61) > Either of my current patches is within cairo; the question is whether to patch > the windows font backend to pad the glyph extents it returns, or to patch > cairo so that it adds the padding when it uses the extents to clip before > painting the glyphs onto a surface. Yeah, sorry. I'm not sure. Better ask Behdad. > The attachments 361625 and 361626 show that the effect on MathML is rather > more than a "small cosmetic change", IMO; I think the spread-out rendering > that results from the patch is not good enough. Note also that this would > become a serious discrepancy between MathML rendering on the different > platforms, even when using exactly the same fonts. Indeed. The question then is, what exactly does MathML want here? You've made it clear that it's not the true rendered ink extents. I don't think it's the *unhinted* glyph outline extents; if hinting makes the glyph wider by pixel-aligning vertical stems, for example, that should affect the extents. (Right?) Maybe it's the ink extents of the glyphs if they were hinted, but not antialiased? Whatever the answer is, it does seem we need new API. BTW the MathML spacing code is just supposed to do what TeX does in the same situations, and you're more likely than anyone else around here to know exactly what that is :-).
(In reply to comment #63) > Indeed. The question then is, what exactly does MathML want here? You've made > it clear that it's not the true rendered ink extents. I don't think it's the > *unhinted* glyph outline extents; if hinting makes the glyph wider by > pixel-aligning vertical stems, for example, that should affect the extents. > (Right?) Maybe it's the ink extents of the glyphs if they were hinted, but not > antialiased? That's probably closest of these options, except if we're trying to do resolution-independent layout, in which case the hinting should be ignored. I'm not 100% sure whether Windows is currently giving us an ink extent based on the hinted (but not ClearTyped) glyph. > BTW the MathML spacing code is just supposed to do what TeX does in the same > situations, and you're more likely than anyone else around here to know exactly > what that is :-). To really do what TeX does would require additional per-glyph metrics ("italic correction", in particular, as well as a nominal glyph height and depth that may not exactly correspond to the bounding box). These are not available from standard TrueType/OpenType fonts, so the code is using the glyph extent to guess at reasonable approximations. (The specific math-layout metrics that TeX's algorithm would need can be found in the new 'MATH' table that MS defined for Word2007 and Cambria Math. There are very few fonts as yet that support this, but as a longer-term goal we should look for it and use it if available.)
(In reply to comment #64) > (In reply to comment #63) > > Maybe it's the ink extents of the glyphs if they were hinted, but not > > antialiased? > > That's probably closest of these options, except if we're trying to do > resolution-independent layout, in which case the hinting should be ignored. SVG defines a CSS property called 'text-rendering', one of whose values is 'geometricPrecision' which should turn off hinting and give resolution-independent layout. We should implement that eventually, but currently all our layout is resolution dependent. > I'm not 100% sure whether Windows is currently giving us an ink extent based > on the hinted (but not ClearTyped) glyph. It looks like GetGlyphOutline supports an option GGO_UNHINTED, which we're not passing, so I assume it's giving us hinted outlines. I also assume, perhaps incorrectly, that those outlines are independent of antialiasing. If so, then the extents cairo currently returns would be the right extents for MathML ... although not strictly covering the "inked" extents of the glyph, as the cairo API advertises. So I think we need a new cairo API to return what the current API returns and then we need to make the current API add the fudging for Cleartype. > To really do what TeX does would require additional per-glyph metrics ("italic > correction", in particular, as well as a nominal glyph height and depth that > may not exactly correspond to the bounding box). These are not available from > standard TrueType/OpenType fonts, so the code is using the glyph extent to > guess at reasonable approximations. > > (The specific math-layout metrics that TeX's algorithm would need can be found > in the new 'MATH' table that MS defined for Word2007 and Cambria Math. There > are very few fonts as yet that support this, but as a longer-term goal we > should look for it and use it if available.) I see. Thanks for the info :-).
(In reply to comment #65) > It looks like GetGlyphOutline supports an option GGO_UNHINTED, which we're not > passing, so I assume it's giving us hinted outlines. I also assume, perhaps > incorrectly, that those outlines are independent of antialiasing. If so, then > the extents cairo currently returns would be the right extents for MathML ... > although not strictly covering the "inked" extents of the glyph, as the cairo > API advertises. > > So I think we need a new cairo API to return what the current API returns and > then we need to make the current API add the fudging for Cleartype. Wait, maybe we don't need any new API at all. Maybe instead all we need to do is take your patch here, but only add the Cleartype fudge factor to the extents when the font options antialiasing mode is not CAIRO_ANTIALIAS_NONE. Then we can fix MathML by ensuring that it uses a font with CAIRO_ANTIALIAS_NONE when it does its spacing measurements.
(In reply to comment #66) > Wait, maybe we don't need any new API at all. Maybe instead all we need to do > is take your patch here, but only add the Cleartype fudge factor to the extents > when the font options antialiasing mode is not CAIRO_ANTIALIAS_NONE. Then we > can fix MathML by ensuring that it uses a font with CAIRO_ANTIALIAS_NONE when > it does its spacing measurements. I don't like this very much. Surely the MathML code should use the same Cairo font for its spacing measurements as it will use to draw the glyphs. We don't want to instantiate an entire parallel set of fonts with different properties just for measurement purposes. (Note that a complex math expression could easily use a dozen or more fonts, when you take different styles and sizes into account.)
(In reply to comment #67) > I don't like this very much. Surely the MathML code should use the same Cairo > font for its spacing measurements as it will use to draw the glyphs. If MathML wants to use the non-antialiased glyph extents, this is the obvious way to get them. Offering a separate "get the non-antialiased glyph extents" API in cairo doesn't seem better to me. > We don't > want to instantiate an entire parallel set of fonts with different properties > just for measurement purposes. (Note that a complex math expression could > easily use a dozen or more fonts, when you take different styles and sizes into > account.) I hope it's not performance you're worried about, because MathML performance is *really* low on the list of priorities :-). I'd like to know what Behdad thinks about all this.
This is the same Cairo patch as previously, adding "slop" to the glyph extents in the windows font back-end. The SVG mochitest failure is resolved by making the test conditions less precise, to allow for the potential added pixels; the MathML reftest failure is marked as known-fail on Windows for now. If we adopt this patch, we need to file a bug on the MathML spacing issue (see reftest 355548-3 and comment 63 and following), and another bug on the SVG hit-test code that is using glyph extents where it should probably use advances instead (except for zero-width glyphs?), see the svg/content/test/test_text mochitest.
Attachment #359559 - Attachment is obsolete: true
Attachment #361745 - Flags: review?(roc)
This patch takes the alternative approach of leaving the glyph metrics unchanged, and adding the necessary "slop" when the extents are used to clip a text-drawing operation in the surface-fallback code. This has the advantage that it is a more localized fix for the actual problem we're seeing, and does not risk disrupting other code that depends on the glyph extents. In particular, it does not cause problems for either the SVG or MathML tests that were affected by the first version of the patch. The disadvantage is that we may eventually find other places that use the glyph extents in a similar way, and have to apply similar fixes there. At present we have not seen any such problems, however. Note that in either case, we still have to do similar adjustments at other levels; in particular, bug 475968 still has to be fixed within layout, it is not resolved by these patches within cairo. Although this patch does not affect the SVG reftest, we may still want to consider filing a bug about the use of glyph extents in getCharNumAtPosition() and changing how this is implemented. My preference would be to take this patch as I think it is less disruptive.
Attachment #361750 - Flags: review?(roc)
I think I prefer the patch in comment #69 with an added condition that we only add the slop when antialiasing is enabled in the font.
(In reply to comment #48) > > > I think getCharNumAtPosition should use advance widths instead of glyph > > > extents. > > > > That seems reasonable to me. However, that's really a separate issue from this > > bug. There's still a reftest I need to look at tomorrow, I think, but once > > that's resolved I'd suggest we mark this aspect of the mochitest as "todo", > > re-land this patch, and open a new bug about getCharNumAtPosition. > > We can't mark it as "todo" because it will work on some platforms and todo will > flag that success as an error. Better to comment out the incorrect test. Please > do file that bug. You could even fix it as a gentle introduction to SVG text > code :-). See bug 478792. This addresses both getExtentOfChar and getCharNumAtPosition; I guess it's debatable which way getExtentOfChar should go (depends how it's likely to be used). About to add a comment there....
Updated patch as suggested, to add padding only when antialiasing is in use. Also no need to add padding if the original extent is zero-width (i.e., the glyph has no ink). I have removed the changes to the SVG mochitest from this patch, as that is addressed by bug 478792; when that lands, this should no longer cause test failures.
Attachment #361745 - Attachment is obsolete: true
Attachment #361750 - Attachment is obsolete: true
Attachment #362706 - Flags: review?(roc)
Attachment #361745 - Flags: review?(roc)
Attachment #361750 - Flags: review?(roc)
Depends on: 478792
Keywords: checkin-needed
Whiteboard: [needs new patch] → [needs landing]
Comment on attachment 362706 [details] [diff] [review] revised as per comment #71 [Checkin: Comment 74+75] http://hg.mozilla.org/mozilla-central/rev/120d20888db9
Attachment #362706 - Attachment description: revised as per comment #71 → revised as per comment #71 [Checkin: Comment 74]
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Comment on attachment 362706 [details] [diff] [review] revised as per comment #71 [Checkin: Comment 74+75] http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235494625.1235502272.27781.gz WINNT 5.2 mozilla-central unit test on 2009/02/24 08:57:05 { REFTEST TEST-UNEXPECTED-PASS | file:///e:/builds/moz2_slave/mozilla-central-win32-unittest/build/layout/reftests/bugs/355548-3.xml | } http://hg.mozilla.org/mozilla-central/rev/86d2afaffabd with roc's approval.
Attachment #362706 - Attachment description: revised as per comment #71 [Checkin: Comment 74] → revised as per comment #71 [Checkin: Comment 74+75]
Please, attach an (updated) 1.9.1 patch.
Keywords: checkin-needed
Attached patch patch for 1.9.1 — — Splinter Review
Note that this will cause reftest failures on Windows unless the patch for bug #478792 is also checked in. (That patch applies cleanly on the 1.9.1 tree, no update appears to be needed for it.)
Has this caused Bug 480255?
(In reply to comment #78) > Has this caused Bug 480255? Not really, but it has exposed a problem with XUL layout; see discussion on that bug.
Requesting blocking reevaluation since I think this should *not* block 1.9.1. Bug 480255 is hairy and messes up layout of XUL content that contains non-XUL text. There is no known low-risk fix to those problems. We shipped this bug in Firefox 3 and honestly there hasn't been much negative feedback about it; this bug has no duplicates, only one bug that depends on it, and I haven't heard complaints about this bug outside Bugzilla. We'll leave this fix on trunk and expect bug 480255 to be fixed by the fix for bug 458231 (which is definitely not suitable for 1.9.1).
Flags: blocking1.9.1+ → blocking1.9.1?
Whiteboard: [needs 1.9.1 landing]
Flags: blocking1.9.1? → blocking1.9.1-
This is what I wrote to cairo list now: Hi Jonathan, Sorry to jump in the game this late. I read the comments on the mozilla bug and I agree with pretty much everything you and roc said there. To clarify, cairo's (and pango's) ink extents are supposed to be the tight bounding box of all areas affected by the operation. So your patch is fixing one bug (not-inclusive box) with another (not-tight), but I'm happy enough with it, so you should feel free to push that in master (or have Vlad or Jeff do if you don't have access). Now I new our text extents are kind of a mess and have been thinking about it for a while. For example, we may use the same scaled font for raster and vector backends. Clearly it doesn't make any sense to consider filter effects into extents used with a vector backend. Perhaps the easiest way to differentiate those cases is to check whether metrics-hinting is enabled. So: * If metrics-hinting is enabled, the returned extents are the user-space tight bounding box of all the pixels affected by the glyph raster, * If metrics-hinting is disabled, the returned extents are the user-space tight bounding box of the glyph path. The glyph path may have been modified as a result of hinting, but that's a separate issue. Now if MathML is (ab)using ink extents in its heuristics, that's another issue. The MathML code can try to turn subpixel off, or paint the glyph to an A1 or A8 surface and compute the tight perceived bounding box itself, etc. That's really the heuristic's concern. Cheers, behdad
Just a heads up: I accidentally reverted this patch when updating cairo. See Bug 494743
Depends on: 511762
A player of a browser game of mine just posted a complaint about this bug in the game forum. This is the image he attached http://img64.imageshack.us/img64/751/senzatitolo1h.gif He's using Firefox 3.5 on Windows Vista.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: