548964 - [DW] Spellchecker's underline is rendered incorrectly for one line text input fields. Most of the line is out of view

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[u88484]

Attachment: 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

When did this regress?  Is there a testcase or steps to reproduce?

Comment 2

[u88484]

(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.

Comment 3

[u88484]

Attachment: 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>

Comment 4

[Bas Schouten (:bas.schouten)]

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.

Comment 5

[u88484]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This must be a blocker of 2.0 final.

Comment 9

[Joe Drew (not getting mail)]

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!

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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 11

[John Daggett (:jtd)]

Comment on attachment 480857 [details] [diff] [review]
Patch v1.0

Switching the review to Jonathan here.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Jonathan, would you review it?

Comment 13

[Jonathan Kew [:jfkthame]]

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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

Comment 15

[Jonathan Kew [:jfkthame]]

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 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 486979 [details] [diff] [review]
Patch v2.0

Thanks, let's take this.

Comment 17

[Benjamin Smedberg]

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 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 486979 [details] [diff] [review]
Patch v2.0

re-nominating for b8

Comment 19

[Jonathan Kew [:jfkthame]]

I'd agree that we should take this as soon as possible; I don't see any benefit to deferring it.

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 486979 [details] [diff] [review]
Patch v2.0

Sounds good to me.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/a86dc579ce88

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

temporary backed out due to a orange of reftest only on Win Opt.
http://hg.mozilla.org/mozilla-central/rev/93b47121bacd

Comment 23

[Jonathan Kew [:jfkthame]]

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.

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: 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 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 488759 [details] [diff] [review]
use sans-serif for bugs/433700 reftest to avoid clipped antialiasing

Great! Thank you!

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/c17742be31fb
http://hg.mozilla.org/mozilla-central/rev/5ea89aae98b5