Last Comment Bug 338209 - Make spellchecker use thicker wavy underlines instead of dotted underlines.
: Make spellchecker use thicker wavy underlines instead of dotted underlines.
Status: RESOLVED FIXED
[at risk]
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: P1 normal with 4 votes (vote)
: mozilla1.9.2a1
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
: 336722 341513 (view as bug list)
Depends on: 623234 486708 486735 486778 560124
Blocks: 482138
  Show dependency treegraph
 
Reported: 2006-05-16 15:27 PDT by Brett Wilson
Modified: 2011-01-05 09:06 PST (History)
29 users (show)
dbaron: blocking1.9-
mbeltzner: blocking1.8.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.39 KB, patch)
2006-05-16 15:31 PDT, Brett Wilson
no flags Details | Diff | Review
Example of new underlining (5.74 KB, image/png)
2006-05-16 15:32 PDT, Brett Wilson
no flags Details
Patch that scales (2.04 KB, patch)
2006-05-16 16:05 PDT, Brett Wilson
mscott: review+
Details | Diff | Review
Example of underline going outside of line (2.83 KB, image/png)
2006-05-19 13:17 PDT, Brett Wilson
no flags Details
New patch that paints better (3.14 KB, patch)
2006-06-27 16:22 PDT, Brett Wilson
no flags Details | Diff | Review
Patch v1.0 (74.40 KB, patch)
2009-03-25 02:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch v2.0 (79.34 KB, patch)
2009-03-26 00:26 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
roc: superreview+
Details | Diff | Review
Patch v2.1 (80.27 KB, patch)
2009-03-26 19:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch v3.0 (131.87 KB, patch)
2009-04-02 04:51 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
roc: superreview+
Details | Diff | Review

Description Brett Wilson 2006-05-16 15:27:57 PDT
 
Comment 1 Brett Wilson 2006-05-16 15:31:48 PDT
Created attachment 222261 [details] [diff] [review]
Patch

Mac is still probably solid underlining. That bug might be in rendering code.
Comment 2 Brett Wilson 2006-05-16 15:32:21 PDT
Created attachment 222262 [details]
Example of new underlining
Comment 3 Brett Wilson 2006-05-16 15:38:53 PDT
Comment on attachment 222261 [details] [diff] [review]
Patch

This patch has a problem when the font is really big ~400%
Comment 4 Scott MacGregor 2006-05-16 15:47:33 PDT
That's still a pretty creative solution. I wish I had thought of that last year :).

In addition to the large font problem which I see too with this patch, I also see a minor issue (not even sure if it is an issue) with certain letters like j and g at a normal font size where the bottom part of the j and g overlap slightly with the first line of dotted text. I just tested open office and I see the same thing there so that overlap should be ok.
Comment 5 Brett Wilson 2006-05-16 16:05:50 PDT
Created attachment 222266 [details] [diff] [review]
Patch that scales

This handles scaling better. The line width is constant if you scale the text, but it always looks "correct."

Scott: Can you SR+approve for this module, or do I need to get somebody else?
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-16 17:40:38 PDT
Isn't this a duplicate of bug 336722 ?
Comment 7 Scott MacGregor 2006-05-16 17:55:24 PDT
Comment on attachment 222266 [details] [diff] [review]
Patch that scales

Even though the change is isolated to inline spell checking behavior, I'd feel more comfortable if a gecko layout owner approved the change for the branch.
Comment 8 Karsten Düsterloh 2006-05-16 22:53:10 PDT
*** Bug 336722 has been marked as a duplicate of this bug. ***
Comment 9 Brett Wilson 2006-05-19 13:16:33 PDT
The problem with this patch is that the lines don't get erased properly. This is visible if you turn off spellchecking, or when you correct an incorrect word. It is only visible with some fonts.

It happens when the bottom line is below the bottom of the line. An easy way to reproduce this is to make the font very small. If you select and the red line is below the selection box, it means this problem will occur.

Any suggestions? It looks to me like if we can detect this condition, we can draw the second line *above* the "regular" one instead of below. In the fonts that were causing problems, this looks like it will always look OK.
Comment 10 Brett Wilson 2006-05-19 13:17:55 PDT
Created attachment 222666 [details]
Example of underline going outside of line
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-05-20 21:41:27 PDT
The "right way" to fix that would be to set the text frame's overflow area to make sure it includes the red wavy line.

