Closed Bug 697981 Opened 13 years ago Closed 9 years ago

Textarea spell checked in the wrong language when not in focus (was: spelling dictionary is reloaded every time the tab is focused)

Categories

(Core :: Spelling checker, defect)

Product:
Core
Component:
Spelling checker
Version:
9 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox9 - ---
firefox10 - ---
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: amir.aharoni, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 16 obsolete files)

text area switching test (needs English and French or Spanish dictionary), now also with <div contenteditable>
(deleted), text/html
Details
Test (v2c) - now using "onfocus".
(deleted), patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Proposed solution: (v9), notifying dictionary removal, changed listener logic.
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111027 Firefox/9.0a2 Build ID: 20111027042020 Steps to reproduce: 1. Install the Hebrew spelling dictionary: https://addons.mozilla.org/he/firefox/addon/hebrew-spell-checking-dictiona/ 2. Install the Catalan spelling dictionary: https://addons.mozilla.org/he/firefox/addon/general-catalan-dictionary/ 3. Open Catalan Wikipedia testing editing page in one tab: https://ca.wikipedia.org/w/index.php?title=User:Amire80/lang_test&action=edit&uselang=en 4. Open Hebrew Wikipedia testing editing page in another tab: https://he.wikipedia.org/w/index.php?title=User:Amire80/lang_test&action=edit&uselang=en Actual results: Spelling dictionary is selected automatically according to the lang attribute of the <textarea>. (In Wikipedia the lang attribute is applied to the <html> element.) This is a very good new feature in Aurora. However, every time i switch between tabs, all the text appears in red, even if all the words are corrected and it appears that the dictionary is reloaded. Firefox probably loads only one spelling dictionary and applies it to all tabs. Expected results: 1. The dictionary file should not be reloaded for every tab. It is especially noticeable with the Hebrew dictionary, which resides in a pretty large file. Turkish, Greek and other languages are even larger. 2. The words should not be marked as incorrect if they were already checked and found to be correct.
Component: General → Spelling checker
Product: Firefox → Core
QA Contact: general → spelling-checker
This seems bad....
Status: UNCONFIRMED → NEW
tracking-firefox10: --- → ?
tracking-firefox9: --- → ?
Ever confirmed: true
Keywords: regression
Looks like a regression from bug 338427?
Blocks: 338427
This is more like an unwanted side effect of bug 338427. In general, it's very nice that bug 338427 was addressed.
So, one (well, 2 to be precise) spell check operations are initiated from nsEditor::Observe (which is supposed to handle the case where a new dictionary is added/removed, see bug 591780). This should be solvable by temporarily turning spellchecking off. Another one is triggered from mozInlineSpellChecker::UpdateCurrentDictionary because previousDictionary != currentDictionary. This one is harder to solve, because we currently don't have the notion of current editor's language, all we know about is the current hunspell language... Jesper/Arno, do any of you guys have enough cycles to work on this?
Blocks: 591780
I won't have time to work on this right now, and I don't know if I will have time at some point, as I am no longer a student. Generally I think the problem is that parts of the design of the spell checker assumes one global current dictionary, chosen via mozISpellCheckingEngine::dictionary. This has never been true, and is even less true with bug 338427 and maybe also bug 591780. A better design, I think, would be to have each dictionary represented as its own XPCOM object, which the spell checker in each editor could refer to. This is the current interface: interface mozISpellCheckingEngine : nsISupports { attribute wstring dictionary; readonly attribute wstring language; readonly attribute boolean providesPersonalDictionary; readonly attribute boolean providesWordUtils; readonly attribute wstring name; readonly attribute wstring copyright; attribute mozIPersonalDictionary personalDictionary; void getDictionaryList([array, size_is(count)] out wstring dictionaries, out PRUint32 count); boolean check(in wstring word); void suggest(in wstring word,[array, size_is(count)] out wstring suggestions, out PRUint32 count); void loadDictionariesFromDir(in nsIFile dir); void addDirectory(in nsIFile dir); void removeDirectory(in nsIFile dir); }; I think it should be split up into a dictionary and an engine: interface mozISpellCheckingDictionary : nsISupports { readonly attribute wstring dictionary; readonly attribute wstring language; attribute mozIPersonalDictionary personalDictionary; boolean check(in wstring word); void suggest(in wstring word,[array, size_is(count)] out wstring suggestions, out PRUint32 count); }; interface mozISpellCheckingEngine : nsISupports { void getDictionaryList([array, size_is(count)] out mozISpellCheckingDictionary dictionaries, out PRUint32 count); void loadDictionariesFromDir(in nsIFile dir); void addDirectory(in nsIFile dir); void removeDirectory(in nsIFile dir); }; (unused and unimplemented methods removed) Now the engine does not have the concept of a current dictionary. Only each editor would have that, by holding a reference to a specific mozISpellCheckingDictionary. This way it would be possible for different editors to refer to different dictionaries directly, and if two open editors use different dictionaries, they could both be loaded into memory at the same time. The dictionaries could be loaded lazily the first time they are needed, and one could add some more advanced logic around when it would be a good time to unload them to free up memory.
(In reply to Ehsan Akhgari [:ehsan] from comment #4) > > Jesper/Arno, do any of you guys have enough cycles to work on this? I've currently no time to work on this.
[Triage Comment] Does this bug imply that 2 languages works correctly but >2 languages do not? Regardless, a partial re-design of the feature is outside the scope of FF9 Aurora/Beta. Please nominate for approval on Aurora/Beta if a lower risk fix can be found.
tracking-firefox10: ? → -
tracking-firefox9: ? → -
Keywords: #relman/triage/needs-info
(In reply to Alex Keybl [:akeybl] from comment #7) > [Triage Comment] > Does this bug imply that 2 languages works correctly but >2 languages do not? no. 1 language works, >1 language does not. > Regardless, a partial re-design of the feature is outside the scope of FF9 > Aurora/Beta. Please nominate for approval on Aurora/Beta if a lower risk fix > can be found. I am not sure how broad this bug is. It seems to talk about both showing wrong spell check markers and about loading dictionaries. The attached test page shows both problems (if both the English and the Danish dictionary is installed): 1: When one of the two editors is focused, the text of the other editor shows as misspelled 2: When focus switches between the two editors, the rectangle temporarily stops spinning, indicating a responsiveness issue. Problem 1 can most likely be fixed without any redesign, but it might make the already very fragile spell checker code even more fragile. I think it would not be safe to take such a patch on aurora/beta outside the normal cycle. Problem 2 is much harder to address without some change to the design.
(In reply to Jesper Kristensen from comment #8) > 1: When one of the two editors is focused, the text of the other editor > shows as misspelled > 2: When focus switches between the two editors, the rectangle temporarily > stops spinning, indicating a responsiveness issue. Thanks for the explanation, Jesper. Although both of these issues are unfortunate, neither seem to warrant backing out bug 338427. Additionally, it appears as if a fix would be outside of the scope of FF9/10, so we won't be tracking for these upcoming releases.
tracking-firefox10: - → +
tracking-firefox9: - → +
Keywords: #relman/triage/needs-info
Incorrect flags - fixing.
tracking-firefox10: + → -
tracking-firefox9: + → -
Jesper, do you think you're going to have cycles to work on this?
I would like to, but I am not a student anymore, and right now I am moving, so I don't think I would have something done in the near future. What do you think the best approach here is? Something minor or something like my comment 5?
I think for the purposes of this bug, we should focus on comment 4. The first paragraph should be easy, the second one I don't know how to solve yet, as I did not delve into it too much. A redesign of the internals of the spell checker is long overdue, but I don't want to do that in this bug, as it's too large of a project. Also, we should keep things like multi-dictionary spell checking, global and per language personal dictionaries, etc. in mind. Josh, do you happen to know anybody who would be willing to delve into this? It's not exactly suitable as a good first bug, but I'd be more than happy to help whoever steps in. :-)
Josh, this does look quite interesting. I will read up on the relevant files and catch on on irc if unclear about anything.
Assignee: nobody → andrew.quartey
Josh, this does look quite interesting. I will look up the relevant files and catch up on on irc if unclear about anything.
Thanks Andrew for jumping in to help. Please let me know if you need help. As a starting point, I would work on my idea in the first paragraph of comment 4. We can tackle the second part of that comment later. Thanks again!
Attached patch Patch v1 (obsolete) (deleted) — — Splinter Review
Restricted reloading of spelling dictionary to focused editors only depending on whether mIsFocused is set or not.
Attachment #593607 - Flags: review?(ehsan)
Comment on attachment 593607 [details] [diff] [review] Patch v1 Review of attachment 593607 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure if I understand the goal of this patch, and the logic it uses to solve the problem. So, mIsFocused actually doesn't capture whether the editor is focused or not. We just toggle it before the SetFlags and OnFocus calls and restore the original value afterwards. How would this actually fix the problem at hand?
(In reply to Ehsan Akhgari [:ehsan] from comment #18) > Comment on attachment 593607 [details] [diff] [review] > Patch v1 > > How would this actually fix the problem at hand? On the look of it, it's not much but when applied it does pass the switching test and addresses Amir's issue in comment 0. In the problem, after an editor's spellchecker has been initialized and focus is switched to another editor to initialize its own checker, the original editor's is notified of the dictionary update and thus rechecks what's been checked already. To prevent that, mIsFocused is introduced in nsEditor::Observe to force an editor to ignore the dictionary update. Thus this being toggled before and after SetFlags as that eventually calls other functions to initialize spell checking. The toggling of mIsFocused before and after OnFocus serves the same purpose as described. OnFocus calls trigger a dictionary update.
Comment on attachment 593607 [details] [diff] [review] Patch v1 As discussed on IRC, I suggested to Andrew to rework his patch to use a RAII object which would add the editor to the observer list in nsEditorEventListener::Focus and remove it when it returns.
Attachment #593607 - Flags: review?(ehsan)
Attached patch Patch v2 (obsolete) (deleted) — — Splinter Review
Changes per comment 20 and try results: https://tbpl.mozilla.org/?tree=Try&rev=3b0d329f44ab
Attachment #593607 - Attachment is obsolete: true
Attachment #595041 - Flags: review?(ehsan)
Comment on attachment 595041 [details] [diff] [review] Patch v2 Review of attachment 595041 [details] [diff] [review]: ----------------------------------------------------------------- So, with this patch, if a dictionary is installed when a textarea is not focused, the changes will never be picked up by the textarea, right? That is a problem... ::: editor/libeditor/base/nsEditor.cpp @@ +5412,5 @@ > + if (obs) { > + obs->RemoveObserver(this, > + SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION); > + } > +} Nit: please use spaces instead of tabs, and fix the indentation in these two functions. ::: editor/libeditor/base/nsEditor.h @@ +219,5 @@ > void BeginKeypressHandling(nsIDOMNSEvent* aEvent); > void EndKeypressHandling() { mLastKeypressEventWasTrusted = eTriUnset; } > + > + void AddToObserverService(); > + void RemoveFromObserverService(); I think we should use a better name. Maybe StartWatchingDictionaryChanges and StopWatchingDictionaryChanges? @@ +242,5 @@ > }; > > + class FireEditorFocusEvent { > + public: > + FireEditorFocusEvent(nsEditor* aEditor) Please make this constructor explicit. ::: editor/libeditor/base/nsEditorEventListener.cpp @@ +926,5 @@ > { > NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE); > NS_ENSURE_ARG(aEvent); > > + nsEditor::FireEditorFocusEvent focusedEvent (mEditor); Nit: no space before the parenthesis.
Attached patch Patch v3 (obsolete) (deleted) — — Splinter Review
(In reply to Ehsan Akhgari [:ehsan] from comment #22) > So, with this patch, if a dictionary is installed when a textarea is not > focused, the changes will never be picked up by the textarea, right? That > is a problem... > If installed dictionaries didn't require a require restart to become 'active' then sure, we would have a problem here.
Attachment #595041 - Attachment is obsolete: true
Attachment #595041 - Flags: review?(ehsan)
Attachment #597057 - Flags: review?(ehsan)
(In reply to Andrew Quartey [:drexler] from comment #23) > If installed dictionaries didn't require a require restart to become > 'active' then sure, we would have a problem here. They don't require a restart (bug 591780). Likewise, what happens when the active dictionary is uninstalled?
It looks like your patch is only preventing the re-check on focus change. But the dictionary would have to be re-loaded anyway or else you get spell checking in the wrong language when you start typing into the editor. Is it the re-check which is causing all the UI freeze, and the re-load is fast and can be done on every editor focus switch?
Jesper, thanks i wasn't aware of this. Besides, when a restartless dictionary is installed/unistalled, is a SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION sent by default or only when the editor requiring it is focused does it become aware of the available/unavailable dictionary? I think i have to qualify my statement in comment 23. We might have a problem depending on what behavior we want. If unfocused editors are to be aware of newly (un)installed restartless dictionaries they specifically require for spell checking, then yes. In that case, a restriction based on the type of notification could be a better approach, that is, if we are still are keen on keeping a global dictionary. Ehsan, is that what is required?
(In reply to Andrew Quartey [:drexler] from comment #26) > Jesper, thanks i wasn't aware of this. Besides, when a restartless > dictionary is installed/unistalled, is a > SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION sent by default or only when the > editor requiring it is focused does it become aware of the > available/unavailable dictionary? The notification is actually sent when the active dictionary changes. It is not sent when a restartless dictionary is installed, and when a restartless dictionary is uninstalled, it is only sent if that dictionary was the active dictionary.
(In reply to Jesper Kristensen from comment #27) > The notification is actually sent when the active dictionary changes. It is > not sent when a restartless dictionary is installed, and when a restartless > dictionary is uninstalled, it is only sent if that dictionary was the active > dictionary. In that case, then this patch should not have a problem with the restartless dictionaries then.
(In reply to Andrew Quartey [:drexler] from comment #28) > (In reply to Jesper Kristensen from comment #27) > > > The notification is actually sent when the active dictionary changes. It is > > not sent when a restartless dictionary is installed, and when a restartless > > dictionary is uninstalled, it is only sent if that dictionary was the active > > dictionary. > > In that case, then this patch should not have a problem with the restartless > dictionaries then. It will however be a problem if the selected dictionary is uninstalled. Maybe we can fix that by sending a different notification in that case, and listen for that notification all the time, like we do right now?
Comment on attachment 597057 [details] [diff] [review] Patch v3 Clearing the review request for now...
Attachment #597057 - Flags: review?(ehsan)
(In reply to Ehsan Akhgari [:ehsan] from comment #29) > It will however be a problem if the selected dictionary is uninstalled. > Maybe we can fix that by sending a different notification in that case, and > listen for that notification all the time, like we do right now? That sounds good.
Blocks: spell-lang-change
Depends on: 959785
This bug has been dormant for a few years. Andrew, would you like to unassign yourself, so someone else can take over?
Flags: needinfo?(andrew.quartey)
Attachment #577664 - Attachment description: text area switching test → text area switching test (needs English and Danish spell checker)
Attachment #577664 - Attachment description: text area switching test (needs English and Danish spell checker) → text area switching test (needs English and Danish dictionary)
Attached patch Patch (v4) (refreshed) (obsolete) (deleted) — — Splinter Review
I took the liberty to refresh the patch and test it. It works great. Ehsan, can you please explain to me, what the problem is. Quoting from comment #31 of March 2012, surely you can remember ;-): > It will however be a problem if the selected dictionary is uninstalled. I uninstalled one of the dictionaries used in my test, then visited the textarea that used that dictionary. Since the dictionary was no longer there, the language defaulted to another dictionary and all the text came out with red underlines. So as far as I can see, this works well. Any ideas on how to write a test for this, if required. P.S.: I'm currently working through meta-bug 1073827 trying to clear out as many dependent bugs as possible. I noticed this bug and since it had always absolutely puzzled me why the spell checker spell checked text areas in the wrong language when not in focus, I've taken over. P.S.2: Perhaps we can get Olli interested in this as well.
Attachment #597057 - Attachment is obsolete: true
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)
Attachment #8659487 - Flags: feedback?(ehsan)
Assignee: andrew.quartey → mozilla
Flags: needinfo?(andrew.quartey)
Summary: spelling dictionary is reloaded every time the tab is focused → Textarea spell checked in the wrong language when not in focus (was: spelling dictionary is reloaded every time the tab is focused)
OS: Windows XP → All
Hardware: x86 → All
(In reply to Jorg K (GMT+2) from comment #34) > Ehsan, can you please explain to me, what the problem is. Quoting from > comment #31 of March 2012, surely you can remember ;-): > > It will however be a problem if the selected dictionary is uninstalled. Seems pretty obvious: What should happen if the selected dictionary in a focused textarea gets uninstalled. > I uninstalled one of the dictionaries used in my test, then visited the > textarea that used that dictionary. Since the dictionary was no longer > there, the language defaulted to another dictionary and all the text came > out with red underlines. So as far as I can see, this works well. Then you didn't test the above scenario! > Any ideas on how to write a test for this, if required. Should be pretty straightforward, basically have a page with the textarea in the desired language, focus it, wait for spell checking to happen, and then uninstall the dictionary programmatically and see what happens. I'll clear the feedback request since I really mean what my bugzilla name says about reviews. :-) Please ask Olli or someone else. Thanks!
Flags: needinfo?(ehsan)
Attachment #8659487 - Flags: feedback?(ehsan)
This is the test I did with English and Spanish dictionaries installed. Open attachment 8659072 [details]. Click in the English textarea. Click in the Spanish text area. No spell check errors show, unlike before. In another window, Tools > Add-ons, Dictionaries. Disable the Spanish dictionary while observing the text areas. Nothing happens, as expected. Now click on the Spanish text area. On focus, a new (default) dictionary is used and all the words are underlined. Is this not acceptable? We need to get the underlines as soon as the dictionary is uninstalled?
Flags: needinfo?(ehsan)
(In reply to Jorg K (GMT+2) from comment #36) > We need to > get the underlines as soon as the dictionary is uninstalled? Yes.
Flags: needinfo?(ehsan)
I don't really follow the reasoning on this. The current state is this: 1) When you first load a page, the textareas are not spell checked. They are spell checked when they receive the focus. 2) If there are two textareas with different dictionaries, on the same page, or in different windows, they are always all spell checked with the language of the textarea that has the focus. This is absolutely terrible and confusing since the users as themselves whether the other non-focused text areas have lost their language setting, which is not the case!! 3) When a dictionary gets uninstalled, which is a rare case, the language of the textarea losing its dictionary gets updated with misspellings immediately. Proposed new state with this simple fix is: 1) Unchanged. 2) Only the text area in focus gets spell checked (again) with its currently set dictionary. This avoids other textareas being spell checked in the wrong language. 3) When a dictionary gets uninstalled, the textarea losing its dictionary gets updated with misspellings when it next receives the focus. Frankly, the is a *HUGE* gain to be had from this solution, with perhaps a little imperfection in the rare case where a dictionary is removed. Let's see whether this breaks anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcedc79d0e6e
Problems caused by this patch are in the area of adding and removing dictionaries. Not a surprise. I will have to analyse: 2265 INFO TEST-UNEXPECTED-FAIL | extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul | map misspellings - got "Frühstückqwertyu", expected "createdimplytomorrowqwertyu" 3094 INFO TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_textbox_dictionary.xul | Is Add to Dictionary visible? 3095 INFO TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_textbox_dictionary.xul | Is separator visible?
OK, the test failure in test_add_remove_dictionaries.xul is understandable. No idea what causes the failure in test_textbox_dictionary.xul. The test runs fine on my local machine: 3 INFO TEST-PASS | toolkit/content/tests/chrome/test_textbox_dictionary.xul | Is Add to Dictionary visible? 4 INFO TEST-PASS | toolkit/content/tests/chrome/test_textbox_dictionary.xul | Is separator visible? 5 INFO TEST-PASS | toolkit/content/tests/chrome/test_textbox_dictionary.xul | Is Undo Add to Dictioanry visible? 6 INFO TEST-PASS | toolkit/content/tests/chrome/test_textbox_dictionary.xul | Is separator hidden? How do I fix a test that doesn't fail for me? I'll refresh my local build and try again.
OK. I'll implement what Ehsan said in comment #31: Send another notification here: http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/src/mozHunspell.cpp#606 and always listen to it. That's a couple of lines of code more to keep everyone happy ;-) Saves me messing around with the tests, too.
Added another event to trigger when a dictionary is removed. This works well with textareas, also when the dictionary is removed, but doesn't work for <div contenteditable>. So more work is required there. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca0341b3bd50
Attachment #8659487 - Attachment is obsolete: true
Test case with <div contenteditable>.
Attachment #577664 - Attachment is obsolete: true
Attachment #8659072 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Only one test failure with the latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca0341b3bd50 2269 INFO TEST-UNEXPECTED-FAIL | extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul | map misspellings - got "Frühstückqwertyu", expected "createdimplytomorrowqwertyu" We're getting there ;-)
Another version. This now works for textareas and it works when dictionaries are removed. It also passes the test that failed before (test_add_remove_dictionaries.xul). Sadly the original author's trickery in nsEditorEventListener::Focus, that is creating an object whose constructor started the event listening and whose destructor stopped the event listening didn't fly, since the listening only happened during the call to nsEditorEventListener::Focus. We need to listen all the time while the field has focus. So it's back to the old-fashioned way: Start listening at focus time, stop listening at blur time. Works and the test passes. Next step is to investigate why <div contenteditable> is different. There the spell check refresh happens all the time.
Attachment #8659795 - Attachment is obsolete: true
Fixed the description: This test case assumes that an English and a Spanish or French spell checker is installed.
Attachment #8659796 - Attachment is obsolete: true
I did some more investigation. On a page with some textareas and a <div contenteditable> the div *always* receives the focus event, regardless of whether it is clicked or a textarea is clicked. Grrr. So this approach doesn't work. Neither would the original approach with the RAII object (comment #20) ever have worked.
This is the proposed solution: - Focus and Blur events start and stop listening to SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION. There is protection against unbalances focus/blur calls. - We listen to SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION. This works well for text areas. I was under the impression that multiple <div contenteditable> on the page have multiple editors, but they don't. That's why I can't suppress the spell check update on the other two, when one is clicked. Still, if the different <div contenteditable> are in different tabs or windows, this is still an improvement. Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e24d97589b28 I will write a test next.
Attachment #8660115 - Attachment is obsolete: true
Not removing the observer caused leaks and test failures. Now it's green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c49e9aef8c4 test_add_remove_dictionaries.xul, the test that removes a dictionary that is in use, is green as part as "oth" (other). Notes to the reviewer: Ehsans original idea of using a RAII object (comment #20) to start and stop listening to SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION in nsEditorEventListener::Focus (see attachment 8659795 [details] [diff] [review]) didn't work, because in test_add_remove_dictionaries.xul the dictionary is changed while the textarea maintains focus (and therefore nsEditorEventListener::Focus has long returned and the destructor of the RAII object has stopped the listening). So therefore the implementation of keeping the listening active between focus and blur. Note also that the three <div contenteditable> in the attached example (attachment 8660290 [details]) have only one editor, so the undesired updating of the text we didn't click on seems unavoidable. As I said before, the situation for <div contenteditable> is improved, if they reside in different tabs/windows.
Attachment #8660312 - Attachment is obsolete: true
Attachment #8660355 - Flags: feedback?(roc)
Attached patch Test (v1) (obsolete) (deleted) — — Splinter Review
Here comes the test. Pretty straight forward. Note that when I created the German dictionary for the test for bug 1200533 (the big spell check clean-up), I was too lazy and copied a dictionary with English words. So now I had to supply some decent German words. I guess the test is cool, that's why I am asking for review here. If you think that the code is OK, you can give an r+ instead of an f+ there, too ;-)
Attachment #8660445 - Flags: review?(roc)
Comment on attachment 8660445 [details] [diff] [review] Test (v1) I'd better change the r? to an f? since the damn thing doesn't work, at least not on Linux. Works fine on my Windows machine, but not on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e92db069d2c7 Puzzling. We get: 2197 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html | expected de-DE - got "en-US", expected "de-DE" 2198 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html | one misspelled word expected: German - got "heuteisteinguter", expected "German" 2201 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html | one misspelled word expected: German - got "heuteisteinguter", expected "German" So the first textarea never receives the de-DE dictionary. Hmm.
Attachment #8660445 - Flags: review?(roc) → feedback?(roc)
Attached patch Test (v2) - now using "onfocus". (obsolete) (deleted) — — Splinter Review
Here is another version of the test. This time it uses "onfocus". Works on Windows. Let's see whether we have more luck this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=082bfa403fbd
While I'm waiting for the try results, here another comment. In Thunderbird each message being composed has its own window/document and each message can have a different language (derived from the document's "lang" attribute. The message body is a big "contenteditable". Without the patch, all concurrently open message compositions are always spelled with the same dictionary, which is really confusing, since red underlines appear and disappear as we switch windows. With the patch, peace and quiet enters the scene. Each window is spelled in its own language, and when we switch to a different window with another language, the one that just lost focus doesn't get updated. So this fix is really for the benefit of many ;-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=082bfa403fbd Still the same failures: 2197 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html | expected de-DE - got "en-US", expected "de-DE" 2198 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html | one misspelled word expected: German - got "heuteisteinguter", expected "German" 2201 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html | one misspelled word expected: German - got "heuteisteinguter", expected "German" Why does that fail on Linux but not on Windows?
I don't know. You should be able to install Linux on a VM (VirtualBox and VMWare Player are free) and run the tests.
Comment on attachment 8660355 [details] [diff] [review] Proposed solution: (v8), notifying dictionary removal, changed listener logic. Review of attachment 8660355 [details] [diff] [review]: ----------------------------------------------------------------- It would be nice to have an idea plan for how spellcheck should work, and then make progress towards that plan. I'm a bit concerned that this patch, though improving the situation, takes the code in a direction that is not ideal. ::: editor/libeditor/nsEditor.cpp @@ +307,5 @@ > > nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > if (obs) { > obs->AddObserver(this, > + SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION, Why a REMOVE notification here? This doesn't look right. @@ +5211,5 @@ > +nsEditor::StartWatchingDictionaryChanges() > +{ > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + obs->AddObserver(this, SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION, false); You're adding a strong reference here. How can we be sure this won't keep this nsEditor alive forever?
Attachment #8660355 - Flags: feedback?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57) > Comment on attachment 8660355 [details] [diff] [review] > Proposed solution: (v8), notifying dictionary removal, changed listener > logic. > > Review of attachment 8660355 [details] [diff] [review]: > ----------------------------------------------------------------- > > It would be nice to have an idea plan for how spellcheck should work, and > then make progress towards that plan. I agree. Bug 1204147 shows, that there is some weird stuff going on. Notifications fire on things that don't seem to be initialised, and the whole thing is fiendish complicated. > I'm a bit concerned that this patch, > though improving the situation, takes the code in a direction that is not > ideal. > > ::: editor/libeditor/nsEditor.cpp > @@ +307,5 @@ > > > > nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > > if (obs) { > > obs->AddObserver(this, > > + SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION, > > Why a REMOVE notification here? This doesn't look right. This stuff managed to confuse you ;-) Without this patch, UPDATE notifications are added in nsEditor::PostCreate(): https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#309 and removed in nsEditor::PreDestroy() https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#452 Now I've change the game a bit. The REMOVE notification is always listened to, the UPDATE notification is only listened to in the focus-blur timeframe. IMHO this change is right: - SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION, + SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION, It says: Let's not register the UPDATE listener at editor creation time, let's always listen to REMOVE instead. > @@ +5211,5 @@ > > +nsEditor::StartWatchingDictionaryChanges() > > +{ > > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > > + if (obs) { > > + obs->AddObserver(this, SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION, false); > > You're adding a strong reference here. How can we be sure this won't keep > this nsEditor alive forever? First of all, I copied this stuff. Secondly, I don't really understand the difference between strong and weak. Thirdly however, we have the mIsFocused member variable, that prevents more than one UPDATE observer being added to the editor upon focus. Then in nsEditor::PreDestroy() the UPDATE and the REMOVE observer get removed. I repeat: The REMOVE observer takes the place of the previous UPDATE observer. If is added and removed as the UPDATE observer was added and remove before. The UPDATE observer only gets added once in focus, and removed in blur, or if blur never arrives, it gets removed in nsEditor::PreDestroy() (together with REMOVE). I had the case where the remove of UPDATE was missing in nsEditor::PreDestroy(), and that led to many leaks. I don't see a problem with this code, other than perhaps that it is adding another, well, how to say it, "unfortunate addition", to something what is already fiendish complicated (see bug 1204147). What is troubling me here is the test case, that works on one platform an not another. I will be travelling until mid-October, and on my travel laptop I won't be able to install a Linux environment.
Here we have another try run with debug to see why it's not working on Linux. Must be some focus problem: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cac57b49188
Debug output on Windows. As expected. de-DE element gets focus, receives dictionary de-DE. Spell check OK. Then en-US element gets focus, received dictionary en-US. Spell check OK. The spelling of the de-DE element doesn't get updated, since it lost focus. When we remove the dictionary, the both elements get notified and update. Works like a charm and as intended!! 0 INFO SimpleTest START 1 INFO TEST-START | editor/composer/test/test_bug697981.html ++DOMWINDOW == 24 (1A7C1280) [pid = 6212] [serial = 24] [outer = 19CE1780] ++DOMWINDOW == 25 (1A7C2400) [pid = 6212] [serial = 25] [outer = 19CE1780] 2 INFO must wait for load 3 INFO TEST-PASS | editor/composer/test/test_bug697981.html | true expected (de_ DE directory should exist) ***** nsEditorEventListener::Focus - StartWatchingDictionaryChanges on 1786b840 ***** mPreferredLang (element) |de-DE| ***** Assigned from element/doc |de-DE| ***** Observer spellcheck-dictionary-update on 1786b840 ***** Set |de-DE|. ***** mPreferredLang (element) |de-DE| ***** Assigned from element/doc |de-DE| ***** Set |de-DE|. 4 INFO TEST-PASS | editor/composer/test/test_bug697981.html | expected de-DE 5 INFO TEST-PASS | editor/composer/test/test_bug697981.html | one misspelled wor d expected: German ***** nsEditorEventListener::Blur - StopWatchingDictionaryChanges on 1786b840 ***** nsEditorEventListener::Focus - StartWatchingDictionaryChanges on 1786dd00 ***** mPreferredLang (element) |en-US| ***** Assigned from element/doc |en-US| ***** Observer spellcheck-dictionary-update on 1786dd00 ***** Set |en-US|. ***** mPreferredLang (element) |en-US| ***** Assigned from element/doc |en-US| ***** Set |en-US|. 6 INFO TEST-PASS | editor/composer/test/test_bug697981.html | expected en-US 7 INFO TEST-PASS | editor/composer/test/test_bug697981.html | one misspelled wor d expected: Nogoodword 8 INFO TEST-PASS | editor/composer/test/test_bug697981.html | one misspelled wor d expected: German ***** Observer spellcheck-dictionary-remove on 1786dd00 ***** Observer spellcheck-dictionary-remove on 1786b840 ***** Observer spellcheck-dictionary-remove on 14111620 9 INFO TEST-PASS | editor/composer/test/test_bug697981.html | expected en-US 10 INFO TEST-PASS | editor/composer/test/test_bug697981.html | some misspelled w ords expected: heute ist ein guter MEMORY STAT | vsize 660MB | vsizeMaxContiguous 1806MB | residentFast 263MB | hea pAllocated 67MB 11 INFO TEST-OK | editor/composer/test/test_bug697981.html | took 2885ms 12 INFO TEST-START | Shutdown 13 INFO Passed: 8 14 INFO Failed: 0 15 INFO Todo: 0
After wrestling with the Linux compiler, I got my Linux results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5af6b2c0ea8 Result: The test is absolutely good, but it fails on stuff left behind by a previous test. Some previous test sets the content preference for the whole site to en-US and therefore the de-DE doesn't take effect. Grrr. These sensitive tests need to run on a clean slate with nothing left behind from previous tests!! (No wonder it works on my home machine, we I run the test individually.) 2192 INFO TEST-START | editor/composer/test/test_bug678842.html ***** mPreferredLang (element) |testing-XXX| ***** Assigned from content preferences |en-US| ***** Writing content preferences for |en-GB| ***** Storing spellchecker.dictionary |en-GB| ***** mPreferredLang (element) |testing-XXX| ***** Assigned from content preferences |en-GB| ***** Observer spellcheck-dictionary-remove on 0x7f28faf41040 ***** Writing content preferences for |en-US| ***** Storing spellchecker.dictionary |en-US| ***** Observer spellcheck-dictionary-remove on 0x7f290b2e40c0 2193 INFO TEST-OK | editor/composer/test/test_bug678842.html | took 647ms 2194 INFO TEST-START | editor/composer/test/test_bug697981.html ***** nsEditorEventListener::Focus - StartWatchingDictionaryChanges on 0x7f28f77a8880 ***** mPreferredLang (element) |de-DE| <====== should be de-DE ***** Assigned from content preferences |en-US| <====== but gets overridden by some left-behind content preferences. ***** mPreferredLang (element) |de-DE| ***** Assigned from content preferences |en-US| 2196 INFO TEST-PASS | editor/composer/test/test_bug697981.html | true expected (de_DE directory should exist) 2197 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html | expected de-DE - got "en-US", expected "de-DE" So how do a guarantee a clean slate before the test runs? Also, Robert, are you willing to take another look at the code patch in attachment 8660355 [details] [diff] [review]? I have answered your questions in comment #58. With the patch we don't make the exiting state any worse, the only thing is that we change the observers around a little.
Flags: needinfo?(roc)
Comment on attachment 8660355 [details] [diff] [review] Proposed solution: (v8), notifying dictionary removal, changed listener logic. Asking for review again, it works.
Attachment #8660355 - Flags: review?(roc)
Comment on attachment 8660481 [details] [diff] [review] Test (v2) - now using "onfocus". Robert, the two tests are basically the same, this one works with "onfocus". Which do you prefer? Both have the problem that they fail in the test suite due to some garbage that is left behind from a previous test. Once I fix that (don't know how yet), the test will be OK, I just need to know which one you prefer.
Attachment #8660481 - Flags: feedback?(roc)
Attached patch Test (v2a) - now using "onfocus". (obsolete) (deleted) — — Splinter Review
Same thing as v2, some polish.
Attachment #8660481 - Attachment is obsolete: true
Attachment #8660481 - Flags: feedback?(roc)
Attachment #8660740 - Flags: feedback?(roc)
OK, I've raised bug 1204540 for a test cleanup, so one test doesn't leave behind what another one doesn't expect.
Attached patch Test (v2b) - now using "onfocus". (obsolete) (deleted) — — Splinter Review
By renaming the file the test resides in, it runs first and doesn't stumble over context preferences left behind by others. This is a "hacky" approach, but it gets the test going: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc91ea07e22c I prefer this version 2, since it's a little clearer what happens due to the use of "onfocus". I'd like to see this landed in FF 43 (the branch date is approaching fast), together will the other spell check improvements we made in bug 1204147 and bug 1200533. I promise to move on to bug 1204540 to clean up the problems caused by the order of the tests (and I can even bother Olli Pettay (smaug) with the review since this is not urgent). Does that sound like a deal? Also, I know that you don't really like the solution. But in fact all we do is move the observers around a little from always listening to listening when it's important. So while this area perhaps needs a more thorough clean-up, we improve the situation ... which really is puzzling: You click on another field/window/tab, and suddenly everything in the previous field/window/tab gets red underlines.
Attachment #8660740 - Attachment is obsolete: true
Attachment #8660740 - Flags: feedback?(roc)
Flags: needinfo?(roc)
Attachment #8660830 - Flags: review?(roc)
Comment on attachment 8660355 [details] [diff] [review] Proposed solution: (v8), notifying dictionary removal, changed listener logic. Review of attachment 8660355 [details] [diff] [review]: ----------------------------------------------------------------- Addressing questions another time. (Sorry, not too familiar with the remove tool.) ::: editor/libeditor/nsEditor.cpp @@ +307,5 @@ > > nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > if (obs) { > obs->AddObserver(this, > + SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION, Yes, this is right. Instead of UPDATE listening always, we now have REMOVE listening always. The spell checking engine is sending REMOVE when a dictionary gets removed from the system. @@ +452,5 @@ > if (obs) { > obs->RemoveObserver(this, > SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION); > + obs->RemoveObserver(this, > + SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION); Here in PreDestroy we now remove UPDATE as well as REMOVE. Removing UPDATE (which is OK, even it it was never added) makes sure, that no UPDATE observer is hanging around. @@ +5211,5 @@ > +nsEditor::StartWatchingDictionaryChanges() > +{ > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + if (obs) { > + obs->AddObserver(this, SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION, false); Well, I think it's properly removed, see comment above.
Sorry, not too familiar with the *review* tool.
If you look at bug 1204540 comment #2, you can see the five tests that leave content preferences set. Until this gets fixed, we need to make sure that the test here runs before these five. This seems to happen with the the silly filename test_abug697981.html. The order won't matter, once bug 1204540 gets fixed. I remember Ehsan saying: "Every time I touch this, there are more problems" or words to this effect. Perhaps he was right. But anyway, once it's all clean, it will be much better. Fixing bug 1204147 has helped a lot cleaning up the confusion.
Comment on attachment 8660355 [details] [diff] [review] Proposed solution: (v8), notifying dictionary removal, changed listener logic. Review of attachment 8660355 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/nsEditor.h @@ +903,5 @@ > bool mDidPostCreate; // whether PostCreate has been called > bool mDispatchInputEvent; > bool mIsInEditAction; // true while the instance is handling an edit action > bool mHidingCaret; // whether caret is hidden forcibly. > + bool mIsFocused; // whether the editor has received focus. OK, let's go with this. Here, rather than mIsFocused, I'd call this mObservingDictionaryUpdates and set/clear it where we add/remove the observer. (And check it before adding the observer so we don't add more than one.) That seems safer, simpler and clearer.
Attachment #8660355 - Flags: review?(roc)
Comment on attachment 8660445 [details] [diff] [review] Test (v1) Obsolete this fine test since we're going with approach 2.
Attachment #8660445 - Attachment is obsolete: true
Carrying forward Robert's r+. Rebased on the now landed bug 1204147.
Attachment #8660830 - Attachment is obsolete: true
Attachment #8661062 - Flags: review+
> OK, let's go with this. Thanks!! > Here, rather than mIsFocused, I'd call this mObservingDictionaryUpdates and > set/clear it where we add/remove the observer. (And check it before adding > the observer so we don't add more than one.) That seems safer, simpler and > clearer. I agree 100%, sorry, it was clumsy, this approach it much better. Sometimes you don't see the forest for the trees.
Attachment #8660355 - Attachment is obsolete: true
Attachment #8661063 - Flags: review?(roc)
Keywords: checkin-needed
Dear Sheriff, two patches here. Apply in any order. No special instructions. Thanks!
https://hg.mozilla.org/mozilla-central/rev/59b6a838f0f2 https://hg.mozilla.org/mozilla-central/rev/c12f114be966
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1205983
Ehsan and Robert O'Callahan (:roc) are of the opinion, that this should be backed out from Aurora. So please back out the equivalent of https://hg.mozilla.org/mozilla-central/rev/59b6a838f0f2 https://hg.mozilla.org/mozilla-central/rev/c12f114be966 from Aurora *only*. Please *leave* the landed patches on mozilla-central. Consequently the target milestone should be changed to mozilla44. Issues with the patch will be addressed in bug 1205983 in the next few days. Ehsan, I'm not sure how to proceed with backing this out. I didn't get any answer from anyone on IRC.
Flags: needinfo?(ehsan)
Wes Kocher (:KWierso) will take care of it. Thanks!
Flags: needinfo?(ehsan)
Bug 1200065 landed on top of this and is making it difficult to back this out. Should I back it out from Aurora as well so that I can easily back these patches out, or can one of you back this out instead of me?
Flags: needinfo?(roc)
Flags: needinfo?(mozilla)
Flags: needinfo?(ehsan)
Sorry for the trouble. https://hg.mozilla.org/mozilla-central/rev/c12f114be966 can be backed out without conflicts. https://hg.mozilla.org/mozilla-central/rev/59b6a838f0f2 changed five files: editor/libeditor/nsEditor.cpp editor/libeditor/nsEditor.h editor/libeditor/nsEditorEventListener.cpp extensions/spellcheck/hunspell/src/mozHunspell.cpp extensions/spellcheck/idl/mozISpellCheckingEngine.idl The changes in extensions/spellcheck cause a conflict with https://hg.mozilla.org/mozilla-central/rev/bc496e1c5963 from bug 1200065. In practical terms, it is sufficient to back out the changes on editor/libeditor only. The stuff in extensions/spellcheck is irrelevant and doesn't need backing out. Technical background: The stuff in extensions/spellcheck adds a new notification. It doesn't hurt to leave this in, since the listener in editor/libeditor will be removed. The notification is sent, and no one listens, so no big deal. Is a partial backout possible? Politically correct? Or do all traces need to be removed 100% even if they are inconsequential?
Flags: needinfo?(mozilla)
Jorg, please prepare a backout patch that Wes can apply cleanly. Thanks!
Flags: needinfo?(roc)
Flags: needinfo?(mozilla)
Flags: needinfo?(ehsan)
Actually, bug 1200065 is a refactoring bug. If that one gets pulled out as well, no one will shed a tear. Either way will do.
Wires crossed with Ehsan. Is there a problem with pulling 1200065 out as well?
Flags: needinfo?(mozilla) → needinfo?(ehsan)
There is another option. Pull out bug 1200065, pull out this bug, reland bug 1200065. Bug 1200065 only moves the files in question.
Perfect!!
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: