1205983 - Further clean-up of spelling re-check on focus after bug 697981: 1) Too much checking for <div contenteditable> (should ignore irrelevant nodes) - 2) Does not work when dictionary is removed.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jorg K (CEST = GMT+2)]

The following was discovered while working on bug 697981:

When you have textareas on a page and a <div contenteditable>, clicking on the textarea triggers two focus events: One for the textarea, another one for the div.

To demonstrate that, install a French or Spanish dictionary and open attachment 8660290 [details]. In bug 697981 we changed the spell check update so it only updates the element which has focus, however, the div updates when clicking into the other textareas, since it always receives focus.

Comment 1

[Jorg K (CEST = GMT+2)]

Ehsan, can you please comment on this.

There is another thing I'd like to ask. Obviously the focus event is dispatched to the editor:
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditorEventListener.cpp#1065.

The three different divs on the page somehow share the same editor which always receives the focus. Why are there not three editors for three divs as there are three editors for three textareas? Is this by design? With only one editor, all three divs always update, unlike the textareas.

IMHO the ideal situation would be three editors which receive focus correctly.

Since bug 697981 landed a few days ago, you can use a Nightly to try it.

Comment 2

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #1)
> Ehsan, can you please comment on this.

I am not sure what the question is.  Sending the focus event to both the div and the textarea seems correct based on a cursory look over <https://html.spec.whatwg.org/multipage/interaction.html#focus-update-steps>, but if you have specific questions about the focus manager, you should ask Neil Deakin.

> There is another thing I'd like to ask. Obviously the focus event is
> dispatched to the editor:
> https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/
> nsEditorEventListener.cpp#1065.
> 
> The three different divs on the page somehow share the same editor which
> always receives the focus. Why are there not three editors for three divs as
> there are three editors for three textareas? Is this by design? With only
> one editor, all three divs always update, unlike the textareas.

That's how contenteditable is implemented in Gecko.  I am not sure about why, presumably because there was support for one editor object for designMode, and we just stuck with that when we implemented contenteditable.

> IMHO the ideal situation would be three editors which receive focus
> correctly.

Sure, but that is a *lot* of work, and you can basically assume that it will never happen in practice unfortunately.

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #2)
> Sending the focus event to both the div
> and the textarea seems correct based on a cursory look over
> <https://html.spec.whatwg.org/multipage/interaction.html#focus-update-steps>,
> but if you have specific questions about the focus manager, you should ask
> Neil Deakin.

Hmm, I have trouble understanding this document, so here's the simple question:

On a page like attachment 8660290 [details] (three textareas and three <div contenteditable>) why does the editor of the <div contenteditable> receive focus and blur events when I'm clicking only on the three text areas?

Comment 4

[Jorg K (CEST = GMT+2)]

To clarify: "editor of the <div contenteditable> receives focus" shall mean:
This is called:
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditorEventListener.cpp#1065

Comment 5

[Neil Deakin]

I don't see multiple events being fired here. Only one focus event is fired when I focus the textarea or contenteditable. Maybe the editors or listeners are being shared or something?

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, which example are you using?

Surely, this example gives two alerts only:
<body>
<textarea onfocus="alert('textarea')">This is a textarea</textarea>
<div style="background-color:#eee;border:1px solid #000;padding:10px" contenteditable=""
onfocus="alert('div')">
here's a div</div>
</body>

But if (without the onfocus="alert('xxx')") you print at
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditorEventListener.cpp#1065
and
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditorEventListener.cpp#1133
you see this:

Click on div:
***** nsEditorEventListener::Focus on 12E9F800 <- div receives focus

Click below the div:
***** nsEditorEventListener::Blur on 12E9F800 <- div loses focus

Click on textarea:
***** nsEditorEventListener::Focus on 12E9F800 <- div receives focus
***** nsEditorEventListener::Focus on 12E53840 <- textarea receives focus

Then click on div:
***** nsEditorEventListener::Blur on 12E9F800 <- div loses focus
***** nsEditorEventListener::Blur on 12E53840 <- textarea loses focus
***** nsEditorEventListener::Focus on 12E9F800 <- div receives focus

Is this expected/desired or is this a bug?

Since the div receives a focus event when you click on the textarea, we get unwanted spell check updates in the div.

Comment 7

[Neil Deakin]

It looks like the contenteditable adds a capturing focus event listener on the document. This of course will capture all focus events on any elements in the document, so both listeners will receive the notification when the textarea is focused.

The listener should either be added to the contenteditable area instead, or check the target during the listener.

Comment 8

[(no longer active)]

