Don't init spell checker for spellcheck=false contentEditable

RESOLVED FIXED in Firefox 55

Status

()

Core
Editor
P1
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: cpearce, Assigned: m_kato)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

5 months ago
Profiling Google Docs we discovered that we're initing the spell checker for Google Docs, even though Google use their own spell checker. This causes a sync IPC call to SendSetDictionary, which can take between 20~50ms on my high end machine.

For example, this profile, on the test case for (bug 1326349):
https://clptr.io/2jGYcyj

Potentially this can happen every time we switch the dictionary language, which I'm told an happen multiple times at runtime.

We can prevent us loading the dict by checking whether the focus is spellcheck=false in EditorBase::OnFocus().

http://searchfox.org/mozilla-central/rev/a712d69adb9b2588f88aff678216b2be94d3719c/editor/libeditor/EditorBase.cpp#5148
(Reporter)

Updated

5 months ago
Summary: Don't init spell checker for non-editable content → Don't init spell checker for spellcheck=false contentEditable
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8827304 - Flags: review?(m_kato) → review?(masayuki)

Comment 2

5 months ago
mozreview-review
Comment on attachment 8827304 [details]
Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content.

https://reviewboard.mozilla.org/r/105018/#review105856

::: editor/libeditor/EditorBase.cpp:108
(Diff revision 1)
>  #include "nsStyleStructFwd.h"           // for nsIFrame::StyleUIReset, etc.
>  #include "nsTextNode.h"                 // for nsTextNode
>  #include "nsThreadUtils.h"              // for nsRunnable
>  #include "nsTransactionManager.h"       // for nsTransactionManager
>  #include "prtime.h"                     // for PR_Now
> +#include "nsGenericHTMLElement.h"

Please keep "A-Z" order of the included header files. And add |// for nsGenericHTMLElement to its tail.

::: editor/libeditor/EditorBase.cpp:5148
(Diff revision 1)
>  
>  void
>  EditorBase::OnFocus(nsIDOMEventTarget* aFocusEventTarget)
>  {
>    InitializeSelection(aFocusEventTarget);
> +  nsCOMPtr<nsIContent> node(do_QueryInterface(aFocusEventTarget));

nit: use nsCOMPtr<nsIContent> node = do_QueryInterface(...); style, see the coding rule:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

And also I like "content" rather than "node" because it's a smart pointer for nsIContent, not nsINode.

::: editor/libeditor/EditorBase.cpp:5150
(Diff revision 1)
> +      (node->IsHTMLElement() &&
> +       !static_cast<nsGenericHTMLElement*>(node.get())->Spellcheck())) {

I think that this is wrong. When it's in design mode, aFocusEventTarget is probably the document node.

I guess that you need to use GetEditorRoot()? But if you need <input> or <textarea> when it's a plaintext editor, you need to use GetExposedRoot()? Although, the latter isn't implemented in HTMLEditor.
Attachment #8827304 - Flags: review?(masayuki) → review-
Blocks: 1331674
(Reporter)

Comment 3

5 months ago
mozreview-review
Comment on attachment 8827304 [details]
Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content.

https://reviewboard.mozilla.org/r/105018/#review111372

::: editor/libeditor/EditorBase.cpp:5150
(Diff revision 1)
> +      (node->IsHTMLElement() &&
> +       !static_cast<nsGenericHTMLElement*>(node.get())->Spellcheck())) {

You're right. I tested 3 cases; contentEditable, designMode, and textArea. The only thing that works in all three is GetExposedRoot().

EditorBase implements GetExposedRoot(), and so since HTMLEditor extends from that, do we not need to worry about HTMLEditor not directly implementing GetExposedRoot() itself?
(Reporter)

Comment 4

5 months ago
mozreview-review-reply
Comment on attachment 8827304 [details]
Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content.

https://reviewboard.mozilla.org/r/105018/#review105856

> I think that this is wrong. When it's in design mode, aFocusEventTarget is probably the document node.
> 
> I guess that you need to use GetEditorRoot()? But if you need <input> or <textarea> when it's a plaintext editor, you need to use GetExposedRoot()? Although, the latter isn't implemented in HTMLEditor.

You're right. I tested 3 cases; contentEditable, designMode, and textArea. The only thing that works in all three is GetExposedRoot().

EditorBase implements GetExposedRoot(), and so since HTMLEditor extends from that, do we not need to worry about HTMLEditor not directly implementing GetExposedRoot() itself?
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8827304 - Flags: review?(m_kato)
(Assignee)

Updated

5 months ago
Attachment #8827304 - Flags: review?(masayuki)
(Assignee)

Comment 6

5 months ago
(why does changing reviewer clear review flag...)

Comment 7

5 months ago
mozreview-review
Comment on attachment 8827304 [details]
Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content.

https://reviewboard.mozilla.org/r/105018/#review111426
Attachment #8827304 - Flags: review?(masayuki) → review+
Chris, did you mean to land this patch?  :-)
Blocks: 1342713
(Reporter)

