Verdana.ttf gets a very thick underline with spelling errors

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: sylvain.pasche, Assigned: masayuki)

Tracking

({regression, testcase})

Trunk
mozilla1.9.3a1
x86
Linux
Points:
---
Bug Flags:
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
Posted image screenshot (obsolete) —
I guess it's Verdana.ttf from msttcorefonts:

fc-match -v Verdana|grep file
        file: "/usr/share/fonts/truetype/msttcorefonts/Verdana.ttf"(s)
(Reporter)

Updated

10 years ago
Blocks: 338209
No longer blocks: 486708
Keywords: regression, testcase
Summary: Verdana.ttf get a very think underline with spelling errors → Verdana.ttf gets a very think underline with spelling errors
(Reporter)

Comment 1

10 years ago
I guess that's because of that font metrics. Maybe the maximum thickness should be clipped?
(Reporter)

Comment 2

10 years ago
From top to bottom:
 - Windows before
 - Windows after
 - Linux before
 - Linux after

In my opinion, the wavy underlines are too thick on Windows/Linux+Verdana.
Attachment #370921 - Attachment is obsolete: true
(Reporter)

Comment 3

10 years ago
or maybe I should say too disruptive. It's not only about thickness, but also amplitude and position relative to the text.
(Reporter)

Comment 4

10 years ago
s/disruptive/distracting/. English fail :-/
Summary: Verdana.ttf gets a very think underline with spelling errors → Verdana.ttf gets a very thick underline with spelling errors
Ugh, this bug is difficult, maybe. Verdana's font metrics maybe tell the thicker underline size to us. But looks like we should have limitation table which has pairs of the font sizes and the selection underline thickness limitations.
Posted patch Patch v1.0 (obsolete) — Splinter Review
fix.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #371018 - Flags: superreview?(roc)
Attachment #371018 - Flags: review?(roc)

Comment 8

10 years ago
please see attachment
Why are we changing size of the spelling error underline 
depending upon font size? 

OpenOffice and MS-Word dont do that, 
they both have a constant height regardless of font size.
We could have pref setting, 
so if people like me who dont like the wavy underline can make to 0px
or those who want bigger can make it bigger.
Because font metrics has underline size information. We honor it.

Comment 10

10 years ago
(In reply to comment #9)
> Because font metrics has underline size information. We honor it.

Other word processors dont do that, so are we smarter than them?
(In reply to comment #10)
> (In reply to comment #9)
> > Because font metrics has underline size information. We honor it.
> 
> Other word processors dont do that, so are we smarter than them?

For CSS3 text-decoration, current our implementation is better then them, especially when we are drawing large text, e.g., heading text.

However, the selection underlines (spellchecker and IME composition) prefer the readability rather than pretty underline. See my posted patch, I think that it's enough. The patch changes to: If the font heigh is 20px of lower, we always use 1px. If the font height is 32px or lower, using lower value of 2px or font metrics's underline size. Otherwise, -- such font size is not used in text editor -- using font metrics's underline size (i.e., current behavior).

Comment 12

10 years ago
I can agree it for accessibility.

On other cases showing spelling error prominently reduces readability.
This is because 
1) we dont have "ignore" option 
2) we dont want some terms to be added to firefox spellcheck dictionary.
   especially acronyms and many tech terms.
3) Always making pretty improves readability.

I have not done a survey on how much the spell error line annoys people. But this is what my friend at a multinational firm advised. When ever you submit a resume make sure you have done "ignore all" for the spelling errors shown for technical words and other words not in dictionary. Otherwise the HR person who is supposed to pre-scan will be annoyed to read it, hence not going to present it to the interviewer/recruiter.
First, the text editors are *not* for reading. It *should* be used for inputting text. Especially, textarea element which enables auto spell checking in default settings. input element is not so, this should mean the unreadability is not a problem on input element.

(In reply to comment #12)
> On other cases showing spelling error prominently reduces readability.
> This is because 
> 1) we dont have "ignore" option 

I don't think so, we can stop the spellchecking from both option dialog and context menu of each editors.
I'm not sure what to do here. Calling UX/design overlords
I think that the selection underline should not honor the underline size of font metrics if the underline size is too thick for the font size. The thick underline can cause the unreadability. The patch using magic number (I hate it, but I have no better idea).

