486735 - Verdana.ttf gets a very thick underline with spelling errors

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Sylvain Pasche]

Attachment: screenshot

I guess it's Verdana.ttf from msttcorefonts:

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

Comment 1

[Sylvain Pasche]

I guess that's because of that font metrics. Maybe the maximum thickness should be clipped?

Comment 2

[Sylvain Pasche]

Attachment: before/after comparisons on Windows and Linux

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.

Comment 3

[Sylvain Pasche]

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

Comment 4

[Sylvain Pasche]

s/disruptive/distracting/. English fail :-/

Comment 5

[Peter van der Woude [:Peter6]]

Attachment: testcase

Comment 6

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

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.

Comment 7

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

Attachment: Patch v1.0

fix.

Comment 8

[Biju]

Attachment: spell_before_after.png

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.

Comment 9

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

Because font metrics has underline size information. We honor it.

Comment 10

[Biju]

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

Comment 11

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

(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

[Biju]

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.

Comment 13

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

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure what to do here. Calling UX/design overlords

Comment 15

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

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

[Biju]

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

Comment 17

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

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.

Comment 18

[Jennifer Morrow [:Boriss] (UX)]

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.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Biju]

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

Comment 21

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

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);

Comment 22

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

Attachment: Patch v2.0

This patch computes the spellchecker underline size as my previous comment.

Comment 23

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

test builds:
https://build.mozilla.org/tryserver-builds/[e-mail]-bug486735-2/

Comment 24

[Biju]

(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

Comment 25

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

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.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What caused the regression?  Something that landed since 1.9.1?  (I'm trying to evaluate the blocking nomination right now, not the review.)

Comment 27

[Sylvain Pasche]

The regression comes from bug 338209 which landed on 1.9.2 (1.9.1 is not affected by this issue).

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 29

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

Attachment: Patch v3.0

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.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 394806 [details] [diff] [review]
Patch v3.0

r=dbaron

Comment 31

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

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

checked-in to trunk, I'll request the a192.

Comment 32

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

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.

Comment 33

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b577f2b964d5

Comment 34

[David McRitchie]

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