Comment 9

4 months ago
There are test failures on my try push, and I've not gotten around to debugging them yet.
(Reporter)

Comment 10

4 months ago
The failing Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2acf014d27cd

The failures are caused by nsGenericHTMLElement::SpellCheck() not assuming that the lack of the "spellcheck" attribute on the root implies that spell checking is not disabled.

I'm not sure if nsGenericHTMLElement::SpellCheck() should or shouldn't assume that.

Irrespective of that, if I change my patch to use GetExposedRoot() in the PlainTextEditor case and GetEditorRoot() otherwise, as Masayuki suggested was necessary, things work as expected. So my previous claim that GetExposedRoot() works in all cases was false; Masayuki correctly pointed out that in some cases we want to use GetExposedRoot() and other we want to use GetEditorRoot().

Try push with fixed patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e8dba747d6023d3e6c3867347f848e2069c176d
Comment hidden (mozreview-request)
Whiteboard: [qf:p1]
(Reporter)

Comment 12

4 months ago
Comment on attachment 8827304 [details]
Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content.

Masayuki: Can you please re-review this patch here. MozReview seems to have lost the request when I re-uploaded this patch after fixing the mochitests. Thanks.
Attachment #8827304 - Flags: review+ → review?(masayuki)

Comment 13

4 months ago
mozreview-review
Comment on attachment 8827304 [details]
Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content.

https://reviewboard.mozilla.org/r/105018/#review120220

::: editor/libeditor/EditorBase.cpp:5192
(Diff revision 3)
>  void
>  EditorBase::OnFocus(nsIDOMEventTarget* aFocusEventTarget)
>  {
>    InitializeSelection(aFocusEventTarget);
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aFocusEventTarget);
> +  Element* root = IsPlaintextEditor() ? GetExposedRoot() : GetEditorRoot();

You could misunderstand about IsPlaintextEditor(). Even if it's a contenteditable editor, this may return true if addon sets plaintext editor flag. If we'd implement contenteditable="plaintext-only", this could be true.

I think that GetExposedRoot() should be a virtual method and overridden by HTMLEditor.  Then, it should return GetEditorRoot() from it.

On the other hand, in designMode, what this should behave? In designMode, HTMLEditor::GetEditorRoot() returns <body> element (if there is).

So, specifying spellcheck="false" to <body> should work even in designMode? But <html spellcheck="false"> should be ignored?
Attachment #8827304 - Flags: review?(masayuki) → review-
See Also: → bug 1350636
Status: NEW → ASSIGNED
(Assignee)

Comment 14

3 months ago
:cpearce, do you have a time to fix this after masayuki's review?  If no, I will take this.
Flags: needinfo?(cpearce)
(Reporter)

Comment 15

3 months ago
Feel free to take it.
Flags: needinfo?(cpearce)
(Assignee)

Updated

3 months ago
Assignee: cpearce → m_kato
(Assignee)

Comment 16

3 months ago
Previous fix won't consider the following case.  So GetEditorRoot isn't better for this situation.

