Closed Bug 548964 Opened 14 years ago Closed 14 years ago

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


(Core :: Graphics, defect)

Windows 7
Not set



Tracking Status
blocking2.0 --- -
status1.9.2 --- unaffected
status1.9.1 --- unaffected


(Reporter: u88484, Assigned: masayuki)



(Keywords: regression)


(4 files, 1 obsolete file)

Attached image 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.
Attached file 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.
blocking2.0: --- → ?
Summary: Spellchecker's underline is rendered incorrectly for one line text input fields. Most of the line is out of view → [DW] Spellchecker's underline is rendered incorrectly for one line text input fields. Most of the line is out of view
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!
blocking2.0: ? → -
Attached patch Patch v1.0 (obsolete) — Splinter Review

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>.
Assignee: nobody → masayuki
Attachment #480857 - Flags: review?(jdaggett)
Comment on attachment 480857 [details] [diff] [review]
Patch v1.0

Switching the review to Jonathan here.
Attachment #480857 - Flags: review?(jdaggett) → review?(jfkthame)
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, except that it also guards against the possibility of internalLeading becoming negative; might be worth doing the same here.
Attached patch Patch v2.0Splinter Review
Attachment #480857 - Attachment is obsolete: true
Attachment #486979 - Flags: review?(jfkthame)
Attachment #480857 - Flags: review?(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!
Attachment #486979 - Flags: review?(jfkthame) → review+
Comment on attachment 486979 [details] [diff] [review]
Patch v2.0

Thanks, let's take this.
Attachment #486979 - Flags: approval2.0?
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.
Attachment #486979 - Flags: approval2.0? → approval2.0-
Comment on attachment 486979 [details] [diff] [review]
Patch v2.0

re-nominating for b8
Attachment #486979 - Flags: approval2.0- → approval2.0?
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.
Attachment #486979 - Flags: approval2.0? → approval2.0+
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
temporary backed out due to a orange of reftest only on Win Opt.
Resolution: FIXED → ---
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.
With this change to the troublesome reftest, the original patch for DW font metrics gives a clean reftest run (all platforms) on tryserver.
Attachment #488759 - Flags: review?
Attachment #488759 - Flags: review? → review?(masayuki)
Comment on attachment 488759 [details] [diff] [review]
use sans-serif for bugs/433700 reftest to avoid clipped antialiasing

Great! Thank you!
Attachment #488759 - Flags: review?(masayuki) → review+
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Blocks: 610330
Depends on: 631507
No longer depends on: 631507
You need to log in before you can comment on or make changes to this bug.