Closed Bug 486735 Opened 11 years ago Closed 11 years ago
.ttf gets a very thick underline with spelling errors
18.87 KB, image/png
254 bytes, text/html
6.45 KB, image/png
10.70 KB, patch
|Details | Diff | Splinter Review|
I guess it's Verdana.ttf from msttcorefonts: fc-match -v Verdana|grep file file: "/usr/share/fonts/truetype/msttcorefonts/Verdana.ttf"(s)
I guess that's because of that font metrics. Maybe the maximum thickness should be clipped?
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
or maybe I should say too disruptive. It's not only about thickness, but also amplitude and position relative to the text.
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.
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.
(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).
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.
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.
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);
This patch computes the spellchecker underline size as my previous comment.
(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.
What caused the regression? Something that landed since 1.9.1? (I'm trying to evaluate the blocking nomination right now, not the review.)
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-
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 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
Closed: 11 years ago
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+
Whiteboard: [checkin-needed for 1.9.2]
You need to log in before you can comment on or make changes to this bug.