(In reply to Neil Deakin from comment #7)
> It looks like the contenteditable adds a capturing focus event listener on
> the document.

Yes, it does.

> The listener should either be added to the contenteditable area instead, or
> check the target during the listener.

We usually check the target of events such as focus to make sure that they fall into an editable region of the document.  We can't easily switch to setting up the event listeners on contenteditable areas.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Neil Deakin from comment #7)
Neil, thank you for your analysis.

> The listener should either be added to the contenteditable area instead, or
> check the target during the listener.
According to what Ehsan said, only the second option is viable.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #8)
> We usually check the target of events such as focus to make sure that they
> fall into an editable region of the document.
Apparently that hasn't worked. Where would I need to implement that?
I added StartWatchingDictionaryChanges() here in nsEditorEventListener::Focus():
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditorEventListener.cpp#1117
There is some checking done in nsEditorEventListener::Focus() (which sadly I do not understand) and there are some early returns, however, the dictionary observer is started although the contenteditable has not received focus. This leads to unwanted spell check updates of unfocused elements.

So how do I go about fixing that, or do you (plural) have time to do it?
Who would review the change?

> We can't easily switch to
> setting up the event listeners on contenteditable areas.
OK, I won't pursue this further then.

Comment 10

[(no longer active)]

The code you have added is wrong.  You need to verify that the target of the event falls into an editable section.  You can do so by calling GetActiveEditingHost() to get the root of the active editable region, if any, and use nsContentUtils::ContentIsDescendantOf() to ensure that the target node is a descendant of the active editable region.

Please also back out bug 697981 from Aurora as the code on central is being uplifted to Aurora today, and I think we need to fix this properly on trunk before this is ready to ride the trains.

Thanks.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #10)
> The code you have added is wrong.
I had it reviewed ;-(

> You need to verify that the target of the
> event falls into an editable section.  You can do so by calling
> GetActiveEditingHost() to get the root of the active editable region, if
> any, and use nsContentUtils::ContentIsDescendantOf() to ensure that the
> target node is a descendant of the active editable region.
Thanks, I'll look into it.

> Please also back out bug 697981 from Aurora as the code on central is being
> uplifted to Aurora today, and I think we need to fix this properly on trunk
> before this is ready to ride the trains.
Is it really this dramatic? It fixes most if not all of bug 697981 (as per it's description), only that I was so picky to start testing with contenteditable.

I'd hardly call it a "regression". Nothing that previously worked stopped working, most of the stuff that previously didn't work now works, but there is room for improvement.

Having this bug will also allow to pin a new test to it.

We can easily uplift the correction to Aurora.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution.

Is there more to it than this? I gave it a quick test and it seems to do the right thing.

Comment 13

[Jorg K (CEST = GMT+2)]

Sorry, I'll remove the trailing whitespace later.

Comment 14

[(no longer active)]

Comment on attachment 8663765 [details] [diff] [review]
Possible solution.

Please ask roc for review, since he reviewed the original patch.  I keep repeating that I don't have review bandwidth.  When that changes, I will update my Bugzilla name to reflect that.

On the question of backout vs take a fix on Aurora, I suggest you discuss with the release manager.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v2).

Forgot to assure that the editing host isn't NULL as Ehsan said in comment #10.

On the non-technical side, Ehsan is not happy with my work in bug 697981, to the point where he requested to have it backed out.

I believe that what was landed in bug 697981 was correct, only incomplete for some cases not even mentioned in the bug. The bug talked about "tabs" and "textareas", and both are fixed. Only due to my obsession with contenteditable, I tested that and noticed that it didn't work 100%. It's OK, if the contenteditable is in another tab, or window, so that's already an improvement.

So how shall we progress?

I suggest:
- You review this patch.
- I add a test.
- We land both.
- We ask the release manager to uplift to Aurora.

There is really no harm done in what got landed in bug 697981. And surely this bug here is not a regression.

What do you think?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8663791 [details] [diff] [review]
Possible solution (v2).

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

We should back out 697981 on Aurora. That's simpler than trying to uplift fixes to Aurora; this might not be the last one.

::: editor/libeditor/nsEditorEventListener.cpp
@@ +1122,5 @@
> +    nsIContent* editingHost = htmlEditor->GetActiveEditingHost();
> +    // If our target node is not a descandant of the editing host,
> +    // we don't start the watch.
> +    if (!(editingHost &&
> +          nsContentUtils::ContentIsDescendantOf(node, editingHost))) {

It seems to me we should just check node->GetEditingHost() == htmlEditor->GetActiveEditingHost() here instead of testing this way.

You should write a test where a non-editable island of content inside a contenteditable gets focus; it should not start watching here.

We'll need a test for the bug you're fixing as well.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> We should back out 697981 on Aurora. That's simpler than trying to uplift
> fixes to Aurora; this might not be the last one.
I'm not sure why you're saying this. Bug 697981 does fix the cases mentioned in its summary and description. We had confusing spell check marks for years (and no one seemed to care), now they are gone is most cases. This is an additional case which can be uplifted to Aurora, but it's not even compulsory. All of what was implemented in bug 697981 reduces the spell check confusion, and in this bug, we will reduce it even more.

> It seems to me we should just check node->GetEditingHost() ==
> htmlEditor->GetActiveEditingHost() here instead of testing this way.
I'll try that.

> You should write a test where a non-editable island of content inside a
> contenteditable gets focus; it should not start watching here.
Hmm. As I said, all this is too avoid undesired spell check marks. If the user clicks on a fixed island surrounded by editable text, it is not fatal that we check the surrounding text.

Please keep comment #2 in mind: Multiple subsequent contenteditables share the *same* editor. So confusing spell checking is unavoidable. Try attachment 8660290 [details].

> We'll need a test for the bug you're fixing as well.
Yes, I will prepare a test for this bug here.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Possible solution (v3)

> It seems to me we should just check node->GetEditingHost() ==
> htmlEditor->GetActiveEditingHost() here instead of testing this way.
Done here.

> You should write a test where a non-editable island of content inside a
> contenteditable gets focus; it should not start watching here.
Further to what I said above:

I've added
  <div contenteditable="" lang="fr">
  Ici unn texte en <div contenteditable="false">français avec beaucoup de mots</div>
  en français</div>
as a "non-editable island". However, this is treated as block, and the spell checking updates when clicked on, since it lies in a surrounding editable area. Actually, when you click on it, it selects as a whole and when you delete the block, you're left with only editable content which should have spell updated before. So unless I am misinterpreting what you said, IMHO your suggestion is not realistic.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: WIP: Possible solution (v4)

I've noticed another twist. Now that we're listening selectively depending on focus, we *always* need to be modified when the dictionary is set, not only when it has changed. We potentially need to update areas which gain focus, but are checked in the previous language. This is of course a rare case only reproducible with contenteditables in different languages, like so:
textarea en
contenteditable en
contenteditable es.
Click on contenteditable "es", contenteditable "en" also updates with misspellings, grrr (but need to accept for now). Click on textarea "en". Textarea updates, contenteditable "en" loses focus and still show misspellings since it was checked in Spanish. Now click on contenteditable "en". It doesn't update, since the language hasn't changed. With this patch, it updates.

Looks like we're set and I'll work on the test next.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v5) with test.

Here is my proposal with test.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d84f258399a3

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Possible solution (v6) with test.

Changed the logic to detect whether we should start watching.
Why isn't it just: Do not watch if !node->GetEditingHost()?
IOW: Do not watch, if the target node does not have an editing host, that is, is not being edited.

Before:
+      nsCOMPtr<nsIContent> nodeEditingHost = content->GetEditingHost();
+      nsCOMPtr<nsIContent> editingHost = htmlEditor->GetActiveEditingHost();
+      if (!(editingHost && nodeEditingHost && editingHost == nodeEditingHost)) {
+        startWatch = false;
+      }
Now:
+      nsCOMPtr<nsIContent> nodeEditingHost = content->GetEditingHost();
+      if (!nodeEditingHost) {
+        startWatch = false;
+      }

Comment 22

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #17)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> > We should back out 697981 on Aurora. That's simpler than trying to uplift
> > fixes to Aurora; this might not be the last one.
> I'm not sure why you're saying this. Bug 697981 does fix the cases mentioned
> in its summary and description. We had confusing spell check marks for years
> (and no one seemed to care), now they are gone is most cases.

Jorg, you seem to be taking what I and roc are telling you as judgement on that patch.  It's not what we are saying, and you don't need to keep defending the patch not being backed out, it's not productive.

The reason why we have the release trains is to give things enough time to bake on trunk before we uplift them to Aurora so that we can fix most regressions on trunk before the Aurora users get affected by it (and similarly for Aurora -> Beta, and Beta -> Release).  In cases where we are dealing with a bug that needs to be fixed both on trunk and Aurora, the *default* action is to back out from Aurora and fix the bug on trunk, unless there is a great reason why the patch should not be backed out from Aurora, *and* the fix is super simple and non-risky.  This case meets neither of the two criteria, and as such, it should be backed out from Aurora.  This is how our development process works.  Reiterating your arguments over and over again changes *nothing*, unless if you can demonstrate that there is a great reason why your patch should not be backed out from Aurora *and* that the fix is super simple in that we can be fairly certain that there will be no regressions from the fix (and no more regressions from the original bug.)

(And please note that this happens to all of us, myself and roc included.  I have had hundreds of patches backed out on Aurora or Beta.  This is quite normal, and nothing to stress out about.)

> This is an
> additional case which can be uplifted to Aurora, but it's not even
> compulsory.

Quite to the contrary.  When a patch that gets landed breaks something, it should be fixed.  That is not a matter of opinion.

> All of what was implemented in bug 697981 reduces the spell
> check confusion, and in this bug, we will reduce it even more.

Sure.  You are essentially arguing that bug 697981 was a useful fix, and everyone agrees with that.  But that is not how we decide whether something needs to be backed out from Aurora.  I hope the above explanation makes sense.

I know that this process may be strange to you, and you may think that this case should be exempt from it, but there is a good reason for this process, that is, eliminating risk to the quality of the software we ship as early as possible, and give developers as long of a time as we can to make sure that their fix is completely correct without any undesirable side effects.

Comment 23

[Jorg K (CEST = GMT+2)]

Backing out of Aurora *only* requested in bug 697981 comment #78.

Comment 24

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d84f258399a3 is green.
Please refer to comment #21 to continue the technical discussion on this bug.

Comment 25

[Jorg K (CEST = GMT+2)]

Here is a green try run with the code from attachment 8664215 [details] [diff] [review]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45cb09a5e9d6

Comment 26

[Jorg K (CEST = GMT+2)]

Houston, we've got a problem.

It was Ehsan's expressed wish from bug 697981 comment #37, that spell check marks should appear when a dictionary gets removed. So we introduced SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION and listen to it. It is treated the same as SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION, that is it triggers a recheck of all elements with the *last* set dictionary, since spell checking only knows the *current* dictionary.

This simplistic approach doesn't even work correctly for two elements: When element 2 has focus, hence element 2's dictionary is current, and we remove the dictionary for element 1, element 1 gets spell checked in element 2's language. Surely that will/may give spell check marks, but isn't right. It becomes apparent with three elements in different languages. Removing element 1's dictionary rechecks elements 1, 2 and 3 in element 2's language.

There is a variation on the theme: If you remove the dictionary of the element that has focus, all other elements are rechecked in the language of the removed dictionary, but since it was removed, this doesn't work and their marks are just removed.

No good.

So next I did this in the observer:
+  if (!strcmp(aTopic, SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION)) {
+    if (mInlineSpellChecker) {
+      // On removal, determine the language of the elements again which
+      // will trigger a recursive update.
+      mInlineSpellChecker->UpdateCurrentDictionary();
+      return NS_OK;
+    }
+  }

This in principle is the correct approach, since we determine the language for each element again which then triggers a recheck.

However, here we run into timing problems: In my test case with three different languages it happens more or less randomly that an element is checked in the wrong language. This is because the observers set the "current" dictionary for all elements visited and create asynchronous re-spell requests. By the time the re-spell finally happens, the "current" dictionary can already be different.

So I'd like to ask Ehsan whether we can part with the idea of having spell check marks appear when a dictionary is removed. Instead the spell check marks will appear when the element receives focus again. Since removing a dictionary manually happens in another tab or window, the element whose dictionary is removed will be re-checked when it next gets focus.

Can we settle for this compromise?

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8664215 [details] [diff] [review]
Possible solution (v6) with test.

Clearing the feedback request for now. We need to decide first how to handle dictionary removals. Due to the limitations of one "current" dictionary and asynchronous processing, a solution that guarantees correct spelling everywhere upon dictionary removal would be quite complex.

I'm still in favour of guaranteeing correct spelling upon focus, and no change when there is no focus. That would simplify the solution tremendously.

Let's keep in mind what bug 697981 was about. It's basically a regression from bug 338427 (spelling by content) and bug 678842 (spelling by website/content preference). Since the days of Mozilla8 people have been puzzled by the wrong spelling in areas that a second ago had focus and were spelled correctly.

Providing a 97.3% solution to the problem (whose only downside would be, that upon removal of a dictionary, an update of the checking would happen at the next focus and not immediately) is much better than doing nothing.

Sadly, the will to handle "dictionary removal spell check updates" immediately, badly derailed the solution of bug 697981, to the point where it now does these updates, but in the wrong language.

How should be proceed?

Ehsan?

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: Various screenshot of previous, current and possible future state.

Can be reproduced with attachment 8660290 [details].

Comment 29

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #26)
> So I'd like to ask Ehsan whether we can part with the idea of having spell
> check marks appear when a dictionary is removed. Instead the spell check
> marks will appear when the element receives focus again. Since removing a
> dictionary manually happens in another tab or window, the element whose
> dictionary is removed will be re-checked when it next gets focus.

Hmm, what would happen in this case?  The user has two windows, in one they focus one editable area with language X, and then they Alt-Tab to another window and remove the dictionary for langauge X, and then they switch back to the first window.  Would we not change the spell checking of the focused area in that case?  Or would that count as the element receiving focus again and thus being re-checked?

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #29)
> Hmm, what would happen in this case?  The user has two windows, in one they
> focus one editable area with language X, and then they Alt-Tab to another
> window and remove the dictionary for langauge X, and then they switch back
> to the first window.  Would we not change the spell checking of the focused
> area in that case?  Or would that count as the element receiving focus again
> and thus being re-checked?

In the case you describe, the editable area with language X receives focus again. Here is the debug:

***** nsEditorEventListener::Focus on 1298A350 <--- first click on the element.
***** nsEditorEventListener::Startwatch on 1298A350
***** mPreferredLang (element) |es-ES|
***** Assigned from element/doc |es-ES|
***** nsEditor::Observe spellcheck-dictionary-update on 1298A350
***** Set |es-ES|.
***** mPreferredLang (element) |es-ES|
***** Assigned from element/doc |es-ES|
***** nsEditor::Observe spellcheck-dictionary-update on 1298A350
***** Set |es-ES|.
***** nsEditorEventListener::Blur on 1298A350  <--- alt tab to a window with the "Add-ons Manager"]
Dictionary is removed, observer is called, but for my experiments I have it just return OK and do nothing, which is the proposed solution.
***** nsEditor::Observe spellcheck-dictionary-remove on 12AF6640
***** nsEditor::Observe spellcheck-dictionary-remove on 1298AD10
***** nsEditor::Observe spellcheck-dictionary-remove on 1298A350
***** nsEditor::Observe spellcheck-dictionary-remove on 0F28D460
Alt tab back, an the area gets focus again:
***** nsEditorEventListener::Focus on 1298A350
***** nsEditorEventListener::Startwatch on 1298A350
***** mPreferredLang (element) |es-ES|
***** Assigned from element/doc |es-ES|
***** Setting of |es-ES| failed (or it wasn't available)
***** Trying to find match for language code |es|
***** Trying locale |en-US|
***** nsEditor::Observe spellcheck-dictionary-update on 1298A350
***** Set |en-US|.
This time, it has to be en-US since es-ES was removed.

Anyway, the question that interests me is how do I get the derailed train back on track. I have a plan, but it's not what you first wanted.

I know you hate repetitions, so I won't repeat that checking everything in the correct language after dictionary removal is not really feasible given the current restrictions ;-)

You could also take a look at the attached PDF which shows the situation.

Comment 31

[Jorg K (CEST = GMT+2)]

Just to clarify:

If the area with language X had focus before the Alt-Tab, then the area will be rechecked upon Alt-Tab back, since the Alt-Tab back re-establishes the focus.

If the area with language X had *lost* focus before the Alt-Tab (because the user clicked elsewhere), then the area will *not* be rechecked upon Alt-Tab back. The recheck will happen when it next receives focus.

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: Patch for feedback (test will be reincluded later)

Guys, we've been shooting ourselves in the foot with the observers.

For the element that receives focus, the spellcheck works without any observer. In the RAII solution presented in attachment 597057 [details] [diff] [review] in bug 697981, the observer was only active during the "on focus" event, and no one ever noticed that the observer *never* got called, since the SetCurrentDictionary call happens *after* the "on focus" has long returned. The observer was added in bug 591780 to (erroneously) update the *other* elements that don't receive focus. If we don't want updating on those, we can just completely *REMOVE all observer stuff*. It works! (... if you accept that dictionary removals don't get rechecked immediately). And surely, the RAII solution also worked (with the same limitation), since it disabled the observer in a very complicated way ... and we all fell for it ... trying to reduce the observer activity time span, to exclude irrelevant targets and trying to make it work for removals (which is a losing battle).

The only other little tweak is mozInlineSpellChecker.cpp:
-  if (!mPreviousDictionary.Equals(currentDictionary)) {
-    nsresult rv = SpellCheckRange(nullptr);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
+  nsresult rv = SpellCheckRange(nullptr);
+  NS_ENSURE_SUCCESS(rv, rv);
The sad part is, that Ehsan knew this all along, see 697981 comment #4.

If you agree, I will re-include the test for this bug and adjust the tests for bug 697981 and test_add_remove_dictionaries.xul (bug 591780), since you now need to focus to see an recheck after dictionary removal.

What do you think?

Ehsan, please note: I'm not asking for review, but since we're already discussing this issue, please spend 30 seconds with the included patch to see that I've just culled code. Only code is removed, zero new code, zero refactoring, straight deletions.

I can also compile a version for you to try, but given bug 1208680, you will currently crash when right-clicking to check the dictionary.

Comment 33

[Jorg K (CEST = GMT+2)]

Green try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb146fc796f5

Two predicted failures that must be fixed:
2560 INFO TEST-UNEXPECTED-FAIL | editor/composer/test/test_bug697981.html
2670 INFO TEST-UNEXPECTED-FAIL | extensions/spellcheck/tests/chrome/test_add_remove_dictionaries.xul

I know Ehsan hates repetition, but I'll say it one more time:

Bug 697981 (and this one here) is an obvious regression of bug 591780. I was wrong in comment #27. It is *not* a regression from bug 338427 (spelling by content) and bug 678842 (spelling by website/content preference). When these landed on mozill8 and mozilla9, text input fields *were* spelled correctly in their individual language. It was just after bug 591780 landed, that they were all spelled in the same (wrong) language, due to the misguided observers:
https://hg.mozilla.org/mozilla-central/rev/e3cfe5ae818c

Selecting the correct spelling language individually was sacrificed for immediate update at dictionary removal time, which IMHO was a very bad trade. Bug 591780 also introduced one absolutely fatal regression (bug 1204147), where *random* content preferences got written wrongly and without user interaction. There was also another variation on the same theme: Bug 1193293. All this, together with other quirks (bug 717433) caused massive user confusion, which is documented in the meta-bug 1073827 with 20 dependants, all but one irreducible bug I have closed without major opposition. (In one of the 19 bugs I closed, a user claims, his problem still exists at random, but couldn't deliver a reproducible case).

If we land this here as suggested, the entire spell-check area will be (almost) squeaky clean, with two shortcomings due to internal restrictions:
1) No immediate update at dictionary removal time, update at next focus.
2) All contenteditables on the same page share the same editor and are hence spelled in the
   same language. I guess, we have to live with that. Note:
   translate.google.com uses a text area for the source language, so it doesn't have a problem.

There, I held my plea for getting the derailed train back on track, and removing all the observer stuff will make things so much simpler. (Perhaps I should have become a lawyer instead ;-))

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution: Complete removal of observer code from nsEditor.

Showing the full extent of the cull. CheckCurrentDictionary is removed as well.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8666435 [details] [diff] [review]
Proposed solution: Complete removal of observer code from nsEditor.

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

That sounds reasonable to me.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: Two updated tests and a new test.

All I changed in the existing tests was to add blur/focus.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a43495ced6

Comment 37

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a43495ced6 - Mochitest "Other" is green.

Testing harder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7face9be4aab

Comment 38

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #33)
> 2) All contenteditables on the same page share the same editor and are hence
> spelled in the
>    same language. I guess, we have to live with that.

Hmm, we're supposed to be able to handle the following test case correctly, but spell checking the first paragraph in Spanish and the second in French if you have both dictionaries installed:

<div contenteditable lang="es-ES">Spanish</div>
<div contenteditable lang="fr-FR">French</div>

Can you please verify that your changes do not break this?

And can you please attach one patch that contains all of the code and test changes related to this so that I can review it?  (It seems that would be a combination of the 2nd and 3rd patches on this bug, but this bug has gotten way too long and I can't be sure of what we're doing here any more!)

Thanks!

Comment 39

[Jorg K (CEST = GMT+2)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #38)
> (In reply to Jorg K (GMT+2) from comment #33)
> > 2) All contenteditables on the same page share the same editor and are hence
> >    spelled in the same language. I guess, we have to live with that.
> 
> Hmm, we're supposed to be able to handle the following test case correctly,
> but spell checking the first paragraph in Spanish and the second in French
> if you have both dictionaries installed:
> 
> <div contenteditable lang="es-ES">Spanish</div>
> <div contenteditable lang="fr-FR">French</div>
(There is no "fr-FR", they have four: fr-modern, fr-classic, fr-reform, fr-classic-reform ;-))

No, we don't handle 
<div contenteditable lang="es-ES">Spanish - español</div>
<div contenteditable lang="fr-modern">French - français</div>
correctly. Never did, don't do now and won't do for a while. Please check attachment 8660290 [details] an a current version of FF.

> Can you please verify that your changes do not break this?
They don't change this. It's "subobtimal" and remains "subobtimal".

> And can you please attach one patch that contains all of the code and test
> changes related to this so that I can review it?  (It seems that would be a
> combination of the 2nd and 3rd patches on this bug, but this bug has gotten
> way too long and I can't be sure of what we're doing here any more!)
Both patches attached represent the solution: One is a code-cull and one are the tests.
(If you think that 39 comments are a lot, I should point you to bugs with 250+ ;-))

