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

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: u88484, Assigned: masayuki)

Tracking

({regression})

Trunk
mozilla2.0b8
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -, status1.9.2 unaffected, status1.9.1 unaffected)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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?
(Reporter)

Comment 2

7 years ago
(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.
(Reporter)

Comment 3

7 years ago
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.
(Reporter)

Comment 5

7 years ago
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: --- → ?
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
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: ? → -
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>.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #480857 - Flags: review?(jdaggett)

Comment 11

7 years ago
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 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
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+
http://hg.mozilla.org/mozilla-central/rev/a86dc579ce88
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
temporary backed out due to a orange of reftest only on Win Opt.
http://hg.mozilla.org/mozilla-central/rev/93b47121bacd
Status: RESOLVED → REOPENED
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.
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.
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+
http://hg.mozilla.org/mozilla-central/rev/c17742be31fb
http://hg.mozilla.org/mozilla-central/rev/5ea89aae98b5
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 610330

Updated

6 years ago
Depends on: 631507

Updated

6 years ago
No longer depends on: 631507
You need to log in before you can comment on or make changes to this bug.