When the font size is 20px or lower, the underline size should be 1px. This is for most plain text editor, i.e., input element and textarea element.

Otherwise, when the font size is 32px or lower, the underline size should be the minimum value of the font metrics's value and 2px. This is for heading text of HTML editor.

Otherwise, we don't need to care, because the font size is enough too large for readability.

Comment 16

10 years ago
NB: Right now I am happy with pref option ui.SpellCheckerUnderlineStyle = 1

Otherwise my point is we should not change size of the spelling error markings to match with font size. I agree underlines should change size with font size, but that rule should not apply for "spelling error markings".

Current behaviour is not only happening for TEXTAREAs but also for contentEditable HTML areas. Looks like we used to do this even 
before bug 338209, but was not that noticable.

We see MS-Word and OpenOffice dont change size of their spelling error markings with font size, but they do change size of underline with font size

See attachment 371020 [details] spell_before_after.png
I don't think that we should not change the underline size. Because large text should have thicker underline for readability.

But I agree that smaller fonts should ignore the thickness.

I'm still waiting roc and jboriss's comments.
My recommendation is to not change the size of the spelling error markings (SEM) to match font size.  

Unlike an underline or any other applied style, SEM is only there to catch the visual attention of the user.  So it needs to be 1. subtle enough to not interfere with text readability and 2. visible enough to be noticed.  The way we balance #2 is by making it small (subtle) and red (unsubtle), acting as a small warning flag that anyone who edits text will become accustomed to noticing.  A change in the size of this flag means there are effectively there are two different symbols for the user to store in memory rather than one.
If the markings don't change to match the font size, what *should* they match?  Should they be a constant size in pixels, or points (our guess at them), or fractions of the user's default font size?  The perceived size of all of these varies widely between users and configurations.

Font size is in some ways a reasonable way to approximate what "small" means in a given context.  Users with high resolution monitors and poor vision might not see a 1px underline; users with good vision and lower resolution monitors probably will -- but their font sizes are likely to be different to match.

I think the approach in this patch is reasonable, although I wonder if it might be better to generalize a little more by clamping to at most 1/20 of the font size rounded up to the nearest pixel, or something like that.

Comment 20

10 years ago
jboriss, thanks for reply.