I'd like to hold off 1.8.1 approval until we've sorted that out.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-05-20 21:44:49 PDT
That means you should add some code near the end of Reflow() so that if your red decoration area is not entirely inside the computed aDesiredSize.width/height, you should set aDesiredSize.mOverflowArea to a rectangle (relative to the frame origin) that is the union of your decoration rect and the frame's basic rect (0,0,aDesiredSize.width,aDesiredSize.height).
Comment 13 Brett Wilson 2006-05-22 16:57:14 PDT
Robert: Sorry, I'm still confused.

Which Reflow? I didn't see a relevant reflow function that has a aDesiredSize parameter. Also, it doesn't sound to me like this would fix all the painting problems. Does reflow get called for every paint? One of the overflow problems is when you turn spellchecking off, the bottom of the lines are not repainted.

I also discovered that this is not a new bug. The regular lines are outside the bounding box and are not painted properly if you make the font small enough.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-13 13:57:58 PDT
There is only one Reflow() method. The parameter in nsTextFrame is actually called aMetrics.

The mOverflowArea in aMetrics lets you record the fact that some of your drawing will slop outside the frame's border-box. Once that's recorded, we will ensure that erasing and redrawing happens properly.

I think bryner can fill you in on more details, or catch me on IRC.
Comment 15 Régis Caspar 2006-06-14 07:11:24 PDT
*** Bug 341513 has been marked as a duplicate of this bug. ***
Comment 16 Arkady Belousov 2006-06-14 07:33:19 PDT
> *** Bug 341513 has been marked as a duplicate of this bug. ***

     Cut&paste of my proposal (bug 341513; see, I think, that not only underline for highlighting is possible):

Currently Composer underlines mis-spelled words by pale-pinked dotted line. For
me, with my bad sight, hard to see this uncontrast highlighting. I will be glad
to have possibility to intensify this highlighting - for example, by more bold
underline or by another border of word.
Comment 17 Brett Wilson 2006-06-20 09:06:39 PDT
Comment on attachment 222266 [details] [diff] [review]
Patch that scales

mconnor: we need a new patch as roc described below. This patch makes an existing problem worse where the underline isn't always repainted properly.
Comment 18 Brett Wilson 2006-06-27 16:22:49 PDT
Created attachment 227316 [details] [diff] [review]
New patch that paints better
Comment 19 Samuel Sidler (old account; do not CC) 2006-06-28 08:57:32 PDT
Is attachment 222262 [details] what the proposed new red line will look like on Mac OS X? And, if so, are we really advocating making the red line look less like OS X on OS X? The current red dots look closer to what's used throughout the OS. Is there any reason they can't be made to look more like what's in OS X instead of less like it?

By all means, if this is a Windows/Linux-specific bug, please let me know.
Comment 20 Brett Wilson 2006-06-28 09:29:52 PDT
It would be better if the lines looked like MSWord/OSX on all platforms, but this is what they look like now becuase this is what our graphics library supports at this time (special code had to be written for Mac even to support this). If you don't like this, please volunteer to enhance the Gfx library.
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-28 10:23:58 PDT
(In reply to comment #20)
> It would be better if the lines looked like MSWord/OSX on all platforms, but

MSWord/OSX don't paint misspelled words the same way. MSWord (and most Windows apps) use a squiggly underline. OS X provides a system-wide spell checker which uses a thick dotted underline. Firefox currently uses a dotted underline on both.

Bug 336772 and other dupes were complaining about lack of visibility of our approach. Making the dots thicker is a solution (since there's no real standard approach on Windows for indicating spelling mistakes), or better yet making thicker dots for OSX and thicker squiggles for Windows (since most windows apps *do* use the squiggle).

Moving back to squiggles on all applications would be a step backwards or OS X users.

> this is what they look like now becuase this is what our graphics library
> supports at this time (special code had to be written for Mac even to support
> this). If you don't like this, please volunteer to enhance the Gfx library.
> 

Comment 22 Brett Wilson 2006-07-27 15:58:57 PDT
The new patch doesn't paint properly. Can roc or somebody else comment on why it still might not work, and what I can do to fix it?
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-07-28 08:29:41 PDT
That patch doesn't modify the overflow area.

I don't think it's worth spending resources on this issue at this point.
Comment 24 Brett Wilson 2006-08-01 13:32:19 PDT
I'm putting this back to blocking-? so drivers can review whether this is really blocking or not. In comment 23 it looks like roc doesn't think it's worth doing.
Comment 25 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-01 15:41:13 PDT
Agreed. Considering roc's comments as well as my own in comment 21, I don't think this blocks release.
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-02-16 13:01:28 PST
Now, bug 392785 was fixed. So, we don't need to worry for the wavy line overflow from font metrics. If you can add the wavy line rendering to nsCSSRendering::PaintDecorationLine, you can fix this bug easily.

However, note that the input element might become scrollable by the wavy line.
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-11 08:27:56 PDT
A latest patch of bug 482138 (I'll post it soon) implemented wavy line. I'll include the style changes for spell checker as well.
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-12 07:40:45 PDT
test build:

https://build.mozilla.org/tryserver-builds/2009-03-12_06:08-masayuki@d-toybox.com-attachment367032 [review]/
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-25 02:34:40 PDT
Created attachment 369247 [details] [diff] [review]
Patch v1.0

roc:

This fixes following issues:
1. support wavy style
2. spellchecker underline uses nsILookAndFeel (on mac, it is thicker dotted line. otherwise, it is wavy line).
3. checking overflow rect from actual selection types.

This patch doesn't have the lifting up the decoration line of the selections. If you want it for this bug, I'll merge it after your review.

And this patch changes the wavy line drawing code from the previous patch of bug 482138. Because the both ends of the wavy line are not aligned to the rect edge in some cases. And also the anti-aliased ends sometimes overflow from the rect. Therefore, this patch uses clipping for the drawing.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-25 22:04:46 PDT
+      gfxFloat rightMost = pt.x + rect.Width() - (lineHeight / 2.0);
+      rightMost += 2.0; // We should draw the wavy line to outside of the rect

Shouldn't this just be
     gfxFloat rightMost = pt.x + rect.Width() + (lineHeight / 2.0);
?

+      pt.x -= 2.0;
+      aGfxContext->MoveTo(pt); // 1
+
+      pt.x += 2.0 + flatLengthAtVertex;
+      pt.x = PR_MIN(pt.x, rightMost);
+      aGfxContext->LineTo(pt); // 2

I'm not sure why you're using 2.0 here.

I'm actually not sure why you need to draw this initial line this way. The way you had it in the previous patch seemed fine, start with a diagonal top-to-bottom line.

+  PRBool IsSelectionUnderlineOverflowing(nsPresContext* aPresContext,
+                                         nsRect& aRect);

const nsRect&

+  if (aLineColor)
+    *aLineColor = color;

{} around this statement

+  if (!NS_IS_VALID_UNDERLINE_STYLE(style))
+    style = NS_UNDERLINE_STYLE_SOLID;

and this one

Shouldn't you be removing DrawIMEUnderline?

I'd prefer CombineSelectionUnderlineRect to take the in/out nsRect& parameter as the last parameter.

+    if (!(sd->mStart != sd->mEnd && sd->mType & SelectionTypesWithDecorations))

I'd prefer
  if (sd->mStart == sd->mEnd || !(sd->mType & SelectionTypesWithDecorations))

+    NS_ASSERTION(!(sd->mType & nsISelectionController::SELECTION_SPELLCHECK),
+                 "still has spell checker selection bit");

I don't think this is worth having since you checked it up above...

+    nsRect r(0, 0, GetRect().width, GetRect().height);

  nsRect r(nsPoint(0, 0), GetSize());

Can you simplify nsTextFrame::IsSelectionUnderlineOverflowing by having GetIMEStyleIndex return a value for SELECTION_SPELLCHECK? You'd want to then rename GetIMEStyleIndex to GetUnderlineStyleIndexForSelectionType or something.

Otherwise looks good.
Comment 31 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-26 00:26:41 PDT
Created attachment 369452 [details] [diff] [review]
Patch v2.0

> I'm actually not sure why you need to draw this initial line this way. The way
> you had it in the previous patch seemed fine, start with a diagonal
> top-to-bottom line.

Because the old drawing code doesn't paint the first pixel, i.e., the top-left pixel of the rect, isn't painted when the line-height is 1px. Maybe, the cause is |pt.x + lineHeight / 2.0|. However, this is needed for pixel alignment and thicker wavy line.

> +  PRBool IsSelectionUnderlineOverflowing(nsPresContext* aPresContext,
> +                                         nsRect& aRect);
> 
> const nsRect&

No, aRect will contain the decoration rect, it is used for overflow rect. I think that the name is wrong, therefore, I renamed it to CombineSelectionUnderlineRect.
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-26 14:11:33 PDT
The patch is OK but I really want to have some tests here before this can land.

You should be able to use mochitests to control nsISelectionController to set up selections of the different types. You should be able to use WindowSnapshot.js to do reftest-style comparisons. You should able to set up references using SVG or canvas APIs to do the same drawing you expect each platform's look-and-feel to request. Or better still, override the look-and-feel preferences to test all the possible line styles. You may also want to use the Ahem font (using @font-face) so you get predictable font metrics. You could also create your own versions of Ahem with different descent metrics and underline positions to test that code. You should test different font sizes as well.

Does that make sense? It sounds like a lot of work, and probably is a lot of work, but I think it's worth it. This is tricky code and it will easily regress if we don't have tests, and a lot of our developers (including me) don't use IMEs so there won't be manual testing.
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-26 18:18:55 PDT
Ugh, it seems that it is too hard work for now.

It seems that we should have better way for the tests. Maybe, the testing of wavy line style can be without text rendering. Because we can use spaces (U+20 or U+A0) for testing decoration lines. And we can clip the rendering area by |overflow: hidden;|. So, if we fix the fonts on the test, we can compare with PNG image which is pre-rendered. Does this make sense?
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-26 18:31:43 PDT
And I'm not sure that we regress the wavy line rendering soon. Because the rendering code is independent from other code. So, if we implement "-moz-text-decoration-style" in bug 59109, doesn't help it us to test the wavy line rendering? I.e., we don't need to use nsISelectionController for the testing.

And also the selection decoration style tests can be separated to another test and we can use solid lines, probably.
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-26 18:35:14 PDT
Oh, I forgot a thing. The patch has a bug for IME underline rendering. I'll post a new patch soon.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-26 18:51:56 PDT
Yes, implementing text-decoration-style would help with testing, but we should still be testing the textframe code that converts selection information to style information. So we still need tests that use nsISelectionController. So you may as well write these tests before bug 59109 is fixed.

Good idea to use spaces instead of actual characters, that may help, but you'll still need predictable font metrics so I recommend using Ahem and/or other custom fonts. It's easy to use with @font-face.

I don't think we can compare to a pre-rendered PNG because path rasterization is different across platforms.

I agree these tests will be some work but it's very important. You're adding a lot of code here and it needs to be tested thoroughly and automatically.
Comment 37 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-26 19:09:28 PDT
Created attachment 369612 [details] [diff] [review]
Patch v2.1

We need gaps between IME selections. DrawIMEUnderline which is removed processed this. But the previous patch didn't do this.

> static void DrawSelectionDecorations(gfxContext* aContext, SelectionType aType,
>     nsTextPaintStyle& aTextPaintStyle, const gfxPoint& aPt, gfxFloat aWidth,
>     gfxFloat aAscent, const gfxFont::Metrics& aFontMetrics)
> {
>   gfxPoint pt(aPt);
>   gfxSize size(aWidth, aFontMetrics.underlineSize);
> 
>   switch (aType) {
>     case nsISelectionController::SELECTION_IME_RAWINPUT:
>     case nsISelectionController::SELECTION_IME_SELECTEDRAWTEXT:
>     case nsISelectionController::SELECTION_IME_CONVERTEDTEXT:
>     case nsISelectionController::SELECTION_IME_SELECTEDCONVERTEDTEXT:
>       // IME decoration lines should not be drawn on the both ends, i.e., we
>       // need to cut both edges of the decoration lines.  Because same style
>       // IME selections can adjoin, but the users need to be able to know
>       // where are the boundaries of the selections.
>       //
>       //  X: underline
>       //
>       //     IME selection #1        IME selection #2      IME selection #3
>       //  |                     |                      |                    
>       //  | XXXXXXXXXXXXXXXXXXX | XXXXXXXXXXXXXXXXXXXX | XXXXXXXXXXXXXXXXXXX
>       //  +---------------------+----------------------+--------------------
>       //   ^                   ^ ^                    ^ ^
>       //  gap                  gap                    gap
>       pt.x += 1.0;
>       size.width -= 2.0;
>     case nsISelectionController::SELECTION_SPELLCHECK: {
>       float relativeSize;
>       PRUint8 style;
>       nscolor color;
>       PRInt32 index =
>         nsTextPaintStyle::GetUnderlineStyleIndexForSelectionType(aType);
>       if (aTextPaintStyle.GetSelectionUnderlineForPaint(index, &color,
>                                                         &relativeSize,
>                                                         &style)) {
>         size.height *= relativeSize;
>         nsCSSRendering::PaintDecorationLine(
>           aContext, color, pt, size, aAscent, aFontMetrics.underlineOffset,
>           NS_STYLE_TEXT_DECORATION_UNDERLINE, style);
>       }
>       break;
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-26 19:46:41 PDT
Can I turn off the antialias of the line on Canvas or SVG??
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-26 19:52:10 PDT
You can with SVG (shape-rendering), not sure about canvas.
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-26 20:04:32 PDT
I could do it, thanks!!
Comment 41 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-29 02:00:53 PDT
roc, I have a question. The Ahem font and the mplus regular font that they are in reftests/fonts have different metrics, so, I can use these fonts for the testing. However, I have no idea how to copy them to mochitest dir, because I'm not familiar to make file hacking. Can I copy them from reftests at build time? Or should I copy them to layout/generic/test dir?
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-03-29 14:46:51 PDT
I think you could add a Makefile.in to layout/reftests/fonts which copies fonts to somewhere mochitest can see them. Something like this:

_TEST_FILES = Ahem.ttf \
              mplus.ttf \
              $(NULL)

libs:: $(_TEST_FILES)
        $(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/tests/fonts
Comment 43 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-03-29 21:46:19 PDT
Thank you, I succeeded to copy them. But I also needed to add the origin folders to layout/Makefile.in.

I'm working on the reference SVG files now.
Comment 44 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-04-02 04:51:57 PDT
Created attachment 370619 [details] [diff] [review]
Patch v3.0

This patch includes the automated test. But it can run only on Windows. Because we cannot get actual font metrics of a font from javascript. Therefore the test has the font metrics of my environment (Ubuntu8.10 and MacOS X 10.4 and 10.5). However, the values are different on tinderbox machines. Therefore, the tests are failed on Mac and Linux. The reason of failure on Linux should be the difference of the version of FreeType and Pango. I'm not sure the reason on Mac.

The patch tests all underline styles on each selection types at 3 sizes of 2 fonts. I.e., 6 * 5 * 3 * 2 = 180 patterns. The tested code is XP level modules, therefore, the test is enough for checking the regression on all platforms even if we cannot run it on Mac and Linux.

And also this patch fixes 2 problems of the latest patch.

1. Dotted and dashed line is drawn to the outside of the decoration rect. If center of last dot/dash is not in the decoration rect, the last dot/dash was not drawn. For this problem, this patch draws the decoration line from the |rect.size.y| to |rect.size.y + rect.size.width + dashwidth| after clipping by the decoration rect.

2. nsPaintTextStyle::mSelectionStyle[4] -> nsPaintTextStyle::mSelectionStyle[5]. I forgot to increase the array length.
Comment 45 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-02 16:30:43 PDT
Comment on attachment 370619 [details] [diff] [review]
Patch v3.0

This is great!

+var gFontMetrics = [];

Can you add a comment explaining which font size and font each array element is for?

It's unfortunate that the font metrics are not predictable here. When we get SVG font support I think we can fix this, because we'll completely bypass the platform font system.

Thanks!
Comment 46 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-04-03 00:30:50 PDT
http://hg.mozilla.org/mozilla-central/rev/8f2fc4d53842
Comment 47 Sylvain Pasche 2009-04-03 11:38:02 PDT
Verdana fonts get a very think wavy underline on Linux, could that be an issue with the font?
Comment 48 Sylvain Pasche 2009-04-03 11:47:42 PDT
filed bug 486735 about it

Note You need to log in before you can comment on or make changes to this bug.