790475 - Inline spellchecker does not correctly traverse text nodes

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jeff Gehlhaar]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.142 Safari/535.19

Steps to reproduce:

Started to type words, allowing spell check to do its thing.


Actual results:

Partial words are marked in error, then the red underline is not removed when the word is spelled correctly (and in total).


Expected results:

Spell check should reevaluate words as they get spelled and not leave partial words marked as incorrect.  Seems fine in 15.0.1, broken in 17.0a2 -- not sure where it broke.

Comment 1

[Thomas D. (:thomas8)]

wfm on current trunk: WinXP/18.0a1 
20120911030227 Mozilla/5.0 (Windows NT 5.1; rv:18.0) Gecko/18.0 Thunderbird/18.0a1

Comment 2

[Jeff Gehlhaar]

This bug appears to have surfaced in TB 16 for me.

Comment 3

[George Sheppard]

Not a TB user, but I have noticed this bug in Firefox since version 16.0
Only when using gmail and backspacing through a word, then retyping it.

described in this forum: http://forums.mozillazine.org/viewtopic.php?f=31&t=2571011&sid=b16ccf58bf408f933121729f6860271e

Some users could not reproduce this bug. I set up a blank gmail account, and sure enough, I couldn't reproduce the bug there.

It seems to be a problem with the gmail interface when you have chosen a preformatted text setting. By clearing any preformatted setting, the error corrected itself.

This was not happening in previous versions of FF.

Comment 4

[Vincent (caméléon)]

More than 5 people report it on GSFN with TB16, so I confirmed the bug report.

Comment 5

[Mark Banner (:standard8)]

It would be very useful to get some steps to repeat on here. Browsing the forum mentioned in comment 3 I cannot repeat any of those steps.

Things to consider when generating steps to repeat: Include all key presses, mouse clicks. Also include if you're using the default fonts, special fonts, non-en-US dictionaries, version of Thunderbird.

Also, can you reproduce the issue on http://www-archive.mozilla.org/editor/midasdemo/

Comment 6

[Vincent (caméléon)]

see also http://forums.mozillazine.org/viewtopic.php?f=31&t=2571011 which as some STR, but which doesn't works for all users (personally I don't succeed to reproduce it).

Comment 7

[Vincent (caméléon)]

and also http://support.mozilla.org/ca/questions/939293 for the Firefox side of this issue (10 peoples have this problem).

Comment 8

[Mark Banner (:standard8)]

Given the fact this seems to affect FF as well, moving to the core component.

Comment 9

[Wayne Mery (:wsmwk)]

question to those how can reproduce - does it reproduce with current trunk?

I can't reproduce with TB18, html or plain text compose, and the mozillazine steps

1) Open a new email message.
2) Type 'In the middle of the day'
3) Backspace until you only have 'In the mi'
4) Then retype the rest so you end up with 'In the middle of the day' again.

However, some say it is intermittent

Comment 10

[LGLC]

I have been experiencing this since upgrading to Thunderbird 16.0.1.  I'm using the en-GB dictionary, with 'Helvetica, Arial' as the font, entered as 'Body Text'.

Here are the steps that enable me to reproduce the bug:

1) Open up Thunderbird (password required for me).
2) Go to Inbox for one of my IMAP accounts.
3) Click on 'Write' to compose a new email.
4) Click inside the main message area.  (My HTML signature has appeared at the bottom of the new email.)
5) Type 'I am testung ' (the word 'testung', purposely spelt incorrectly, followed by a space).
6) Press backspace 4 times, leaving just 'I am test'.  Currently nothing is underlined as red.
7) Type 'ing' (to make it into the word 'testing'), followed by a space.
8) Begin typing any word at all (e.g. 'this') and the moment the first non-space character is entered, the red underline comes up underneath 'ing', hich is treated as a separate word.
9) Right-clicking gives me the spelling correction options of 'in', 'sing', 'ring', 'ting', 'ling'.  

It appears to be an issue with backspacing into a word and then any new characters typed being treated as a new word entirely, as if you'd entered a space beforehand.

For me, the issue only occurs in Thunderbird.  I have been unable to recreate it in Firefox 16.0.1 (using the en-GB dictionary), even within Gmail.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to LGLC from comment #10)
> I have been experiencing this since upgrading to Thunderbird 16.0.1.  I'm
> using the en-GB dictionary, with 'Helvetica, Arial' as the font, entered as
> 'Body Text'.
> 
> Here are the steps that enable me to reproduce the bug:
> 
> 1) Open up Thunderbird (password required for me).
> 2) Go to Inbox for one of my IMAP accounts.
> 3) Click on 'Write' to compose a new email.
> 4) Click inside the main message area.  (My HTML signature has appeared at
> the bottom of the new email.)
> 5) Type 'I am testung ' (the word 'testung', purposely spelt incorrectly,
> followed by a space).
> 6) Press backspace 4 times, leaving just 'I am test'.  Currently nothing is
> underlined as red.
> 7) Type 'ing' (to make it into the word 'testing'), followed by a space.
> 8) Begin typing any word at all (e.g. 'this') and the moment the first
> non-space character is entered, the red underline comes up underneath 'ing',
> hich is treated as a separate word.
> 9) Right-clicking gives me the spelling correction options of 'in', 'sing',
> 'ring', 'ting', 'ling'.  
> 
> It appears to be an issue with backspacing into a word and then any new
> characters typed being treated as a new word entirely, as if you'd entered a
> space beforehand.
> 
> For me, the issue only occurs in Thunderbird.  I have been unable to
> recreate it in Firefox 16.0.1 (using the en-GB dictionary), even within
> Gmail.

I was able to reproduce this issue using these steps, but ONLY when I had the font set to Helvetica, Arial. The dictionary choice between US, Canadian and British English didn't affect this.

Setting the font to Variable Width made the problem go away.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