I have another try run going:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7face9be4aab
Too many oranges for my taste, but nothing that I can link back to the changes I made.
Especially W-e10s(2 and 3) seem weird and don't offer a bug to pin it to.

Comment 40

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #39)
> Please check attachment 8660290 [details] on a "current" version of FF.
Actually, anything with contenteditable triggers bug 1204147, and the languages determined by the elements quickly get overridden be erroneously written content preferences, so it's hard to see what the current version actually does. If you try a "current" version of FF, use a SQLLite browser and keep deleting the unwanted content preference. It's a real pain.

Maybe you want to try a daily to convince yourself that multiple contenteditables in different languages never worked.

Comment 41

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #39)
> No, we don't handle 
> <div contenteditable lang="es-ES">Spanish - español</div>
> <div contenteditable lang="fr-modern">French - français</div>
> correctly. Never did, don't do now and won't do for a while. Please check
> attachment 8660290 [details] an a current version of FF.

That is surprising, since this was done in bug 338427 (and you should probably keep that in mind since you're working on spell checker code.  I appreciate if you can investigate why that doesn't work here, but we don't need to talk about that here.)

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #41)
> That is surprising, since this was done in bug 338427 (and you should
> probably keep that in mind since you're working on spell checker code.  I
> appreciate if you can investigate why that doesn't work here, but we don't
> need to talk about that here.)

I'm surprised that you're surprised, since we talked about it in comment #2. ;-)
The test for bug 338427 is absolutely "rudimentary", not to call it "poor".
I'm doing some more testing in the test for bug 1200533, but it's using textareas.
Without diving into it, I believe it may have to do with the
mozInlineSpellChecker.cpp: SpellCheckRange(nullptr); where we blindly recheck all ranges.
(I'm not sure what "all ranges" encompasses, maybe various ranges in the same editor but in different <div>s - correct me, if I'm wrong.)

Yes, we can continue the discussion elsewhere.

Comment 43

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #42)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #41)
> > That is surprising, since this was done in bug 338427 (and you should
> > probably keep that in mind since you're working on spell checker code.  I
> > appreciate if you can investigate why that doesn't work here, but we don't
> > need to talk about that here.)
> 
> I'm surprised that you're surprised, since we talked about it in comment #2.

We're talking past each other.  :-)  I filed bug 1209220.

Comment 44

[(no longer active)]

Comment on attachment 8666435 [details] [diff] [review]
Proposed solution: Complete removal of observer code from nsEditor.

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

::: extensions/spellcheck/hunspell/glue/mozHunspell.cpp
@@ +608,5 @@
>  NS_IMETHODIMP mozHunspell::RemoveDirectory(nsIFile *aDir)
>  {
>    mDynamicDirectories.RemoveObject(aDir);
>    LoadDictionaryList(true);
>    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();

You don't need this part any more though, right?  Please remove it.  You should also remove similar code for SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION.  And after that you should remove both #defines from mozISpellCheckingEngine.idl.

Comment 45

[(no longer active)]

Comment on attachment 8666713 [details] [diff] [review]
Two updated tests and a new test.

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

::: editor/composer/test/test_bug697981.html
@@ +98,5 @@
>      // Remove the fake de_DE dictionary again.
>      hunspell.removeDirectory(de_DE);
>  
> +    // Focus again, so the spelling gets updated, but before we need to kill the focus handler.
> +    elem_de.onfocus = function () { return; };

Nit: just set onfocus to null.

Comment 46

[Jorg K (CEST = GMT+2)]

Before I address the nits:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7face9be4aab
There are thee failures that don't offer anything to pin:

TEST-UNEXPECTED-TIMEOUT | /html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-03.html | expected OK
Return code: 1 

PROCESS | 2104 | Assertion failure: mRawPtr != 0 (You can't dereference a NULL nsCOMPtr with operator->().), at ../../dist/include/nsCOMPtr.h:734
TEST-UNEXPECTED-TIMEOUT | /html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html | expected OK
Return code: 1

I'm not to blame, am I?

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: Two updated tests and a new test. (v2, nits fixed)

Carrying forward Ehsan's r+.
Nits fixed.

Comment 48

[(no longer active)]

(In reply to Jorg K (GMT+2) from comment #46)
> Before I address the nits:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7face9be4aab
> There are thee failures that don't offer anything to pin:
> 
> TEST-UNEXPECTED-TIMEOUT |
> /html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-03.
> html | expected OK
> Return code: 1 
> 
> PROCESS | 2104 | Assertion failure: mRawPtr != 0 (You can't dereference a
> NULL nsCOMPtr with operator->().), at ../../dist/include/nsCOMPtr.h:734
> TEST-UNEXPECTED-TIMEOUT |
> /html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.
> html | expected OK
> Return code: 1
> 
> I'm not to blame, am I?

I doubt it.  You can ask :jgraham to double check.

Comment 49

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution: Complete removal of observer code from nsEditor (v2).

Carrying forward f+ from Ehsan and Robert (:roc).

The difference to the previous version is that SPELLCHECK_DICTIONARY_UPDATE_NOTIFICATION
was also culled, since it's not needed any more.
(I was going to leave it, since one day it could become useful again.)

The SPELLCHECK_DICTIONARY_REMOVE_NOTIFICATION is needed for Thunderbird.
There the language is determined by the document's "lang" attribute. If a dictionary is removed, we need to set a different value.

I'm confused as to who to ask for review, ah well ;-)

Comment 50

[Jorg K (CEST = GMT+2)]

Interdiff to see the extended cull:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8666435&action=interdiff&newid=8666927&headers=1
(I also shifted a comment.)
(I'm not sure whether the verb "to cull" is Australian English only. There they use it to mean: "Remove/kill something that's not needed or superfluous").

Comment 51

[James Graham [:jgraham]]

I've seen those errors on other wpt e10s runs, so I don't think anything here causes it.

Comment 52

[Jorg K (CEST = GMT+2)]

But the sheriffs need some bug to pin it to, right? Otherwise they'll back out my patch :-((

Comment 53

[James Graham [:jgraham]]

wpt-e10s isn't running on inbound yet, so you won't be backed out for failures. The Windows backlog from last week has unfortunately caused problems getting all the metadata up to date and the tests enabled.

Comment 54

[Jorg K (CEST = GMT+2)]

Just noticed: #include "mozISpellCheckingEngine.h" can go from nsEditor.cpp.
I'll fix that up after the review together with the review issues.

Comment 55

[(no longer active)]

Comment on attachment 8666927 [details] [diff] [review]
Proposed solution: Complete removal of observer code from nsEditor (v2).

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

r=me with the below fixed.

::: extensions/spellcheck/hunspell/glue/mozHunspell.cpp
@@ +595,5 @@
> +  /*
> +   * This notification is needed for Thunderbird. Thunderbird derives the dictionary
> +   * from the document's "lang" attribute. If a dictionary is removed,
> +   * we need to change the "lang" attribute.
> +   */

Sorry, I didn't realize before that you actually need this for Thunderbird.

But let's make this doubly clear.  Can you please hide the code below under #ifdef MOZ_THUNDERBIRD?  That way, it won't get built in Firefox and nobody will be able to accidentally depend on it.

Comment 56

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution: Complete removal of observer code from nsEditor (v3).

Fixed review issue. r+ as per comment #55.

Comment 57

[Jorg K (CEST = GMT+2)]

Dear Sheriff,

two patches here, which can be applied in any order. May I ask one favour to correct the checkin comment of one of the patches:

Proposed solution: Complete removal of observer code from nsEditor (v3).
Good:    Bug 1205983 - Remove all observer code from nsEditor. r=ehsan

Two updated tests and a new test. (v2, nits fixed)
Replace: Bug 1205983 - Remove all observer code (tests). r=ehsan
with:    Bug 1205983 - Remove all observer code from nsEditor (tests). r=ehsan

Thank you.

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a81630dba992
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c49c0ab84f

Comment 59

[Carsten Book [:Tomcat]]

sorry had to back this out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=14878310&repo=mozilla-inbound

Comment 60

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/009bf768c97c

Comment 61

[Jorg K (CEST = GMT+2)]

The try run on inbound is here:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d990673dc675
Windows 8 x64 debug (oth).

No idea how to address those leaks. Are they related to the code removal?

During my local testing I randomly got some failed leak check, but couldn't make much sense of it.

Comment 62

[Jorg K (CEST = GMT+2)]

Attachment: Leaks report

I ran my test locally:
mach mochitest editor/composer/test/test_bug1205983.html

Enclosed the result. When I ran that yesterday before refreshing my source today, I didn't see this. But I saw leak reports (more or less randomly, they came and went) last week.

Nice to look at line 197 of the enclosed file (quote):
WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME.
  FIX THIS!
Leaked URLs:
  chrome://layoutdebug/content/layoutdebug-overlay.xul
  chrome://browser/content/report-phishing-overlay.xul
  (followed by a long list).

Comment 63

[Jorg K (CEST = GMT+2)]

editor/composer/test/test_bug697981.html is a very similar test, the difference is, that 697981 uses a textarea, where 1205983 uses a contenteditable. test_bug697981.html doesn't seem to give any trouble.

Comment 64

[Jorg K (CEST = GMT+2)]

Attachment: Two updated tests and a new test. (v3)

Hmm, I added
// If we don't reset this, we cause massive leaks.
selcon_de = null;
editor_de = null;
at the end of the test. That seems to fix the problem.

Comment 65

[Jorg K (CEST = GMT+2)]

Easily visible in the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8666920&action=interdiff&newid=8667332&headers=1

Comment 66

[(no longer active)]

Please post a try server run that shows the leak has been fixed.  Thanks!

Comment 67

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=074e5bb3a579
This failed before: Windows 8 x64 debug (oth).

This will take ages to build/run. Can I stick an r+ onto it, if it turns out green in the morning?

Are you OK with the three lines I added:
  // If we don't reset this, we cause massive leaks.
  selcon_de = null;
  editor_de = null;

Do you have any idea why this is necessary? That's a JS bug, right? We shouldn't need to do this?

Comment 68

[(no longer active)]

Yes, you can check this in if it's green.

Comment 69

[Jorg K (CEST = GMT+2)]

Thanks. May I repeat my question(s):
Do you have any idea why this is necessary? That's a JS bug, right? We shouldn't need to do this?

Comment 70

[(no longer active)]

It's not a JS bug.  You're creating a cycle which involves the global window object which causes us to hold it all alive in memory, and setting these variables to null breaks the cycle, allowing us to free the memory.

Comment 71

[Jorg K (CEST = GMT+2)]

Dear Sheriff,
two patches here, apply in any order.
To the best of my knowledge, the memory leak is fixed now.

Comment 72

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03afd4febf0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7ec0b2d382

Comment 73

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/03afd4febf0b
https://hg.mozilla.org/mozilla-central/rev/ee7ec0b2d382

Comment 74

[Liz Henry (:lizzard) (relman/hg->git project)]

Ehsan, do you think this is risky to uplift to aurora?  I can't tell how intertwined this is with other related issues (such as bug 697981, which had to get backed out of aurora.)  

I'm leaning against uplifting this, but if you think it's ok, then I'd take it.

Comment 75

[Jorg K (CEST = GMT+2)]

Let me give you the complete picture: I cleaned up the spell checker in the following bugs:
bug 717433 (43), bug 1200533 (43), bug 697981 (44), bug 1193293 (44), bug 1204147 (43), bug 1204540 (44, test clean-up only), bug 1205983 (44), which landed on the Gecko versions indicated.

Bug 697981 was backed out, not because it broke anything, but because it only half-solved the problem. Ehsan decided to back it out rather than to wait for the full solution, which came in this bug here, bug 1205983.

So there is a possibility to uplift bug 697981 back to Aurora, where it was already, and then also uplift this bug here.

One could argue that the clean-up should be delivered in one hit and therefore do the uplift.
The area is now reasonably well covered by tests.

Risk-adverse people would argue that
- the spell check area has been broken since Firefox 9, so six weeks more or less
  are not so important.
- the worst bugs 717433 (43) and bug 1204147 (43) have already landed on 43.
- uplifting bug 697981 and bug 1205983 is quite some hassle, since they are intertwined with
  the other bugs landed on 44.
- the full solution should ride the trains.
So therefore there is no need for the uplift of bug 697981 and bug 1205983.

To save me the hassle, we could just forget about the uplift. I'm sure Ehsan agrees ;-)

Comment 76

[(no longer active)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #74)
> Ehsan, do you think this is risky to uplift to aurora?  I can't tell how
> intertwined this is with other related issues (such as bug 697981, which had
> to get backed out of aurora.)  

The whole point of backing out bug 697981 on Aurora was to not have to uplift another fix on top of it.

> I'm leaning against uplifting this, but if you think it's ok, then I'd take
> it.

I prefer if we let this and bug 697981 ride the trains as normal.

Comment 77

[Liz Henry (:lizzard) (relman/hg->git project)]

OK. Thanks for the clarifications.  
Wontfixing for 43 and tracking for 44 in case there are any problems that cause this to reopen.