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

RESOLVED FIXED in Firefox 44

Status

()

Core
Spelling checker
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Amir Aharoni, Assigned: Jorg K (GMT+2))

Tracking

(Blocks: 1 bug, {regression})

9 Branch
mozilla44
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox9-, firefox10-, firefox43 affected, firefox44 fixed)

Details

Attachments

(3 attachments, 16 obsolete attachments)

1.40 KB, text/html
Details
5.68 KB, patch
Jorg K (GMT+2)
: review+
Details | Diff | Splinter Review
8.19 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.

Updated

6 years ago
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
(Reporter)

Comment 3

6 years ago
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

Comment 5

6 years ago
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.

Comment 6

6 years ago
(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.

Comment 7

6 years ago
[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

Comment 8

6 years ago
Created attachment 577664 [details]
text area switching test (needs English and Danish dictionary)

(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.

Comment 9

6 years ago
(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?

Comment 12

5 years ago
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!
Created attachment 593607 [details] [diff] [review]
Patch v1

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)
Created attachment 595041 [details] [diff] [review]
Patch v2

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.
Created attachment 597057 [details] [diff] [review]
Patch v3

(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)

Comment 24

5 years ago
(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?

Comment 25

5 years ago
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?

Comment 27

5 years ago
(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.

Updated

3 years ago
Blocks: 1073827

Updated

3 years ago
Depends on: 959785
(Assignee)

Comment 32

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #577664 - Attachment description: text area switching test → text area switching test (needs English and Danish spell checker)
(Assignee)

Comment 33

2 years ago
Created attachment 8659072 [details]
text area switching test (needs English and French or Spanish dictionary)
(Assignee)

Updated

2 years ago
Attachment #577664 - Attachment description: text area switching test (needs English and Danish spell checker) → text area switching test (needs English and Danish dictionary)
(Assignee)

Comment 34

2 years ago
Created attachment 8659487 [details] [diff] [review]
Patch (v4) (refreshed)

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)

Updated

2 years ago
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)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 36

2 years ago
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)
(Assignee)

Comment 38

2 years ago
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
(Assignee)

Comment 39

2 years ago
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?
(Assignee)

Comment 40

2 years ago
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.
(Assignee)

Comment 41

2 years ago
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.
(Assignee)

Comment 42

2 years ago
Created attachment 8659795 [details] [diff] [review]
WIP:  (v5), now also notifying dictionary removal.

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
(Assignee)

Comment 43

2 years ago
Created attachment 8659796 [details]
text area switching test (needs English and French or Spanish dictionary), now also with <div contenteditable>

Test case with <div contenteditable>.
Attachment #577664 - Attachment is obsolete: true
Attachment #8659072 - Attachment is obsolete: true
Flags: needinfo?(bugs)
(Assignee)

Comment 44

2 years ago
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 ;-)
(Assignee)

Comment 45

2 years ago
Created attachment 8660115 [details] [diff] [review]
WIP: (v6), now also notifying dictionary removal, changed listener logic.

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
(Assignee)

Comment 46

2 years ago
Created attachment 8660290 [details]
text area switching test (needs English and French or Spanish dictionary), now also with <div contenteditable>

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
(Assignee)

Comment 47

2 years ago
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.
(Assignee)

Comment 48

2 years ago
Created attachment 8660310 [details] [diff] [review]
Proposed solution: (v7), notifying dictionary removal, changed listener logic.

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
(Assignee)

Comment 49

2 years ago
Created attachment 8660312 [details] [diff] [review]
Proposed solution: (v7), notifying dictionary removal, changed listener logic.

Made it compile on Linux. Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b16833982c0f
Attachment #8660310 - Attachment is obsolete: true
(Assignee)

Comment 50

2 years ago
Created attachment 8660355 [details] [diff] [review]
Proposed solution: (v8), notifying dictionary removal, changed listener logic.

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)
(Assignee)

Comment 51

2 years ago
Created attachment 8660445 [details] [diff] [review]
Test (v1)

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)
(Assignee)

Comment 52

2 years ago
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)
(Assignee)

Comment 53

2 years ago
Created attachment 8660481 [details] [diff] [review]
Test (v2) - now using "onfocus".

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
(Assignee)

Comment 54

2 years ago
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 ;-)
(Assignee)

Comment 55

2 years ago
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)
Attachment #8660445 - Flags: feedback?(roc) → feedback+
(Assignee)

Comment 58

2 years ago
(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.
(Assignee)

Comment 59

2 years ago
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
(Assignee)

Comment 60

2 years ago
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
(Assignee)

Comment 61

2 years ago
Sorry, one more time, with more debug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83692b820227
(Assignee)

Comment 62

2 years ago
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)
(Assignee)

Comment 63

2 years ago
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)
(Assignee)

Comment 64

2 years ago
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)
(Assignee)

Comment 65

2 years ago
Created attachment 8660740 [details] [diff] [review]
Test (v2a) - now using "onfocus".

Same thing as v2, some polish.
Attachment #8660481 - Attachment is obsolete: true
Attachment #8660481 - Flags: feedback?(roc)
Attachment #8660740 - Flags: feedback?(roc)
(Assignee)

Comment 66

2 years ago
OK, I've raised bug 1204540 for a test cleanup, so one test doesn't leave behind what another one doesn't expect.
(Assignee)

Comment 67

2 years ago
Created attachment 8660830 [details] [diff] [review]
Test (v2b) - now using "onfocus".

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)
(Assignee)

Comment 68

2 years ago
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.
(Assignee)

Comment 69

2 years ago
Sorry, not too familiar with the *review* tool.
(Assignee)

Comment 70

2 years ago
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)
Attachment #8660830 - Flags: review?(roc) → review+
(Assignee)

Comment 72

2 years ago
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
(Assignee)

Comment 73

2 years ago
Created attachment 8661062 [details] [diff] [review]
Test (v2c) - now using "onfocus".

Carrying forward Robert's r+.
Rebased on the now landed bug 1204147.
Attachment #8660830 - Attachment is obsolete: true
Attachment #8661062 - Flags: review+
(Assignee)

Comment 74

2 years ago
Created attachment 8661063 [details] [diff] [review]
Proposed solution: (v9), notifying dictionary removal, changed listener logic.

> 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)
Attachment #8661063 - Flags: review?(roc) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 75

2 years ago
Dear Sheriff, two patches here. Apply in any order. No special instructions. Thanks!

Comment 76

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b6a838f0f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c12f114be966
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/59b6a838f0f2
https://hg.mozilla.org/mozilla-central/rev/c12f114be966
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1205983
(Assignee)

Comment 78

2 years ago
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)
(Assignee)

Comment 79

2 years ago
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)
(Assignee)

Comment 81

2 years ago
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)
(Assignee)

Comment 83

2 years ago
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.
(Assignee)

Comment 84

2 years ago
Wires crossed with Ehsan. Is there a problem with pulling 1200065 out as well?
Flags: needinfo?(mozilla) → needinfo?(ehsan)
(Assignee)

Comment 85

2 years ago
There is another option. Pull out bug 1200065, pull out this bug, reland bug 1200065.
Bug 1200065 only moves the files in question.
Comment 85 seemed to work:
Backed out from aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/38fe96244714
https://hg.mozilla.org/releases/mozilla-aurora/rev/43a54fcfb388
status-firefox43: fixed → affected
status-firefox44: --- → fixed
Target Milestone: mozilla43 → mozilla44
(Assignee)

Comment 87

2 years ago
Perfect!!
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.