This problem seems to affect Release, all the way to Daily.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

I was able to reproduce this in Firefox 16.0.1 with the following STR:

1) Go to http://www-archive.mozilla.org/editor/midasdemo/
2) Change the font to Arial
3) In the textarea, type "I am testung"
4) Press space - you should see the red squiggly under "testung"
5) Hit backspace 4 times, so you just have "I am test"
6) Type "ing", followed by a space, followed by any alphanumeric character.

What happens?

"ing" in "testing" gets marked with a red squiggly as being incorrectly spelled.

What's expected?

"testing" should be recognized as a single word, and a valid one at that.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

After a long period of spell-check brokenness, the bug is first reproducible on the Firefox Nightly on July 14th, 2012.

According to DOMi, what's happening is that the text is wrapped in a FONT tag, and is actually composed of two text nodes.

The first contains "I am test", and the other contains "ing".

If I don't set the font first, no font node exists, and there's only a single text node containing "I am testing".

Standard8 and I are starting to suspect editor on this one.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Here's an FTP for the first busted Nightly:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/07/2012-07-14-03-05-54-mozilla-central/

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Yeah, the regression window I had before wasn't very good - I was doing my regression testing on Windows, and a bunch of those builds were busted for various reasons.

I was able to drill it down on OSX though.

Last good Nightly was on May 18th.
First bad was on May 19th.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

And here are the changesets between those two Nightlies:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e794cef56df6&tochange=642d1a36702f

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Bisecting revealed this to be a regression caused by bug 590640 landing.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

We would not chemspill for this so not tracking for 16, but assigning to Aryeh to do some investigation on possible fix/backout for this regression.  If you have something speculative we can take it in the next week and a half for consideration on 17.

Comment 20

[(no longer active)]

So this is caused by the fact that our spellchecker is stupid and cannot handle adjacent textnodes, and we need to fix that.  Aryeh, I'd appreciate if you can take a look at this.

Comment 21

[Jeff Gehlhaar]

I have seen this problem with the Calibri font as well.

Comment 22

[R Benz]

I am also seeing these odd spelling markings.  I did some experimenting and I can see this strange behavior in Thunderbird 16 that wasn't in Thunderbird 15.

Create a new message and enter several lines of text.  Highlight all of the lines of your email text with CTRL-A, then click on Toolbar Insert > HTML.  Add a [font size=3] at the very beginning of your text and [/font] at the very end of all your text.  NOTE: change the [ to < and the ] to > in last sentence.

Then go back to your email body and insert more text in the middle of your email, backspace, pause, add, and correct the text.

Again do a CTRL-A and highlight all your lines, Go to Toolbar Insert > HTML.

What I then see is that Thunderbird has added multiple instances of the [font size=3] throughout my email message, often times breaking a word with that HTML.  So the red underlined words look like this:

In my regular email text body, looks like this:  Check this crazy stuff out.

In the Insert > HTML view, looks like this:  Check this c[font size=3]razy stu[font size=3]ff out.

I get the red underlines in the word where the extra [font size=3] is being generated and inserted in the words.

NOTE: I used [ for the < character, and the ] for the > character to prevent the removal of the HTML tags in my example.

Comment 23

[R Benz]

In my previous comments... I am seeing this in Thunderbird 16.0.1 on Windows XP SP3.  After I entered my above comments, I noticed that this is marked for "OS X", but I am seeing this on Windows XP platform.  I don't know how to add to the Bugzilla top details that this is also happening on Windows XP platform.

Comment 24

[(no longer active)]

