560124 - spellcheck thicker wavy underlines do not appear

Status: RESOLVED WORKSFORME

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

Description

[Brian Wasserman]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

When upgrading from Firefox 3.5.9 to 3.6.x (3.6.3 now) I noticed the spellcheck underlining no longer works.  After some troubleshooting, I determined that it was partly working, in that if I right-click on a word that was intentionally misspelled then I do get the suggestions, etc. The only part that is not working is the visual indicator.  

Reproducible: Always




I am using Ubuntu Karmic (9.10) with the latest Firefox from mozilla.com, not the Ubuntu distributed version.  I do have the spellchecking enabled/checked through the option Edit->Preferences->Advanced->Check my spelling.  I have regressed the spellchecking feature last working as expected with firefox-3.6a1pre.en-US.linux-i686.2009-04-02-03.tar.bz2, and breaking in firefox-3.6a1pre.en-US.linux-i686.2009-04-03-03.tar.bz2.  Here's the regression link:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d3240cd1c610&tochange=1886b176f000

Here's the apparent bug related to this feature:
https://bugzilla.mozilla.org/show_bug.cgi?id=338209

Comment 1

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

If you force the window to repaint (e.g., by resizing it or, depending on your window manager, moving another window over it), does that make them appear?  (In other words, I'd like to know if the bug is just a failure to cause the necessary area to repaint.)

Have you tried on pages that use a few different fonts for the text that the spellcheck underline is on?  (Could the bug be specific to a certain font?)

Comment 2

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

I have more questions:

* Does that happen on textarea and HTML editor too? I mean that the underline may be clipped by the box of input element.

* Can you reproduce this bug after you add a int pref "ui.SpellCheckerUnderlineStyle" and set its value to 1 (dotted)?

Comment 3

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

(In reply to comment #2)
> * Can you reproduce this bug after you add a int pref
> "ui.SpellCheckerUnderlineStyle" and set its value to 1 (dotted)?

FYI: You need to restart Fx when you change this pref.

Comment 4

[Brian Wasserman]

(In reply to comment #1)
> If you force the window to repaint (e.g., by resizing it or, depending on your
> window manager, moving another window over it), does that make them appear? 
> (In other words, I'd like to know if the bug is just a failure to cause the
> necessary area to repaint.)
> 
> Have you tried on pages that use a few different fonts for the text that the
> spellcheck underline is on?  (Could the bug be specific to a certain font?)

I tried resizing and moving windows over the text and this did not cause the underlining to show.

I forced the font by unchecking: Edit->Preferences->Content->Advanced->Allow pages to use their own fonts.

I see no red underline in a textbox on gmail compose or gist.github.com.  I'm not sure of the native fonts.

Comment 5

[Brian Wasserman]

(In reply to comment #2)
> I have more questions:
> 
> * Does that happen on textarea and HTML editor too? I mean that the underline
> may be clipped by the box of input element.
> 
> * Can you reproduce this bug after you add a int pref
> "ui.SpellCheckerUnderlineStyle" and set its value to 1 (dotted)?

I'm not sure what you mean by HTML editor.

ui.SpellCheckerUnderlineStyle set to 1 does fix the problem.

Comment 6

[Brian Wasserman]

ui.SpellCheckerUnderlineStyle values of 0 and 5 both have no underline.  All other values appear to work.

Comment 7

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

(In reply to comment #6)
> I'm not sure what you mean by HTML editor.

E.g., gmail's rich text editor.

> ui.SpellCheckerUnderlineStyle values of 0 and 5 both have no underline.  All
> other values appear to work.

0 is none, so, not shown is correct. 5 is wavy, this is default settings on Linux. 4 is dobule. Can you see two lines? or only one line?

I have no idea why only wavy line cannot be shown.

Comment 8

[Brian Wasserman]

I see a double line when set it to 4.  HTML and plain textarea have equivalent results.  I was looking at the changeset and was wondering if there is a way I can run layout/generic/test/test_selection_underline.html to help diagnose the problem.  I'm personally satisfied with the ui.SpellCheckerUnderlineStyle workaround, but I am willing to download the source and run some tests and help troubleshoot further since this problem may be unique to my hardware or system configuration.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> wondering if there is a way I can run
> layout/generic/test/test_selection_underline.html

If you have a tree that you built, yes.  Just need to make sure you build with --enable-tests and follow the directions at https://developer.mozilla.org/en/Mochitest#Running_select_tests

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get a regression range, and an explanation of why we think this blocks?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Mike, there's a regression range in comment 0.  The symptoms of this bug are that spellchecker is effectively not working for some users on Linux (in that it doesn't flag misspelled things).  Not sure whether you want to consider that a serious enough problem to block on.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we know which users of Linux? Yes, regressing the spellchecker is pretty serious, but I'd also be pretty surprised if that happened back in alpha 1 and we're only hearing about it now.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Do we know which users of Linux?

Unclear so far; almost certainly depends on the exact fonts installed on the system....

Comment 14

[Brian Wasserman]

Attachment: mochitest.log for test run on layout/generic/test

Comment 15

[Brian Wasserman]

(In reply to comment #14)
> Created an attachment (id=440172) [details]
> mochitest.log for test run on layout/generic/test

I tested with firefox-3.6.3.source.tar.bz2.  This is my first time compiling/testing any firefox code.  

I used .mozconfig:
ac_add_options --enable-application=browser --enable-tests
mk_add_options MOZ_CO_PROJECT=browser

In the mozilla-1.9.2/_tests/testing/mochitest director, I trued to run the 
test /tests/layout/generic/test/test_selection_underline.html, but all I got was a blank screen.  I then ran this:
 sudo python runtests.py --test-path=layout/generic/test/ --log-file=~/mochitest.log --file-level=DEBUG

It hangs on "Currently Executing: /tests/layout/generic/test/test_selection_underline.html"
 
I attached the mochitest.log file.  The summary is below:
Status: Fail
Passed: 699
Failed: 8
Todo: 2

Passed 	Failed 	Todo 	Test Files
0 	0 	0 	/tests/layout/generic/test/bug344830_testembed.svg
0 	0 	0 	/tests/layout/generic/test/bug421839-2-page.html
0 	0 	0 	/tests/layout/generic/test/file_BrokenImageReference.png
0 	0 	0 	/tests/layout/generic/test/file_Dolske.png
0 	0 	0 	/tests/layout/generic/test/file_IconTestServer.sjs
0 	0 	0 	/tests/layout/generic/test/file_LoadingImageReference.png
0 	0 	0 	/tests/layout/generic/test/file_bug514732_1.html
0 	0 	0 	/tests/layout/generic/test/file_bug514732_helper.html
0 	0 	0 	/tests/layout/generic/test/frame_selection_underline-ref.xhtml
0 	0 	0 	/tests/layout/generic/test/frame_selection_underline.css
0 	0 	0 	/tests/layout/generic/test/frame_selection_underline.xhtml
0 	0 	0 	/tests/layout/generic/test/plugin_clipping_helper.xhtml
0 	0 	0 	/tests/layout/generic/test/plugin_clipping_helper2.xhtml
0 	0 	0 	/tests/layout/generic/test/plugin_clipping_helper_table.xhtml
0 	0 	0 	/tests/layout/generic/test/plugin_clipping_helper_transformed.xhtml
0 	0 	0 	/tests/layout/generic/test/plugin_clipping_lib.js
20 	8 	0 	/tests/layout/generic/test/test_backspace_delete.xul
2 	0 	0 	/tests/layout/generic/test/test_bug263683.html - Bug 263683
2 	0 	0 	/tests/layout/generic/test/test_bug288789.html - Bug 288789
1 	0 	0 	/tests/layout/generic/test/test_bug323656.html - Bug 323656
3 	0 	0 	/tests/layout/generic/test/test_bug344830.html - Bug 344830
72 	0 	0 	/tests/layout/generic/test/test_bug348681.html - Bug 348681
2 	0 	0 	/tests/layout/generic/test/test_bug382429.html - Bug 382429
1 	0 	0 	/tests/layout/generic/test/test_bug384527.html - Bug 384527
1 	0 	0 	/tests/layout/generic/test/test_bug385751.html - Bug 385751
1 	0 	0 	/tests/layout/generic/test/test_bug389630.html - Bug 389630
1 	0 	0 	/tests/layout/generic/test/test_bug391747.html - Bug 391747
0 	0 	1 	/tests/layout/generic/test/test_bug392746.html - Bug 392746
1 	0 	0 	/tests/layout/generic/test/test_bug392923.html - Bug 392923
1 	0 	0 	/tests/layout/generic/test/test_bug394173.html - Bug 394173
1 	0 	0 	/tests/layout/generic/test/test_bug394239.html - Bug 394239
1 	0 	0 	/tests/layout/generic/test/test_bug402380.html - Bug 402380
1 	0 	0 	/tests/layout/generic/test/test_bug404872.html - Bug 404872
1 	0 	0 	/tests/layout/generic/test/test_bug405178.html - Bug 405178
3 	0 	0 	/tests/layout/generic/test/test_bug416168.html - Bug 416168
1 	0 	1 	/tests/layout/generic/test/test_bug421436.html - Bug 421436
1 	0 	0 	/tests/layout/generic/test/test_bug421839-2.html - Bug 421839
2 	0 	0 	/tests/layout/generic/test/test_bug438840.html - Bug 438840
1 	0 	0 	/tests/layout/generic/test/test_bug448860.html - Bug 448860
1 	0 	0 	/tests/layout/generic/test/test_bug460532.html - Bug 460532
4 	0 	0 	/tests/layout/generic/test/test_bug468167.html - Bug 468167
3 	0 	0 	/tests/layout/generic/test/test_bug469613.xul - Bug 469613
1 	0 	0 	/tests/layout/generic/test/test_bug469774.xul - Bug 469774
1 	0 	0 	/tests/layout/generic/test/test_bug470212.html - Bug 470212
2 	0 	0 	/tests/layout/generic/test/test_bug503813.html - Bug 503813
131 	0 	0 	/tests/layout/generic/test/test_bug514732.html - Bug 514732
1 	0 	0 	/tests/layout/generic/test/test_bug527306.html - Bug 527306
56 	0 	0 	/tests/layout/generic/test/test_movement_by_characters.html
280 	0 	0 	/tests/layout/generic/test/test_movement_by_words.html
25 	0 	0 	/tests/layout/generic/test/test_plugin_clipping.xhtml
36 	0 	0 	/tests/layout/generic/test/test_plugin_clipping2.xhtml
10 	0 	0 	/tests/layout/generic/test/test_plugin_clipping_table.xhtml
2 	0 	0 	/tests/layout/generic/test/test_plugin_clipping_transformed.xhtml
18 	0 	0 	/tests/layout/generic/test/test_plugin_mouse_coords.html
8 	0 	0 	/tests/layout/generic/test/test_plugin_position.xhtml
0 	0 	0 	/tests/layout/generic/test/test_selection_underline.html

Comment 16

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

test_selection_underline.html does nothing on Linux (and Mac)...

http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_selection_underline.html?force=1#93

The test can run only on Windows.

Comment 17

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

By comment 6 and comment 8, the common code for all styles should be working fine. So, the following three blocks should have the cause of this bug.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3303
> nsCSSRendering::PaintDecorationLine

> 3303     case DECORATION_STYLE_WAVY:
> 3304       aGfxContext->Save();
> 3305       contextIsSaved = PR_TRUE;
> 3306       aGfxContext->Clip(rect);
> 3307       if (lineHeight > 2.0) {
> 3308         aGfxContext->SetAntialiasMode(gfxContext::MODE_COVERAGE);
> 3309       } else {
> 3310         // Don't use anti-aliasing here.  Because looks like lighter color wavy
> 3311         // line at this case.  And probably, users don't think the
> 3312         // non-anti-aliased wavy line is not pretty.
> 3313         aGfxContext->SetAntialiasMode(gfxContext::MODE_ALIASED);
> 3314       }
> 3315       break;

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3363

> 3363     case DECORATION_STYLE_WAVY: {
> 3364       /**
> 3365        *  We are drawing wavy line as:
> 3366        *
> 3367        *  P: Path, X: Painted pixel
> 3368        *
> 3369        *     +---------------------------------------+
> 3370        *   XX|X            XXXXXX            XXXXXX  |
> 3371        *   PP|PX          XPPPPPPX          XPPPPPPX |    ^
> 3372        *   XX|XPX        XPXXXXXXPX        XPXXXXXXPX|    |
> 3373        *     | XPX      XPX      XPX      XPX      XP|X   |adv
> 3374        *     |  XPXXXXXXPX        XPXXXXXXPX        X|PX  |
> 3375        *     |   XPPPPPPX          XPPPPPPX          |XPX v
> 3376        *     |    XXXXXX            XXXXXX           | XX
> 3377        *     +---------------------------------------+
> 3378        *      <---><--->                                ^
> 3379        *      adv  flatLengthAtVertex                   rightMost
> 3380        *
> 3381        *  1. Always starts from top-left of the drawing area, however, we need
> 3382        *     to draw  the line from outside of the rect.  Because the start
> 3383        *     point of the line is not good style if we draw from inside it.
> 3384        *  2. First, draw horizontal line from outside the rect to top-left of
> 3385        *     the rect;
> 3386        *  3. Goes down to bottom of the area at 45 degrees.
> 3387        *  4. Slides to right horizontaly, see |flatLengthAtVertex|.
> 3388        *  5. Goes up to top of the area at 45 degrees.
> 3389        *  6. Slides to right horizontaly.
> 3390        *  7. Repeat from 2 until reached to right-most edge of the area.
> 3391        */
> 3392
> 3393       rect.pos.x += lineHeight / 2.0;
> 3394       aGfxContext->NewPath();
> 3395 
> 3396       gfxPoint pt(rect.pos);
> 3397       gfxFloat rightMost = pt.x + rect.Width() + lineHeight;
> 3398       gfxFloat adv = rect.Height() - lineHeight;
> 3399       gfxFloat flatLengthAtVertex = NS_MAX((lineHeight - 1.0) * 2.0, 1.0);
> 3400 
> 3401       pt.x -= lineHeight;
> 3402       aGfxContext->MoveTo(pt); // 1
> 3403 
> 3404       pt.x = rect.pos.x;
> 3405       aGfxContext->LineTo(pt); // 2
> 3406 
> 3407       PRBool goDown = PR_TRUE;
> 3408       while (pt.x < rightMost) {
> 3409         pt.x += adv;
> 3410         pt.y += goDown ? adv : -adv;
> 3411 
> 3412         aGfxContext->LineTo(pt); // 3 and 5
> 3413 
> 3414         pt.x += flatLengthAtVertex;
> 3415         aGfxContext->LineTo(pt); // 4 and 6
> 3416 
> 3417         goDown = !goDown;
> 3418       }
> 3419       aGfxContext->Stroke();
> 3420       break;
> 3421     }

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3513

> nsCSSRendering::GetTextDecorationRectInternal

> 3513   } else if (aStyle == DECORATION_STYLE_WAVY) {
> 3514     /**
> 3515      *  We will draw wavy line as:
> 3516      *
> 3517      * +-------------------------------------------+
> 3518      * |XXXXX            XXXXXX            XXXXXX  | ^
> 3519      * |XXXXXX          XXXXXXXX          XXXXXXXX | | lineHeight
> 3520      * |XXXXXXX        XXXXXXXXXX        XXXXXXXXXX| v
> 3521      * |     XXX      XXX      XXX      XXX      XX|
> 3522      * |      XXXXXXXXXX        XXXXXXXXXX        X|
> 3523      * |       XXXXXXXX          XXXXXXXX          |
> 3524      * |        XXXXXX            XXXXXX           |
> 3525      * +-------------------------------------------+
> 3526      */
> 3527     r.size.height = lineHeight > 2.0 ? lineHeight * 4.0 : lineHeight * 3.0;
> 3528     if (canLiftUnderline) {
> 3529       if (r.Height() > suggestedMaxRectHeight) {
> 3530         // Don't shrink the line height even if there is not enough space,
> 3531         // because the thickness has some meaning.  E.g., the 1px wavy line and
> 3532         // 2px wavy line can be used for different meaning in IME selections
> 3533         // at same time.
> 3534         r.size.height = NS_MAX(suggestedMaxRectHeight, lineHeight * 2.0);
> 3535       }
> 3536     }
> 3537   }

But I don't find any problem on these code...

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Until we know the size of the audience, we'll call this wanted but not blocking.

Comment 19

[Brian Wasserman]

(In reply to comment #12)
> Do we know which users of Linux? Yes, regressing the spellchecker is pretty
> serious, but I'd also be pretty surprised if that happened back in alpha 1 and
> we're only hearing about it now.

Is there anything else I can do to help track down this problem?  I agree that it would be difficult to determine the audience size.  I am not suggesting that it needs to be made a priority or categorized as being more serious than you suggest.

(In reply to comment #18)
> Until we know the size of the audience, we'll call this wanted but not
> blocking.

Even though users with the affected configuration probably is small, it would be virtually impossible to determine the size of this audience considering they would need to realize that the underlining has stopped and they would have to know how and be willing to report the bug.

Since my configuration does show the symptom when ui.SpellCheckerUnderlineStyle is 5, I am willing to help troubleshoot this some more.  Since the test_selection_underline.html test only works on Windows, is there an alternative test that can be created for Linux?  Alternatively, how about some console/debug output that can be added just for narrowing down the problem?  Do you have any suggestions?

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

I think we're on track to figuring out what's causing this bug; Masayuki, is there additional information you need from the person in comment 19?

As I said, we will take a fix for this bug, we're just not going to block a support release on it being fixed.

Comment 21

[Brian Wasserman]

At the very least, I don't mind verifying the fix on my configuration.  Although in a month or so I may upgrade from Ubuntu Karmic to Lucid, which may not continue to show the symptom.  If there is no fix by then then I can attempt to preserve a test environment by migrating my current machine to a VM, if it still has the problem described.

Comment 22

[Ian Nartowicz]

I still get this in 4b8.  Strangely, the wavy underline works in some pages.  An easy example is the review box for Mozilla addons.  Ironically I noticed it when about to write a review for the en-GB dictionary :)

Most pages though, do not show the standard underline but do show the other styles.

Linux, Ubuntu Jaunty 9.04 variant (Crunchbang).

Comment 23

[Ian Nartowicz]

In FF17 ESR, I see a wavy red underline with the default (no hidden preference) and with the preference set to "5".  Fixed?  All other values I tried also show some sort of red underlining.

Comment 24

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

Reopen if someone finds it's still broken on trunk.