(In reply to comment #19)
> what *should* they match? 

constant size in pixels

May be some pref item 
(Why ui.SpellCheckerUnderline?, why always an Underline?
 why cant it be a background/foreground color change?)
* ui.SpellChecker.Mark.Thickness - gives thickness 
* ui.SpellChecker.Mark.Height - gives for wavy it gives wave amplitude
* ui.SpellChecker.Mark.Width - gives for wavy it gives wave length 
                               (or half wave length)
Optional if the value of above items is of the form "1.5pt" then it take as points. Or if is of the form "0.15ex" then it take it as relative to font size.

> Users with high resolution monitors and poor vision might not
> see a 1px underline; users with good vision and lower resolution monitors

I have worked in various companies and here is how they solved it.
* Case I : 40 thousand CSRs using intranet app. 
  Solution : Increased size of the monitor for low vision users
* Case II : 4 thousand CSRs using thick client app. 
  Solution : Increased size of the monitor for low vision users
* Case III : 2 thousand non CSR users using various apps. 
  Solution : Changed screen resolution to 600x800
Um, IME selection underlines of wordpad on Vista are constant size excepting when the style is solid. When it's solid, looks like the thickness depends on the font metrics.

On OOo3, the spellchecker underline is constant size but the IME selection underlines always depend on the font metrics.

In most cases, IME selection underline color is same as the text color. So, the IME underline thickness is important. But the spellchecker underline can be found easily by the color. So, I agree to use constant size only for the spellchecker underline.

How about this?
1. Use 1/20 size of the user default font size. (of course, rounded up to the nearest pixels)
2. If the default underline size of #1 is larger than 1px but the computed underline size from the font size (1/20) is smaller than the #1, we should use the latter.

So, I think:
baseSize = MIN(user-default-font-size, actual-font-size);
underlinesize = roundup(baseSize / 20);
Posted patch Patch v2.0 (obsolete) — Splinter Review
This patch computes the spellchecker underline size as my previous comment.
Attachment #371018 - Attachment is obsolete: true
Attachment #390457 - Flags: review?(jboriss)
Attachment #390457 - Flags: review?(dbaron)
Attachment #371018 - Flags: superreview?(roc)
Attachment #371018 - Flags: review?(roc)

Comment 24

10 years ago
(In reply to comment #21)
> So, I think:
> baseSize = MIN(user-default-font-size, actual-font-size);
> underlinesize = roundup(baseSize / 20);
I think that will be good enough
I think that this should be fixed before 192 final. It's not good that we change the behavior after 193 or later. The UI change should be one time.
Flags: blocking1.9.2?
What caused the regression?  Something that landed since 1.9.1?  (I'm trying to evaluate the blocking nomination right now, not the review.)
(Reporter)

Comment 27

10 years ago
The regression comes from bug 338209 which landed on 1.9.2 (1.9.1 is not affected by this issue).
Flags: blocking1.9.2? → wanted1.9.2+
Comment on attachment 390457 [details] [diff] [review]
Patch v2.0

>+      // The thickness of the spellchecker underline shouldn't honor the font
>+      // metrics.  It should be constant pixels value which is decided from the
>+      // default font size.  Note that if the actual font size is smaller than
>+      // the default font size, we should use the actual font size because the
>+      // computed value from the default font size can be too thick for the
>+      // current font size.
>+      nsAdoptingCString defaultLangGroup =
>+        nsContentUtils::GetCharPref("font.language.group");
>+      nsCAutoString defaultFontSizePrefName("font.size.variable.");
>+      defaultFontSizePrefName.Append(defaultLangGroup);
>+      PRInt32 defaultFontSize =
>+        nsContentUtils::GetIntPref(defaultFontSizePrefName.get(), 16);
>+      gfxFloat fontSize = PR_MIN(gfxFloat(defaultFontSize),
>+                                 aFontMetrics.emHeight);
>+      fontSize = PR_MAX(fontSize, 1.0);
>+      return NS_ceil(fontSize / 20);


If you want to get the default font size, just call:
  nscoord defaultFontSize =
    aPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID)->size;

However, that doesn't account for text zoom.  Could could easily account for that by instead doing:

  nscoord defaultFontSize = nsStyleFont(aPresContext).mFont.size;

(And you have a pres context at both callers.  And you would need to convert that to device pixels, I presume.)



Otherwise I think this seems ok, though, although I'm not sure why the particular formula you picked is ideal.
Attachment #390457 - Flags: review?(dbaron) → review-
Posted patch Patch v3.0Splinter Review
Thank you, dbaron.

> Otherwise I think this seems ok, though, although I'm not sure why the
> particular formula you picked is ideal.

I'm not sure whether the magic number 20 is good or not. But I don't find the problems by the number. I hope that it's suitable value.
Attachment #390457 - Attachment is obsolete: true
Attachment #394806 - Flags: review?(jboriss)
Attachment #394806 - Flags: review?(dbaron)
Attachment #390457 - Flags: review?(jboriss)
Comment on attachment 394806 [details] [diff] [review]
Patch v3.0

r=dbaron
Attachment #394806 - Flags: review?(dbaron) → review+
Attachment #394806 - Flags: review?(jboriss) → review+
http://hg.mozilla.org/mozilla-central/rev/f04f32458068

checked-in to trunk, I'll request the a192.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 394806 [details] [diff] [review]
Patch v3.0

Let's take this patch to 1.9.2.

The risk is low, this patch changes the thickness of the spellchecker underline to a consistency size which is computed from the default font size of the profile.
Attachment #394806 - Flags: approval1.9.2?
Attachment #394806 - Flags: approval1.9.2? → approval1.9.2+
Keywords: checkin-needed
Whiteboard: [checkin-needed for 1.9.2]

Comment 34

9 years ago
Even harder to see under an underlined link, can the wavy underline be dropped so it is entirely below a solid underline by the thickness of the solid underscore.      The following makes a web page editable to see this and to check for spelling errors.   

javascript:document.body.contentEditable='true';%20document.designMode='on';%20void%200
You need to log in before you can comment on or make changes to this bug.