1837268 - Inline spell checker loses red underlines after a backspace is used - Take Three

Status: VERIFIED FIXED

(Core :: Spelling checker, defect, P2)

Description

[Magnus]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:109.0) Gecko/20100101 Firefox/114.0

Steps to reproduce:

Type any set of words with a misspelling in a message forum reply box and then hit a backspace and the red line will disappear. If you're sitting next to the misspelled word or even if it's just the last misspelled word, it will reappear if you space or otherwise move forward, but previous misspelled words you have in the same paragraphs/page will NOT reappear. They remain unmarked and therefore will be uncorrected when you're done typing. You would have to fix every spelling mistake as you type instead of at the end. The only way around this is to turn off the context menu spell check and then turn it back on again. THEN, all the misspelled words will get underlined again.

This doesn't happen in the text box here for reporting the bug or in Thunderbird emails, but ALL message forums I've tested have this problem so far. Google's search box doesn't get underlined at all (maybe a good thing there)

There has been two previous reports of the same bug that claim to have been fixed like 8 years ago (I've been using mostly Chrome for awhile until they stopped supporting the macOS I'm using so I haven't noticed), but I don't know if they just checked text boxes like this one with plain text or if they also tested message forum reply boxes. LINK: (https://bugzilla.mozilla.org/show_bug.cgi?id=1154791)

Actual results:

Underlined red misspelled words disappear in message forum text boxes after a backspace is pressed (and sometimes with a period for a new sentence as well). It only reappears for the most recent misspelled word to the left. Previous misspelled words remain with no red underline.

Expected results:

The underline red line should only disappear when editing that one word with a backspace. Previous words should be unaffected.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Editor' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Eemeli Aro [:eemeli]]

We're now seeing this in the contenteditable editor that we're introducing for Pontoon, Mozilla's own localization platform.

We're using CodeMirror 6 for the editor, which has its own keyboard handling. I'm attaching a screen recording of the behaviour with it. Andrew, would this be sufficient to update the status of this bug?

This problem does not occur on a bare HTML element that uses contenteditable and spellcheck, so it may be related to how DOM modifications are handled.

The bug is currently replicable on the Pontoon staging server: https://mozilla-pontoon-staging.herokuapp.com/

Comment 3

[Eemeli Aro [:eemeli]]

Attachment: Firefox Nightly 117.0a1 Pontoon spellcheck.mov

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Eemeli Aro [:eemeli] from comment #2)

We're using CodeMirror 6 for the editor, which has its own keyboard handling. I'm attaching a screen recording of the behaviour with it. Andrew, would this be sufficient to update the status of this bug?

This can be a P2 but the spell checker isn't particularly strongly owned or resourced. I guess one question is whether this is likely to actually be something more like an interaction with the editor component? :smaug is out on PTO so needinfo-ing the editor component owner, :masayuki.

Comment 5

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

I'm not familiar with specllchecker's implementation, but this is probably a long standing bug of basic implementation.

Currently, spellchecker works in a special path when deletion:

• https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/extensions/spellcheck/src/mozInlineSpellChecker.cpp#136,150-153,163-164
• https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/extensions/spellcheck/src/mozInlineSpellChecker.cpp#109-110,112
• https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/extensions/spellcheck/src/mozInlineSpellChecker.cpp#342-351

And the "anchor" position is considered with Selection at typing a delete key:

• https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/editor/libeditor/EditorBase.cpp#5351,5361-5362
• https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/editor/libeditor/HTMLEditSubActionHandler.cpp#722
• https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/editor/libeditor/HTMLEditSubActionHandler.cpp#256-257

And our spellchecker may not work with DOM mutation caused by web apps (I guess that if it's modified by input event handler, this does not work):

• https://searchfox.org/mozilla-central/rev/00e6644d0db8acf9372702324151b8077a3d2bb7/editor/libeditor/HTMLEditor.cpp#4459-4460,4474-4501,4533-4547

So, somebody needs to investigate a lot about this kind of issue...

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Thanks so much for the detailed investigation and specific pointers as well as scoping context!

:eemeli, if this is meaningfully impacting the Pontoon effort it may be worth discussing with product in order to try and get an OKR on improving the spell checker from that direction (as opposed to only having a web-platform tech debt OKR which I've just started the ball rolling on).

Comment 7

[Eemeli Aro [:eemeli]]

This is meaningfully impacting our ability to introduce an editor with syntax highlighting to Pontoon, where spellchecking provides significant value.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=923376 and https://bugzilla.mozilla.org/show_bug.cgi?id=743819 changed some of the relevant code.

Comment 9

[Eemeli Aro [:eemeli]]

Attachment: bug-1837268.html

I've attached a minimal reproduction of at least a part of the problem. It's not quite what I see with CodeMirror 6, but it would appear that setting the nodeValue of a TextNode in a contenteditable blocks spellcheck updates until the element is re-focused.

CM6 uses this approach at least for some of its updates: https://github.com/codemirror/view/blob/fae53172152842a451a0db9e930125baf51b5523/src/inlineview.ts#L29

Comment 10

[Eemeli Aro [:eemeli]]

Attachment: bug-1837268-codemirror.html

This is a minimal demo of the bugginess experienced with CodeMirror 6, which relies on the externally hosted file https://codemirror.net/codemirror.js.

To see the bug, open the page and:

1. focus cursor at end of contenteditable
2. press backspace
3. press space
4. observe lack of error on aaa.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1837268, update inline spellchecker when text data is modified, r=masayuki

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Eemeli, want to try out the patch locally? (or use the builds from tryserver, see the links in phabricator)

Comment 13

[Eemeli Aro [:eemeli]]

That's beautiful! Built it locally, and can verify that it fixes both the minimal and CodeMirror test included above, and works as expected on Pontoon's staging server, where the new editor is currently deployed.

The visible flashing on backspace is a different bug, I'm pretty sure that also happens when e.g. an element's textContent is set.

Any chance of still getting this into 117?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

The current patch seems to break some tests, I'll need to investigate those. So 117 is unlikely.
(performance numbers look ok)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01837134e47e
update inline spellchecker when text data is modified, r=masayuki

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/01837134e47e

Comment 17

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-08-01 Nightly build on Windows 10.
Verified as fixed on Firefox 118.0b2(20230829180158) and Nightly 119.0a1(20230830212731) on Windows 10, macOS 12, Ubuntu 22.

Comment 18

[Sören Hentzschel]

Can this be considered for an ESR uplift, or do you think it is too risky? This affects the CKEditor, one of the most popular WYSIWYG editors. I can verify that the spell checker works with CKEditor since this patch has landed in Firefox 118.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

This has been now on release for some time and at least I'm not aware of regressions and this shouldn't be too risky, so yes, we could try ESR too

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I need to check if the patch applies cleanly to ESR

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1837268, update inline spellchecker when text data is modified, r=masayuki

Using the test for bug 1602526 as the basis for the new test but tweaking it by
adding the event listener used in the first testcase of the bug.

Original Revision: https://phabricator.services.mozilla.com/D184987

Comment 22

[Phabricator Automation]

Uplift Approval Request

• Explanation of risk level: see above
• Steps to reproduce for manual QE testing: https://bugzilla.mozilla.org/show_bug.cgi?id=1837268#c10
• Is Android affected?: no
• Code covered by automated testing: yes
• Risk associated with taking this patch: Should be relatively small, it makes us spellcheck more in certain cases
• String changes made/needed: NA
• Needs manual QE test: yes
• User impact if declined: spellchecking doesn't work with CKEditor
• Fix verified in Nightly: yes

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9366496 [details]
Bug 1837268, update inline spellchecker when text data is modified, r=masayuki

Approved for 115.6esr.

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/552e49e9a6a5

Comment 25

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Verified as fixed on Firefox ESR 115.6.0 on Windows 10, Ubuntu 22, macOS 13.

Comment 26

[Magnus]

There's still something badly amiss here with Firefox ESR 115.6 on my Mac running macOS Mojave. The "fix," which I was thrilled to get since I still use Mojave a lot in order to run some very high-end 32-bit Mac software that won't work in newer versions of macOS, seems to work at first, but after a lot of editing on a forum (in this case the AVS Forums for home theater), the spell checking just flat out "stops". It sees and corrects absolutely nothing at that point.

I don't know where the exact point is in editing large multi-paragraph replies (i.e. It's still working typing this here in one paragraph), but it's definitely losing track somewhere and then it doesn't work in that text box at all anymore. Maybe it's due to lots of cursor moving around, but I can't get it to replicate here while typing this for whatever reason. It's possible it's the "preview" function or something on the AVS forums somehow changing something despite it being shown separately from the text box you type in. I just know I have a reply in progress there right now that's no longer showing spelling errors and despite it working here in this tab, it's not working in that one at all.

In fact, I just now posted that message and the new 'add reply' text box shows no spelling corrections either. It just seems broken on that page now. Reloading the same web page with the "refresh" button gets it working again. I'm not sure offhand if it does this in the newest full Firefox in Windows 10. I didn't notice it offhand when I was using it, though.

Also, potentially unrelated, once in awhile, the whole paragraph will light up like a giant spelling mistake for reasons unknown and the right context click shows nothing. The ONLY way to get it to not highlight the entire paragraph at that point is to hit a carriage return (enter key) and then backspace or otherwise move back to the paragraph. That removes the entire red line and it checks it normally again. That also did that before the fix sometimes too and behaved the same way so it may be unrelated in that regard to the "fixed" behavior. I've seen this behavior in Windows 10 Firefox using the latest Firefox (same Mac booted into Windows 10) and thus I suspect it's something else entirely causing it.