Spell checking regression in contentEditable elements being focused by script

RESOLVED FIXED in Firefox 24

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: adw)

Tracking

({regression, testcase})

Trunk
mozilla24
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24+ fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 762270 [details]
Testcase

See the test case.  Focusing the element used to trigger spell checking but it doesn't any more.

adw, do you mind taking this please?  Thanks!
(Reporter)

Comment 1

5 years ago
I found this out when I was trying to check in the test case patch to bug 674927.  We should probably check that in along the fix to this bug.
Blocks: 674927
status-firefox24: --- → affected
tracking-firefox24: --- → ?
(Reporter)

Updated

5 years ago
Attachment #762270 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 2

5 years ago
Interesting, if you modify the test case to focus the contenteditable after page load, spell check is properly triggered.
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
06/06 nightly good, 06/07 nightly bad:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-06-06&enddate=2013-06-07

Which includes my patches from bug 856270.
(Assignee)

Comment 4

5 years ago
Created attachment 762409 [details] [diff] [review]
queue UpdateCurrentDictionary while init is pending

Here's what used to happen before bug 856270:

1. Sometime early before page load (when the contenteditable is created?),
   mozInlineSpellChecker::SetEnableRealTimeSpell(aEnabled=true)
2. nsEditorSpellCheck::InitSpellChecker on mozInlineSpellChecker's mSpellCheck
3. nsEditorSpellCheck::UpdateCurrentDictionary, but this fails because:

> WARNING: NS_ENSURE_TRUE(rootContent) failed: file /Users/adw/mi/editor/composer/src/nsEditorSpellCheck.cpp

   (rootContent is the return value of htmlEditor->GetActiveEditingHost().
   It's null because GetActiveEditingHost looks at the focused node, which
   for some reason here is the body, which is not editable.)

4. At this point, mSpellCheck has no dictionary.  But then:
5. nsEditor::OnFocus when the contenteditable is focused
6. mozInlineSpellChecker::UpdateCurrentDictionary, which finally sets a
   dictionary
7. And then a spell check, which is successful because mSpellCheck has a
   dictionary.

After bug 856270:

1. Sometime early before page load (when the contenteditable is created?),
   mozInlineSpellChecker::SetEnableRealTimeSpell(aEnabled=true)
2. nsEditorSpellCheck::InitSpellChecker on mozInlineSpellChecker's
   mPendingSpellCheck, while mSpellCheck remains null
3. nsEditorSpellCheck::UpdateCurrentDictionary, but this fails for the same
   reason as above
4. At this point, mozInlineSpellChecker's mPendingSpellCheck has no
   dictionary, and mSpellCheck is null
5. nsEditor::OnFocus when the contenteditable is focused
6. mozInlineSpellChecker::UpdateCurrentDictionary, but this bails early
   because mSpellCheck is null because nsEditorSpellCheck::InitSpellChecker's
   callback hasn't been called yet
7. nsEditorSpellCheck::InitSpellChecker's callback is called
8. A spell check, but mSpellCheck has no dictionary, so there's effectively
   no spell check at all.

If you can follow that, the problem is step 6 in the latter series of steps.  There's no reason I can see that UpdateCurrentDictionary couldn't use mPendingSpellCheck if it's nonnull.  The dictionary update will just be queued after the pending initialization.

https://tbpl.mozilla.org/?tree=Try&rev=f4d25f200d23
Attachment #762409 - Flags: review?(ehsan)
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Updated

5 years ago
Attachment #762409 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/602b497adc01
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

5 years ago
status-firefox24: affected → fixed

Updated

5 years ago
tracking-firefox24: ? → +
You need to log in before you can comment on or make changes to this bug.