2.75 KB, image/png
172 bytes, text/html
1.17 KB, patch
Joe Drew (not getting mail): approval2.0+
|Details | Diff | Splinter Review|
1.41 KB, patch
|Details | Diff | Splinter Review|
Created attachment 429246 [details] Screenshot The spellchecker's red underline is render incorrectly in that the line is mostly out of view and only a pixel or two of the line can be seen in one line text input fields.
When did this regress? Is there a testcase or steps to reproduce?
(In reply to comment #1) > When did this regress? Is there a testcase or steps to reproduce? Regressed with the landing of the D2D patches. Bug only occurs when D2D is enabled. Testcase coming.
Created attachment 429260 [details] Reduced testcase For some reason the first input field with the <form></form> code around it is needed for this bug to show up in the second input field. Makes no sense to me and took me quite a while to figure this out. <form><span><input type="text" value="asdfqjpy"></span></form> <span><input type="text" value="asdfqjpy" size="40"></span>
Interesting, I suspect this has something to do with the difference in font metrics between GDI fonts and DirectWrite fonts, I have no idea how the spellchecker line is rendered though, so I don't have a quick answer to where exactly the problem is.
Masayuki, can you check this out and see if you know what is going on. You did a lot of the new spellchecker's wavy underline work and this bug is sort of related to but 486778 that also dealt with the wavy line drawing incorrectly on one line text input boxes.
Oops, sorry for the delay. I missed to catch the your comment. I cannot reproduce this bug on the testcase with latest trunk build, can you still reproduce this?
I can reproduce this bug only on some input fields which doesn't have enough height for the fonts. E.g., in the bugzilla's search page on Win7-Ja.
This must be a blocker of 2.0 final.
Masayuki, why do you think this blocks? It's a little ugly, but I'd release with this bug. I'd take a patch though!
Created attachment 480857 [details] [diff] [review] Patch v1.0 Joe: Because this is a regression, and this isn't good thing for user experience. I find the cause. The internal leading value is incorrect. It must be difference between maxHeight and emHeight because line height is computed from emHeight + internal leading + external leading. Looks like the current computed value is always 1px smaller. This patch also fixes a bug that is the bottom of 'g' is hidden in <input>.
Comment on attachment 480857 [details] [diff] [review] Patch v1.0 Switching the review to Jonathan here.
Jonathan, would you review it?
Comment on attachment 480857 [details] [diff] [review] Patch v1.0 (Sorry for the delay.) I notice the same calculation is done in gfxFont::CalculateDerivedMetrics() at http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#1550, except that it also guards against the possibility of internalLeading becoming negative; might be worth doing the same here.
Created attachment 486979 [details] [diff] [review] Patch v2.0
Comment on attachment 486979 [details] [diff] [review] Patch v2.0 Ok, this looks like the right thing to do; I hope it won't have unwanted effects on metrics/spacing elsewhere!
Comment on attachment 486979 [details] [diff] [review] Patch v2.0 Thanks, let's take this.
That doesn't make me feel confident. Let's wait until after FF4 unless there's some really good reason we should take this now.
Comment on attachment 486979 [details] [diff] [review] Patch v2.0 re-nominating for b8
I'd agree that we should take this as soon as possible; I don't see any benefit to deferring it.
Comment on attachment 486979 [details] [diff] [review] Patch v2.0 Sounds good to me.
temporary backed out due to a orange of reftest only on Win Opt. http://hg.mozilla.org/mozilla-central/rev/93b47121bacd
The reftest failure is due to a couple of antialiasing pixels at the tips of serifs that project beyond the bounds used when painting, as in bug 475968; it does not indicate a real failure in this patch. I'm not sure what has triggered the issue in this particular case, but I think the simplest fix is to add "font-family: sans-serif" to the body style of the test and reference. This will eliminate the serifs where the excess antialiasing pixels occur.
Created attachment 488759 [details] [diff] [review] use sans-serif for bugs/433700 reftest to avoid clipped antialiasing With this change to the troublesome reftest, the original patch for DW font metrics gives a clean reftest run (all platforms) on tryserver.
Comment on attachment 488759 [details] [diff] [review] use sans-serif for bugs/433700 reftest to avoid clipped antialiasing Great! Thank you!