data:text/html,<div contenteditable=true spellcheck=false>ABC<br><div spellcheck=true>DEF<br></div></div>
(In reply to Makoto Kato [:m_kato] from comment #16)
> Previous fix won't consider the following case.  So GetEditorRoot isn't
> better for this situation.
> 
> data:text/html,<div contenteditable=true spellcheck=false>ABC<br><div
> spellcheck=true>DEF<br></div></div>

hmm, indeed, the spec allows such complicated behavior...
https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-checking

Is it possible to control spellchecker working in specific nodes? Even if it's possible, it sounds not good for performance. What other browsers do?

# Anyway, it's worthwhile not to load spellchecker dictionaries when moving focus to an editor which active editing host disables the spellchecker. If user wants to do it, should be able to do it forcibly from context menu.
(Assignee)

Comment 18

3 months ago
(In reply to Masayuki Nakano [:masayuki] from comment #17)

> Is it possible to control spellchecker working in specific nodes? Even if
> it's possible, it sounds not good for performance.

It will causes performance issue.  When moving caret, we might have to load dictionary...

>  What other browsers do?

At latest, Chrome seems to use browser's setting, not lang attribute.  Also, mixed spellcheck case is same behavior as Firefox.

I would not like to traverse all nodes into contenteditable due to performance, so we should change SetDictionary to async if possible.  If <input> or <textarea>, we might be able to check SpellCheck() easily.
(Assignee)

Comment 19

3 months ago
(but after adding spellcheck attribute by DOM, we might have to load it on focus even if <input> with spellcheck=false)
(In reply to Makoto Kato [:m_kato] from comment #18)
> (In reply to Masayuki Nakano [:masayuki] from comment #17)
> >  What other browsers do?
> 
> At latest, Chrome seems to use browser's setting, not lang attribute.  Also,
> mixed spellcheck case is same behavior as Firefox.
> 
> I would not like to traverse all nodes into contenteditable due to
> performance, so we should change SetDictionary to async if possible.  If
> <input> or <textarea>, we might be able to check SpellCheck() easily.

Sound like that we should file an issue to WHATWG. I think that the attribute of any elements in active editing host should be ignored for performance.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

3 months ago
mozreview-review
Comment on attachment 8856399 [details]
Bug 1330912 - Part 2. Use async version on UpdateCurrentDictionary.

https://reviewboard.mozilla.org/r/128334/#review130850

::: editor/composer/nsEditorSpellCheck.cpp:753
(Diff revision 2)
>        // dictionary from the list. This must work, if it doesn't, there is
>        // no point trying another one.
> -      break;
> +      return;
>      }
>    }
> -  return rv;
> +  return;

nit: unnecessary return.

::: editor/composer/nsEditorSpellCheck.cpp:867
(Diff revision 2)
> +nsEditorSpellCheck::SetFallbackDictionary(DictionaryFetcher* aFetcher)
> +{
> +  MOZ_ASSERT(mUpdateDictionaryRunning);
> +
> +  AutoTArray<nsString, 6> tryDictList;
> +
> +  // We obtain a list of available dictionaries.
> +  nsTArray<nsString> dictList;

Cannot use AutoTArray here? (If so, it's okay.)

::: editor/composer/nsEditorSpellCheck.cpp:892
(Diff revision 2)
>    nsAutoString preferredDict;
>    preferredDict = Preferences::GetLocalizedString("spellchecker.dictionary");

Although, out of scope of this bug, if this method is called a lot, this might bad for performance. This might cache the value or observe the change of the value, but I'm not sure if it's possible due to localized string pref...

::: editor/composer/nsEditorSpellCheck.cpp:1013
(Diff revision 2)
> +  mSpellChecker->SetCurrentDictionaryFromList(tryDictList)->Then(
> +    AbstractThread::MainThread(),
> +    __func__,
> +    [self, fetcher]() {
> -  // If an error was thrown while setting the dictionary, just
> +      // If an error was thrown while setting the dictionary, just
> -  // fail silently so that the spellchecker dialog is allowed to come
> +      // fail silently so that the spellchecker dialog is allowed to come
> -  // up. The user can manually reset the language to their choice on
> +      // up. The user can manually reset the language to their choice on
> -  // the dialog if it is wrong.
> +      // the dialog if it is wrong.
> -
> -  DeleteSuggestedWordList();
> -
> -  return NS_OK;
> +      self->DeleteSuggestedWordList();
> +      self->EndUpdateDictionary();
> +      fetcher->mCallback->EditorSpellCheckDone();
> +    });

Although, I'm not sure, at reject, needs to delete the suggested words.

And anyway, don't we need to call EndUpdateDictionary in such case?
Attachment #8856399 - Flags: review?(masayuki) → review+

Comment 30

3 months ago
mozreview-review
Comment on attachment 8856400 [details]
Bug 1330912 - Part 3. Don't update dictionary during onfocus when spellcheck is unnecessary.

https://reviewboard.mozilla.org/r/128336/#review130864

::: editor/libeditor/EditorBase.h:1086
(Diff revision 2)
>    // True while the instance is handling an edit action.
>    bool mIsInEditAction;
>    // Whether caret is hidden forcibly.
>    bool mHidingCaret;
> +  // Whether spellchecker dictionary is initialized after focused.
> +  bool mUpdatedSpellCheckerDictionary;

Hmm, I like |mUpdateSpellCheckerDictionary| (reverted the meaning) or |mSpellCheckerDictionaryUpdated| better.

::: editor/libeditor/EditorBase.cpp:1352
(Diff revision 2)
>    if (mInlineSpellChecker) {
> +    if (!mUpdatedSpellCheckerDictionary && enable) {
> +      mInlineSpellChecker->UpdateCurrentDictionary();
> +      mUpdatedSpellCheckerDictionary = true;
> +    }
> +    

nit: remove the spaces of this line.
Attachment #8856400 - Flags: review?(masayuki) → review+

Comment 31

3 months ago
mozreview-review
Comment on attachment 8856401 [details]
Bug 1330912 - Part 4. Update some tests to wait updating dictionary.

https://reviewboard.mozilla.org/r/128338/#review130866
Attachment #8856401 - Flags: review?(masayuki) → review+
Comment on attachment 8856398 [details]
Bug 1330912 - Part 1. Add async version of SetCurrentDictionary using list.

https://reviewboard.mozilla.org/r/128332/#review131196

Nice, thank you!

Will we still be using the sync messages after these patches land?  If no, can you please file a follow-up bug to remove them?  Thanks!
Attachment #8856398 - Flags: review?(ehsan) → review+
(Assignee)

Comment 33

2 months ago
mozreview-review
Comment on attachment 8856399 [details]
Bug 1330912 - Part 2. Use async version on UpdateCurrentDictionary.

https://reviewboard.mozilla.org/r/128334/#review131246

::: editor/composer/nsEditorSpellCheck.cpp:1013
(Diff revision 2)
> +  mSpellChecker->SetCurrentDictionaryFromList(tryDictList)->Then(
> +    AbstractThread::MainThread(),
> +    __func__,
> +    [self, fetcher]() {
> -  // If an error was thrown while setting the dictionary, just
> +      // If an error was thrown while setting the dictionary, just
> -  // fail silently so that the spellchecker dialog is allowed to come
> +      // fail silently so that the spellchecker dialog is allowed to come
> -  // up. The user can manually reset the language to their choice on
> +      // up. The user can manually reset the language to their choice on
> -  // the dialog if it is wrong.
> +      // the dialog if it is wrong.
> -
> -  DeleteSuggestedWordList();
> -
> -  return NS_OK;
> +      self->DeleteSuggestedWordList();
> +      self->EndUpdateDictionary();
> +      fetcher->mCallback->EditorSpellCheckDone();
> +    });

This uses ResoveRejectMethodType, so this lamda is be called by both case.

Comment 34

2 months ago
mozreview-review
Comment on attachment 8856399 [details]
Bug 1330912 - Part 2. Use async version on UpdateCurrentDictionary.

https://reviewboard.mozilla.org/r/128334/#review131256

::: editor/composer/nsEditorSpellCheck.cpp:1013
(Diff revision 2)
> +  mSpellChecker->SetCurrentDictionaryFromList(tryDictList)->Then(
> +    AbstractThread::MainThread(),
> +    __func__,
> +    [self, fetcher]() {
> -  // If an error was thrown while setting the dictionary, just
> +      // If an error was thrown while setting the dictionary, just
> -  // fail silently so that the spellchecker dialog is allowed to come
> +      // fail silently so that the spellchecker dialog is allowed to come
> -  // up. The user can manually reset the language to their choice on
> +      // up. The user can manually reset the language to their choice on
> -  // the dialog if it is wrong.
> +      // the dialog if it is wrong.
> -
> -  DeleteSuggestedWordList();
> -
> -  return NS_OK;
> +      self->DeleteSuggestedWordList();
> +      self->EndUpdateDictionary();
> +      fetcher->mCallback->EditorSpellCheckDone();
> +    });

Hmm, okay, but it's very bad thing that we cannot know the fact from the caller...
(Assignee)

Comment 35

2 months ago
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #32)
> Comment on attachment 8856398 [details]
> Bug 1330912 - Part 1. Add async version of SetCurrentDictionary using list.
> 
> https://reviewboard.mozilla.org/r/128332/#review131196
> 
> Nice, thank you!
> 
> Will we still be using the sync messages after these patches land?  If no,
> can you please file a follow-up bug to remove them?  Thanks!

A lot of our tests use sync version.  And, context menu for selecting dictionary still uses it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

2 months ago
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/3facdd7bcabd
Part 1. Add async version of SetCurrentDictionary using list. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/d813879551cb
Part 2. Use async version on UpdateCurrentDictionary. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/ebdf1fcdd758
Part 3. Don't update dictionary during onfocus when spellcheck is unnecessary. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/665564221f54
Part 4. Update some tests to wait updating dictionary. r=masayuki

Comment 43

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3facdd7bcabd
https://hg.mozilla.org/mozilla-central/rev/d813879551cb
https://hg.mozilla.org/mozilla-central/rev/ebdf1fcdd758
https://hg.mozilla.org/mozilla-central/rev/665564221f54
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a month ago
Depends on: 1365383
You need to log in before you can comment on or make changes to this bug.