(In reply to comment #21)
> I have seen this problem with the Calibri font as well.

Right.  The actual font probably doesn't matter.  The only reason why changing the font is required to reproduce it is to get us into a situation where we create two adjacent textnodes.

Comment 25

[Tony Mechelynck [:tonymec]]

(In reply to R Benz from comment #23)
> In my previous comments... I am seeing this in Thunderbird 16.0.1 on Windows
> XP SP3.  After I entered my above comments, I noticed that this is marked
> for "OS X", but I am seeing this on Windows XP platform.  I don't know how
> to add to the Bugzilla top details that this is also happening on Windows XP
> platform.

If the bug is seen on both OS X and Windows (or on two or more of Windows, Linux and Mac), it deserves "OS: All". To set that setting, you need EDITBUGS privileges on bugzilla.mozilla.org, except for bugs which you reported (which you can always edit). So if you don't have the necessary privs, just mention the fact (as you did) and someone will make the change (as I'm now doing).

Comment 26

[George Sheppard]

(In reply to Tony Mechelynck [:tonymec] from comment #25)

> If the bug is seen on both OS X and Windows (or on two or more of Windows,
> Linux and Mac), it deserves "OS: All". To set that setting, you need
> EDITBUGS privileges on bugzilla.mozilla.org, except for bugs which you
> reported (which you can always edit). So if you don't have the necessary
> privs, just mention the fact (as you did) and someone will make the change
> (as I'm now doing).

I have this bug on Linux.

Comment 28

[Greta]

Thunderbird 16, Fedora 17 64-bit is where the problem started for me.  Comment 9 illustrates the problem I have.  If I see the red underline while I'm still typing on the same line, if I then type a space followed by a backspace, the red underline goes away.

Comment 29

[(no longer active)]

Thanks everyone for trying to help here.  I believe that at this point we have enough information about why this bug happens and which Firefox and Thunderbird versions if affects, and how we should fix it.  Adding comments that are not directly related to fixing this bug just makes it harder for us to focus on it, so I would appreciate if you would consider voting for the bug instead.  Thanks!  :-)

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> So this is caused by the fact that our spellchecker is stupid and cannot
> handle adjacent textnodes, and we need to fix that.  Aryeh, I'd appreciate
> if you can take a look at this.

So I think what's happening here is that when a text node changes, in some cases we're invalidating only that node, not neighboring ones.  I haven't found yet where this is happening, but it should be a simple fix once I do.  Changing one text node really needs to invalidate the whole block.

Comment 33

[(no longer active)]

(In reply to comment #30)
> (In reply to Ehsan Akhgari [:ehsan] from comment #20)
> > So this is caused by the fact that our spellchecker is stupid and cannot
> > handle adjacent textnodes, and we need to fix that.  Aryeh, I'd appreciate
> > if you can take a look at this.
> 
> So I think what's happening here is that when a text node changes, in some
> cases we're invalidating only that node, not neighboring ones.  I haven't found
> yet where this is happening, but it should be a simple fix once I do.  Changing
> one text node really needs to invalidate the whole block.

Agreed.

Comment 34

[Dan Moore]

This may help.  First I use FF 15.0 and TB 16.0.  I only have issues with the TB.  Others in the office use TB 16.0 without issue.  So it is random. I have tried changing fonts, variable vs. fixed and even other dictionaries with the same negative results.

I just typed a note and emailed it to myself.  The received email had an extra space between the words TB had thought were misspelled.  In my email I back spaced into system (and TB underlined only tem)and the next word was and.  The received email had two spaces between them (system _ _ and)  So I think the composer is not displaying what it sees.  It may think the first space after retyping letters is a character not a space.

Comment 35

[(no longer active)]

(In reply to comment #34)
> I just typed a note and emailed it to myself.  The received email had an extra
> space between the words TB had thought were misspelled.  In my email I back
> spaced into system (and TB underlined only tem)and the next word was and.  The
> received email had two spaces between them (system _ _ and)  So I think the
> composer is not displaying what it sees.  It may think the first space after
> retyping letters is a character not a space.

That's a different bug.  Can you please report it separately?  Thanks!

Comment 36

[Dan Pernokis]

Dan Moore:  The issue is not random just because others are using TB16.  The question is whether they are using the CHECK-AS-YOU-TYPE option which seems to be faulty in this release.

Comment 37

[dan.amsentry]

The other PC does have TB16.0.1 and Check as you type is enabled.  Since I forgot to uncheck "check for updates" on TB when I downgraded to 15 it went back to TB16 and the unwanted spell check as you type came back.  I now have FF16 and I do not have a problem when composing in Gmail.  I downloaded the new dictionary, but when I go to options and try to select the new US dictionary with the fix I don't see any new options, so I assume it is overwriting the existing.  Does anyone know if I need to delete the existing US dictionary and install the fix one?

Comment 38

[dan.amsentry]

When I sent the email to myself with the extra spaces here is the html

<font size="+1">This is a test of the sp<font size="+1">ell che<font
          size="+1">ck sys<font size="+1">tem&nbsp; and its results <font
              size="+1">w<font size="+1">hen typ<font size="+1">ing&nbsp; w<font
                    size="+1">ords</font></font></font></font></font></font></font><br>

Comment 39

[Mike Conley (:mconley) (:⚙️)]

I'll try to take this.

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

This patch addresses the bug exposed by the STR. Editor reftests pass, as well as the mochitests under editor/libeditor/text/tests.

Going to toss this up on try to see what we get.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Try builds coming in here: https://tbpl.mozilla.org/?tree=Try&rev=08c4bbb29212

Comment 42

[Joe Sabash [:JoeS1]]

Testing with the midas demo and a try build:
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:19.0) Gecko/19.0 Firefox/19.0
I'm no longer able to reproduce.
Given the importance of bug 590640 for TB, I'm happy the fix could be made in mozInlineSpellWordUtil.cpp rather than endangering the fixes there.

Comment 43

[(no longer active)]

Comment on attachment 674808 [details] [diff] [review]
Patch v1

Review of attachment 674808 [details] [diff] [review]:
-----------------------------------------------------------------

Here's my comments on the patch.  Of course this needs a mochitest as well, and writing one should be fairly easy based on the STR in the bug.

::: extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
@@ +459,5 @@
>    nsINode* node = mSoftBegin.mNode;
>    int32_t firstOffsetInNode = 0;
>    int32_t checkBeforeOffset = mSoftBegin.mOffset;
>    while (node) {
> +

Nit: drop the blank line please.

@@ +461,5 @@
>    int32_t checkBeforeOffset = mSoftBegin.mOffset;
>    while (node) {
> +
> +    if (IsTextNode(node) &&
> +        ContainsDOMWordSeparator(node, checkBeforeOffset, &firstOffsetInNode)) {

This is adding a bug, since if IsTextNode returns true, IsBRElement called inside ContainsDOMWordSeparator can never return true, so we'll break those cases.

This made me think about how we can make this code better.  Maybe an easy way would be to refactor the stuff after "aNode is actually an nsIContent..." into its own function, say, TextNodeContainsDOMWordSeparator, don't touch this part of the code, and down in the inner loop call TextNodeContainsDOMWordSeparator directly.

@@ +472,5 @@
>            // of ContainsDOMWordSeparator here because there might be no preceding
>            // word separator (such as when we're at the end of the first word in
>            // the text node), in which case we just set the found offsets to 0.
>            // Otherwise, ContainsDOMWordSeparator finds us the correct word
>            // boundary so that we can avoid looking at too many words.

This comment is no longer accurate, please correct it.

@@ +474,5 @@
>            // the text node), in which case we just set the found offsets to 0.
>            // Otherwise, ContainsDOMWordSeparator finds us the correct word
>            // boundary so that we can avoid looking at too many words.
> +          if (!ContainsDOMWordSeparator(node, firstOffsetInNode - 1, &newOffset)) {
> +            nsINode* prevNode = node->GetPreviousContent(mRootNode);

You should do GetPreviousSibling here.

@@ +477,5 @@
> +          if (!ContainsDOMWordSeparator(node, firstOffsetInNode - 1, &newOffset)) {
> +            nsINode* prevNode = node->GetPreviousContent(mRootNode);
> +            while (prevNode && IsTextNode(prevNode)) {
> +              if (ContainsDOMWordSeparator(prevNode, INT32_MAX, &newOffset))
> +                break;

See above.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Thanks for the review, Ehsan. Any better?

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 46

[(no longer active)]

Comment on attachment 675227 [details] [diff] [review]
Patch v2

Review of attachment 675227 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
@@ +492,5 @@
>          int32_t newOffset = 0;
>          if (firstOffsetInNode > 0) {
> +          // Try to find the previous word boundary in the current node. If
> +          // we can't find one, start checking previous sibling nodes (if any)
> +          // to see if we can find any text nodes with DOM word separators.

s/any/any adjacent/

@@ +508,5 @@
> +                                                   &newOffset)) {
> +                break;
> +              }
> +              prevNode = prevNode->GetPreviousSibling();
> +              mSoftBegin.mNode = prevNode;

This code has a bug.  If TextNodeContainsDOMWordSeparator returns true on the first iteration for example, mSoftBegin.mNode will point to the next node.

Comment 47

[(no longer active)]

Comment on attachment 675228 [details] [diff] [review]
Regression test

Review of attachment 675228 [details] [diff] [review]:
-----------------------------------------------------------------

I think it's better to test the behavior of multiple adjacent textnodes explicitly, since we might change something in the future which would prevent the creation of multiple textnodes, and then this will end up being untested.

Comment 48

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

Thanks for the review - and good catches. Fixed.

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test v2

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test v3

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Try for the latest patch for m-c: https://tbpl.mozilla.org/?tree=Try&rev=f55f364f9ae0

I'm also doing a Thunderbird try as well: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=ad320e7e5772

Comment 52

[(no longer active)]

Comment on attachment 675259 [details] [diff] [review]
Regression test v3

Review of attachment 675259 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/tests/test_bug790475.html
@@ +27,5 @@
> +
> +var gMisspeltWords;
> +
> +function getEditor() {
> +  var Ci = Components.interfaces;

Nit: const.

@@ +28,5 @@
> +var gMisspeltWords;
> +
> +function getEditor() {
> +  var Ci = Components.interfaces;
> +  var win = window;

What's the point of doing this?

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test v4 (r+'d by ehsan)

Thanks ehsan - nits fixed.

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Landed on inbound:

Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/72e6fd5a0358
Regression test: https://hg.mozilla.org/integration/mozilla-inbound/rev/a75c3d861bf8

Comment 55

[Mike Conley (:mconley) (:⚙️)]

I've got some try builds here - unfortunately, my Windows ones didn't go through (mozilla-central patch didn't apply cleanly on them for some reason), so I only have Linux and OSX.

I'd love it if some people could try them out and see if we've addressed the bug:

Linux: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-ad320e7e5772/try-comm-central-linux/thunderbird-19.0a1.en-US.linux-i686.tar.bz2

OSX: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-ad320e7e5772/try-comm-central-macosx64/thunderbird-19.0a1.en-US.mac.dmg

If the patch I just landed on inbound sticks, Windows users might be able to test in Daily either tomorrow or the next day.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72e6fd5a0358
https://hg.mozilla.org/mozilla-central/rev/a75c3d861bf8

Comment 57

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 675255 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 590640
User impact if declined: Broken behaviour for inline spellcheck - particularly evident in Thunderbird.
Testing completed (on m-c, etc.): Simple regression test. We'd need more people to kick the tires on this - especially if this is something we wanted to fix in the ESRs...
Risk to taking this patch (and alternatives if risky): The inline spellchecking code is pretty complex, and doesn't have much in the way of automated testing, so while I believe I've fixed the original problem, I can't guarantee I haven't introduced any new problems. The alternative is simply to keep having busted inline spellcheck. Backing out 590640 isn't really an option, and might be riskier than taking this patch.
String or UUID changes made by this patch: None.

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 675255 [details] [diff] [review]
Patch v3

See above.

Comment 59

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 675255 [details] [diff] [review]
Patch v3

ESR 17 hasn't branched yet - de-requesting approval.

Comment 60

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 675255 [details] [diff] [review]
Patch v3

Go ahead and land to branches so we can get the tire kicking needed.  Getting this in now will put it in ESR 17 for free, it doesn't fit the criteria for ESR uplift so we wouldn't take it for ESR 10.

Comment 61

[Lukas Blakk [:lsblakk] use ?needinfo]

adding qawanted and verifyme to make sure we get some testing around this once it's on Beta.

Comment 62

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/94e58a4f2153
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e92a75116bb
https://hg.mozilla.org/releases/mozilla-beta/rev/6efc3102c810
https://hg.mozilla.org/releases/mozilla-beta/rev/8fb3c3ea5e07

Comment 66

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0

Could reproduce the initial issue on an older beta when using Arial font as in comment 13. 

Could no longer reproduce the initial issue in 17 beta 4. Verified with Windows 7, Mac OS 10.7 and Ubuntu 12.04. 
Misspelled words are underlined as expected, correctly spelled words are not underlined. Used random fonts within Preferences/Content, mail.yahoo.com, gmail.com.
Also installed a Romanian dictionary, everything worked as expected afterwards as well.

Comment 67

[sfhowes]

Still getting correctly-spelled words partially flagged (red line) with English (CA) dictionary, Variable width, on Earlybird (TB 18, Nov. 10)/XP.

Comment 68

[Mike Conley (:mconley) (:⚙️)]

(In reply to sfhowes from comment #67)
> Still getting correctly-spelled words partially flagged (red line) with
> English (CA) dictionary, Variable width, on Earlybird (TB 18, Nov. 10)/XP.

Can you give us some steps to reproduce?

Comment 69

[sfhowes]

(In reply to Mike Conley (:mconley) from comment #68)
> (In reply to sfhowes from comment #67)
> > Still getting correctly-spelled words partially flagged (red line) with
> > English (CA) dictionary, Variable width, on Earlybird (TB 18, Nov. 10)/XP.
> 
> Can you give us some steps to reproduce?

I wish I could, but it seems to appear randomly.  I even replied to the same message with the same words, but this time there were no issues.

Comment 70

[R Benz]

I think the real root cause of this problem is not in the spelling logic.  Rather, I see that Thunderbird is putting random <font size="3"> tags throughout my email message.  When these font size tags split a word, that is when the spelling logic flags it as bad.

For example:  When I type this line

The HTML email message has:  <font size="3">When I ty<font size="3">pe thi<font size="3">s lin<font size="3">e

So spelling would not be flagging this as incorrect if all those extraneous font size tags were not getting put in there by mistake.

So is this a spelling logic issue ... or is this an issue with the font size tags being generated randomly and incorrectly?

Comment 71

[(no longer active)]

This was a spell checking logic issue.

Comment 72

[Joe Smith]

(In reply to Ehsan Akhgari [:ehsan] from comment #71)
> This was a spell checking logic issue.

Does that mean that we need to submit another bug for the massively superfluous addition of <font> information (in e.g. TB 16.0.2)? Because that's what became visible as the "spell check issue" to the user. The <font> info is being added randomly, with no reason, sometimes within a word, which then for the spell checker splits the word. (Which one might even argue could be an expected behavior on the part of the spell checker, e.g. with artificially put together words e.g. in company logos, etc., when the font is manually changed within the word by the user.) One can see it when they save an e-mail to the drafts and then open the source code view of the e-mail; or view the source code of an already sent e-mail.

Another side effect is that the e-mail size grows significantly with no reason.

Comment 73

[(no longer active)]

Yes, if you can reproduce, please file another bug with exact steps to reproduce.

Comment 74

[R Benz]

(In reply to Ehsan Akhgari [:ehsan] from comment #73)
> Yes, if you can reproduce, please file another bug with exact steps to
> reproduce.

I submitted Bug 812638 - Thunderbird is inserting random and incorrect <font size="3"> tags in the middle of words throughout my email message.

Comment 75

[Ken Saunders]

Hey I hate to bother everyone getting this, and I'm not sure if this is uncommon or not, and, if I had a better way to do this then I would, but I wanted to send sincere thanks for fixing this. 
You all rock.

This bug drove me nuts. It interrupted my workflow and distracted me. 
Kudos.

Consider a virtual beer, or whatever your favorite beverage is sent. :)

Comment 76

[Joe Smith]

Unfortunately, this is still broken for me in the TB 17.0 release. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=812638#c11 for reproduction procedure.

Comment 77

[Gary Schweinshaupt]

Still have the problem. Updated both Firefox and Thunderbird to version 17, nothing seems to have changed. Agree with Comment 75.

Have resorted to composing messages with HTML turned off, which is what I did with version 16.

Not only is spell check problem still there, when composing in HTML, when spell check error occurs, outgoing message text changes font part-way down the message.

I was holding off on installing pre-version 16 waiting for your fix, now guess I must try to go back to version 15.

Comment 78

[Ken Saunders]

Unfortunately, this issue has just made an appearance again.
It's the first time since updating to 17 and oddly enough, it's only happened once and I can't reproduce it (sorry).

I typed resources > data and urces was underlined with "sources" as one of the spell check suggestions.

I first of all have to add my "I'm not a coder so I really don't know what I'm talking about and I'm just throwing darts" line, and I haven't read all of the comments but I'm wondering.

Has this bug been reproduced in a new profile? I can't check that since it's only occurred once in 17.0. I could try 16.0 or 0.2 if anyone thinks it would help.

I ask because I have a decent amount of add-ons in my default profile and the profile is very old.

I don't remember this issue going all the way back to 10/9 or 11, but it may have on 10/29 with the update, I'm not sure but was it an update that has caused this, or some sort of corruption in a file? Or is that known?

Comment 79

[Lukas Blakk [:lsblakk] use ?needinfo]

Anthony/Juan - can you try reproducing in FF17 with or without a new profile?

Comment 80

[Virgil Dicu [:virgil] [QA]]

If anyone can reproduce, mind specifying the fonts you use, the dictionaries the steps to reproduce (if you can reproduce reliably)?

FTR, this was initially reproduced consistently with the Arial font. Comment 13 lists the initial steps. These work for me with F17. If anyone wants to try to reproduce, those can be a good start (without switching to Arial and keeping your own fonts).

Comment 81

[(no longer active)]

(In reply to comment #80)
> If anyone can reproduce, mind specifying the fonts you use, the dictionaries
> the steps to reproduce (if you can reproduce reliably)?
> 
> FTR, this was initially reproduced consistently with the Arial font. Comment 13
> lists the initial steps. These work for me with F17. If anyone wants to try to
> reproduce, those can be a good start (without switching to Arial and keeping
> your own fonts).

The font being used doesn't (shouldn't!) make any difference.  The most interesting bits are the source for the original email and the exact editing operations performed on it.  FWIW, I'm fairly confident that the original bug here has been fixed, and what we're seeing is an unrelated issue which has the same manifestation.

Comment 82

[Serg Iv]

here's two screencasts with def. font and with changed font (to courier).

http://db.tt/DgcAAuag
http://db.tt/Lpyi71Yy

but it's not new issues. my ancient TB14 has exactly the same behavior so i'm not sure if my report can help you.

Comment 83

[Joe Smith]

(In reply to Ehsan Akhgari [:ehsan] from comment #81)
> (In reply to comment #80)
> > If anyone can reproduce, mind specifying the fonts you use, the dictionaries
> > the steps to reproduce (if you can reproduce reliably)?
> > 
> > FTR, this was initially reproduced consistently with the Arial font. Comment 13
> > lists the initial steps. These work for me with F17. If anyone wants to try to
> > reproduce, those can be a good start (without switching to Arial and keeping
> > your own fonts).
> 
> The font being used doesn't (shouldn't!) make any difference.  The most
> interesting bits are the source for the original email and the exact editing
> operations performed on it.  FWIW, I'm fairly confident that the original
> bug here has been fixed, and what we're seeing is an unrelated issue which
> has the same manifestation.

1. As repetition of comment 76, please see https://bugzilla.mozilla.org/show_bug.cgi?id=812638#c11 for step-by-step reproduction procedure on Thunderbird (also release 17.0). The issue is 100% reproducible. The "spell checking brocken" issue occurs when the <font> info is placed within of a word. (Additionally to the backspacing described in the reproduction procedure, one could also try other in-text modifications, e.g. replacement, insertion within written text.)

2. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=812638#c8 for used dictionaries. (I don't think they matter.)

2. It does, indeed, not depend of the used font (size) - it just needs to be non-default.

Comment 84

[Ken Saunders]

Results (17.0)

I am writing to you about what's up. Spell check reproducing bug

    <font size="-1"><font face="Helvetica, Arial, sans-serif">Hi,<br>
        <br>
        <font size="-1"><font face="Helvetica, Arial, sans-serif">I am
            writ<font size="-1"><font face="Helvetica, Arial,
                sans-serif">ing to you abo<font size="-1"><font
                    face="Helvetica, Arial, sans-serif">ut what's up.
                    Spell check reproduc<font size="-1"><font
                        face="Helvetica, Arial, sans-serif">ing bug</font></font></font></font></font></font></font></font><br>
      </font></font>

Changing font to Arial. We'll see what happens 

    <font size="-1">Chang<font size="-1">ing font to Ari<font size="-1">al<font
            size="-1">. We'll see w<font size="-1">hat happens</font></font></font></font><br>
    </font>

Fixed width font. Does this work? I suppose I'll find out

    <font size="-1"><tt>Fixed width f<tt><font size="-1">ont<tt><font
                size="-1">. Does this wo<tt><font size="-1">rk?<tt><font
                        size="-1"> I suppo<tt><font size="-1">se I'll
                            find out</font></tt></font></tt><br>
                  </font></tt></font></tt></font></tt></tt></font>

Now some notes.
After saving and editing again, the (supposedly) misspelled words are not underlined as they were when composing.

When replying to an email, this issue has only occurred twice now for me (despite what's shown above), since updating to 17 but it happened while composing a new email or replying to one with 16.0.1 (or 2) every time.

Comment 85

[Joe Smith]

(In reply to Gary Schweinshaupt from comment #77)
> [...]
> Not only is spell check problem still there, when composing in HTML, when
> spell check error occurs, outgoing message text changes font part-way down
> the message.
> [...]

That could be one of the following (related?):
bug 664615
bug 782215
bug 780406

Comment 86

[R Benz]

(In reply to Ken Saunders from comment #84)
> Results (17.0)
> 
> I am writing to you about what's up. Spell check reproducing bug
> 
>     <font size="-1"><font face="Helvetica, Arial, sans-serif">Hi,<br>
>         <br>
>         <font size="-1"><font face="Helvetica, Arial, sans-serif">I am
>             writ<font size="-1"><font face="Helvetica, Arial,
>                 sans-serif">ing to you abo<font size="-1"><font
>                     face="Helvetica, Arial, sans-serif">ut what's up.
>                     Spell check reproduc<font size="-1"><font
>                         face="Helvetica, Arial, sans-serif">ing
> bug</font></font></font></font></font></font></font></font><br>
>       </font></font>
> 
> [...]

This looks like the problem being described in: Bug 812638 - Thunderbird is inserting random and incorrect <font size="3"> tags in the middle of words throughout my email message.  You might want to post your comments in that bug report regarding the insertion of font tags in the middle of words.

Comment 87

[R Benz]

(In reply to Joe Smith from comment #85)
> That could be one of the following (related?):
> bug 664615
> bug 782215
> bug 780406
  Bug 812638 might also be related to this.

Comment 88

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18.0 Firefox/18.0

Marking as verified with Firefox 18 beta 1 on Windows 7, Mac OS 10.7 and Ubuntu 12.04.
The initial issue described in comment 13 is fixed. Logged a similar bug in the process of verifying: bug 816073 (regression from Firefox 15).

Comment 89

[Virgil Dicu [:virgil] [QA]]

None of the issues reported in the last few comments have spun off from this bug, so I'll remove qawanted. 
Please re-add if there's anything left to do/test.

Comment 90

[G. R. Main]

NOT FIXED!

Problem persists in TB17.0

Comment 91

[Mike Conley (:mconley) (:⚙️)]

(In reply to G. R. Main from comment #90)
> NOT FIXED!
> 
> Problem persists in TB17.0

Hello G.R. Main.

If you're seeing the described symptoms after following the precise steps in comment 13, we'll consider re-opening this bug.

If not, what you're seeing is likely a separate bug that has a similar manifestation. Please see the comments above for the related bugs that have been filed, and please comment and follow those, as opposed to this closed bug.

Thanks!

-Mike

Comment 92

[G. R. Main]

Sir,
Problem persists:

TB 17.0
Linux (Linux Mint 12 64Bit)
WinXP Sp3 (32Bit)

With Composition Perfs set to default for HTML ( Variable Width, Medium size)
Problem appears to be gone.

Set Composition Perfs for HTML to Helvectia,Arial Large Size,
Problem of inserting " <font size="+1"> " reappears. 

View of Source Code:

User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0
MIME-Version: 1.0
X-Enigmail-Version: 1.4.6
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <font size="+1"><font face="Helvetica, Arial, sans-serif">Now is the
        time for all good men<font size="+1"> to come to th<font
            size="+1">e aid o<font size="+1">f th<font size="+1">eir
                count<font size="+1">ry.<br>
                  <font size="+1">The quick brown fox jumped over the
                    lazy dog's back 134567890 times.<br>
                    <br>
                    <font size="+1">Now is the time for all good men to
                      come to <font size="+1">the aid o<font size="+1">f
                          t<font size="+1">heir <font size="+1">country.<br>
                              <font size="+1">The quick brown fox j<font
                                  size="+1">ump<font size="+1">ed over
                                    the l<font size="+1">azy dog's back
                                      1234567890 time<font size="+1">s.<br>
                                        <br>
                                        <br>
                                      </font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font>
  </body>
</html>


End of Source Code

Cheers,

    G. R. Main

Comment 93

[Thomas D. (:thomas8)]

Whatever the technical relation of this bug to bug Bug 812638, let's add a link for ease of tracking and analysis, per Bug 812638 Comment 2.

Comment 94

[Thomas D. (:thomas8)]

(In reply to Mike Conley (:mconley) from comment #91)
> (In reply to G. R. Main from comment #90)
> > NOT FIXED!
> > Problem persists in TB17.0

+1

> If you're seeing the described symptoms after following the precise steps in
> comment 13, we'll consider re-opening this bug.

(In reply to Mike Conley (:mconley) from comment #13)
> I was able to reproduce this in Firefox 16.0.1 with the following STR:
> 1) Go to http://www-archive.mozilla.org/editor/midasdemo/
> 2) Change the font to Arial
> 3) In the textarea, type "I am testung"
> 4) Press space - you should see the red squiggly under "testung"

Mike, typing misspelled English words like "testung" into *Midasdemo* per your STR does *not* show any red squiggly underlining for me. Does it for you?

Otherwise, basically following your STR of comment 13 in Thunderbird reproduces this bug for me every time (both release & trunk), albeit only if using certain size values for deprecated font size attribute in <font size="x"></font> around the msg body (which is what we use when changing composition's default font size in preferences!).

STR

1 compose new HTML msg
2 in msg body, type misspelled word "I am testung " (sic! testung followed by one space)
3 Ctrl+A, Insert > HTML
4 add font tags around msg body HTML, then [Insert], like this:

<font size="x">I am testung <br></font>
where x must be between 1 and 6 (sic!), or between -3 and +3 (sic!);
surprisingly, bug will *not* occur for
- max values of font size attribute (7, +4)
- invalid values of font size attribute (>7, or >+4, e.g. 8 or +5 etc.)
- non-default font *face* attributes aka <font face="Calibri"></font>

5 hit backspace 4 times to erase "ung "
6 type "ing x" to make it "I am testing x" where x is any alphanumeric character
7 observe spell check marks on "testing" (red squiggly)
8 verify HTML source (Ctrl+A, Insert > HTML)

Actual result

7 "ing" part of "testing" is marked as misspelled (red squiggly) - this bug
8 redundant font tags are wrongly added in source (emphasis stars added by me), Bug 812638:

<font size="+3">I am test*<font size="+3">*ing a*</font>*<br>
</font>


Expected result

7 "testing" is correctly spelled hence should not get marked misspelled in any way (this bug 790475, currently titled "Spell checking is broken"!)

8 redundant font tags should not be added (Bug 812638; imho this is a major bug which should be fixed immediately to stop that tag pollution)

(In reply to Mike Conley (:mconley) from comment #91)
> (In reply to G. R. Main from comment #90)
> > NOT FIXED!
> > Problem persists in TB17.0
> If you're seeing the described symptoms after following the precise steps in
> comment 13, we'll consider re-opening this bug.
> 
> If not, what you're seeing is likely a separate bug that has a similar
> manifestation. Please see the comments above for the related bugs that have
> been filed, and please comment and follow those, as opposed to this closed
> bug. Thanks! -Mike

Mike, if you think that that this bug is fixed and whatever remains should be fixed in other bugs like bug 812638, then I recommend the following:

1) clarify in the bug summary and a comment what exactly this bug has fixed, and for which version of TB these fixes will land: The current summary ("Spell checking broken") is very unspecific and will naturally invite "me too" comments of all sorts, even after reading STR of comment 13, more so given that the exact *symptoms* of this bug as described in comment 13 are still present (as shown above in this comment)

2) add a whiteboard disclaimer pointing to an authoritative comment that explains what this bug is (not) about, to what extent further comments here are unwanted, and in which bugs the remaining problems are addressed.

Comment 95

[Mike Conley (:mconley) (:⚙️)]

(In reply to Thomas D. from comment #94)
> (In reply to Mike Conley (:mconley) from comment #91)
> > (In reply to G. R. Main from comment #90)
> > > NOT FIXED!
> > > Problem persists in TB17.0
> 
> +1
> 
> > If you're seeing the described symptoms after following the precise steps in
> > comment 13, we'll consider re-opening this bug.
> 
> (In reply to Mike Conley (:mconley) from comment #13)
> > I was able to reproduce this in Firefox 16.0.1 with the following STR:
> > 1) Go to http://www-archive.mozilla.org/editor/midasdemo/
> > 2) Change the font to Arial
> > 3) In the textarea, type "I am testung"
> > 4) Press space - you should see the red squiggly under "testung"
> 
> Mike, typing misspelled English words like "testung" into *Midasdemo* per
> your STR does *not* show any red squiggly underlining for me. Does it for
> you?
> 
> Otherwise, basically following your STR of comment 13 in Thunderbird
> reproduces this bug for me every time (both release & trunk), albeit only if
> using certain size values for deprecated font size attribute in <font
> size="x"></font> around the msg body (which is what we use when changing
> composition's default font size in preferences!).
> 
> STR
> 
> 1 compose new HTML msg
> 2 in msg body, type misspelled word "I am testung " (sic! testung followed
> by one space)
> 3 Ctrl+A, Insert > HTML
> 4 add font tags around msg body HTML, then [Insert], like this:
> 
> <font size="x">I am testung <br></font>
> where x must be between 1 and 6 (sic!), or between -3 and +3 (sic!);
> surprisingly, bug will *not* occur for
> - max values of font size attribute (7, +4)
> - invalid values of font size attribute (>7, or >+4, e.g. 8 or +5 etc.)
> - non-default font *face* attributes aka <font face="Calibri"></font>
> 
> 5 hit backspace 4 times to erase "ung "
> 6 type "ing x" to make it "I am testing x" where x is any alphanumeric
> character
> 7 observe spell check marks on "testing" (red squiggly)
> 8 verify HTML source (Ctrl+A, Insert > HTML)
> 
> Actual result
> 
> 7 "ing" part of "testing" is marked as misspelled (red squiggly) - this bug
> 8 redundant font tags are wrongly added in source (emphasis stars added by
> me), Bug 812638:
> 
> <font size="+3">I am test*<font size="+3">*ing a*</font>*<br>
> </font>
> 
> 
> Expected result
> 
> 7 "testing" is correctly spelled hence should not get marked misspelled in
> any way (this bug 790475, currently titled "Spell checking is broken"!)
> 
> 8 redundant font tags should not be added (Bug 812638; imho this is a major
> bug which should be fixed immediately to stop that tag pollution)
> 

I believe that Actual Result #7 is a direct result of Actual Result #8. I believe you're seeing symptoms of bug 812638.

> (In reply to Mike Conley (:mconley) from comment #91)
> > (In reply to G. R. Main from comment #90)
> > > NOT FIXED!
> > > Problem persists in TB17.0
> > If you're seeing the described symptoms after following the precise steps in
> > comment 13, we'll consider re-opening this bug.
> > 
> > If not, what you're seeing is likely a separate bug that has a similar
> > manifestation. Please see the comments above for the related bugs that have
> > been filed, and please comment and follow those, as opposed to this closed
> > bug. Thanks! -Mike
> 
> Mike, if you think that that this bug is fixed and whatever remains should
> be fixed in other bugs like bug 812638, then I recommend the following:
> 
> 1) clarify in the bug summary and a comment what exactly this bug has fixed,
> and for which version of TB these fixes will land: The current summary
> ("Spell checking broken") is very unspecific and will naturally invite "me
> too" comments of all sorts, even after reading STR of comment 13, more so
> given that the exact *symptoms* of this bug as described in comment 13 are
> still present (as shown above in this comment)
> 

Good idea.

> 2) add a whiteboard disclaimer pointing to an authoritative comment that
> explains what this bug is (not) about, to what extent further comments here
> are unwanted, and in which bugs the remaining problems are addressed.

Ok, I'm making this the authoritative comment.

Folks: this bug dealt with the inline spellcheck not correctly traversing sibling text nodes. The exact steps to reproduce are in Comment 13, and should be executed in a new profile. If you're seeing similar behaviours, it's almost certainly due to bug 812638.

Comment 96

[Bob]

Statuus should NOT be FIXED!

Still having this bug in 17.0.3, and it's really getting old!

Comment 97

[Bob]

Status should NOT be "resolved".

Still having this bug in 17.0.3, and it's really getting old!

Comment 98

[Bob]

Damn, why isn't there an edit/delete link?!

Comment 99

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

"Target Milestone" says mozilla19.  That means that the fix has landed in the central source code repository, and is present in Firefox/Thunderbird 19.  It isn't fixed in 17 or 18.  Hope that helps!

Comment 100

[Lee_Dailey]

howdy y'all,

i'm a tad ignorant of what this means ...
"status-firefox17: 	verified "

i THOT it meant that this was fixed in ff17. i also thot it would mean that it would be fixed in tbird17 since this is apparently core stuff, not firefox specific stuff.

i'm aware that the poster from comment 96 may well be seeing the results of related bug 812638. that one is listed as "Target Milestone: 	mozilla21" and marked as FIXED. that won't show up until tbird 17.0.5 or later, i suppose. that's another bug, tho.

so, is this specific core bug NOT fixed in tb17?

take care,
lee

Comment 101

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Lee_Dailey from comment #100)
> i'm a tad ignorant of what this means ...
> "status-firefox17: 	verified "

IIUC, it means that it has been verified that the bug exists in version 17, not that it's been fixed.  The first fixed version is given by the target milestone.  If the fix were backported, it would be "status-firefox17: fixed".  But I'm not sure what the difference is between "affected"/"verified" and "wontfix", and can't find any page that gives info on the status flags.

Hope that helps.

Comment 102

[Magnus Melin [:mkmelin]]

AFAIK
 - "affected" means it's known to exist in a certian version (but not yet fixed)
 - "verified" means QA has tested that a fix committed for that version ineeed works (status is changed from "fixed" to "verified").

Comment 103

[(no longer active)]

(In reply to comment #102)
> AFAIK
>  - "affected" means it's known to exist in a certian version (but not yet
> fixed)
>  - "verified" means QA has tested that a fix committed for that version ineeed
> works (status is changed from "fixed" to "verified").

Yeah I think that's the case.

Comment 104

[Lee_Dailey]

howdy y'all,

thanks for the confirmation. i had seen another bug comment that explained what was meant but can't find it. as i recall, it matches the info from Magnus Melin.

so this _specific_ bug otta be fixed in any gecko-17 based app. that would include both ff17 and tb17.

hopefully the poster from comment 96 will see this and look into bug 812638.

take care,
lee

Comment 105

[André Ziegler]

In Firefox 23 (beta) the spellcheck is broken again for me.