Remove spelling markers in disabled and readonly form elements

third_party/WebKit/Source/core/html/TextControlElement.cpp

Index: third_party/WebKit/Source/core/html/TextControlElement.cpp
diff --git a/third_party/WebKit/Source/core/html/TextControlElement.cpp b/third_party/WebKit/Source/core/html/TextControlElement.cpp
index 1286574aecb2df48cb31c388a30bacdcc8aeb0c0..b85f8e8a06f9744768c1866439e2ae105c4ad529 100644
--- a/third_party/WebKit/Source/core/html/TextControlElement.cpp
+++ b/third_party/WebKit/Source/core/html/TextControlElement.cpp
@@ -37,6 +37,7 @@
 #include "core/editing/iterators/CharacterIterator.h"
 #include "core/editing/iterators/TextIterator.h"
 #include "core/editing/serializers/Serialization.h"
+#include "core/editing/spellcheck/SpellChecker.h"
 #include "core/events/Event.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/UseCounter.h"
@@ -713,6 +714,13 @@ void TextControlElement::parseAttribute(
   } else {
     HTMLFormControlElementWithState::parseAttribute(params);
   }
+
+  if ((params.name == disabledAttr || params.name == readonlyAttr) &&
+      (params.oldValue.isNull() != params.newValue.isNull()) &&
+      isTextControl() && isDisabledOrReadOnly() && document().frame()) {
+    setNeedsWillValidateCheck();

[tkent, 2017/02/07 14:17:06]

Why do we need setNeedsWillValidateCheck()?

[Manuel Rego, 2017/02/07 15:06:03]

Now that we use parseAttribute() I need to call setNeedsWillValidateCheck()
or I'll get a crash: http://sprunge.us/dQSE
It seems to be triggered by the call to
updateStyleAndLayoutTreeForNode() in removeSpellingAndGrammarMarkers().

If I use attributeChanged() I don't need.
So I'm not sure if it's better to come back to the previous approach
and override attributeChanged().

[yosin_UTC9, 2017/02/08 02:07:44]

It seems we should refactor removeSpellingAndGrammarMarkers().

There are two call sites of removeSpellingAndGrammarMarkers()
 1. HTMLElement::attributeChanged() calls removeXXX() with
ElementsType::kOnlyNonEditable
 2. SpellChecker::didEndEditingOnTextField(): calls removeXXX() with
ElementsType::kAll

If we change HTMLElement::attributeChanged() to call removeXXX() only when
contenteditable="false",
we don't need to have ElementType parameter in removeXXX() and we don't need to
call hasEditableStyle() and updateStyleAndLayoutXXX()
in removeSpellingAndGrammarMarkers().


Could you make this change in another CL for a preparation of this patch?

[tkent, 2017/02/08 02:12:14]

Thanks.  I understand.  We handle |disabled| attribute in
HTMLFormControlElement::attributeChanged, not parseAttribute().
With this patch, style is invalidated twice if disabled attribute is changed.
It's not ideal.
I'd like:
 * TextControlElement overrides disabledAttributeChanged() like Patch Set 1, and
 * TextControlElement::parseAttribute() handles readonlyAttr.

[Manuel Rego, 2017/02/08 09:00:34]

We still need to use hasEditableStyle() in removeSpellingAndGrammarMarkers().
For example if you have:
  <div id="parent" contenteditable>
    foo
    <span id="child" contenteditable>test</span>
    bar
  </div>

If you mark "parent" as "contenteditable=false",
you have to remove the spell markers from "foo" and "bar",
but not inside "child" as it's still editable.
That was the reason why we added the ElementType parameter in
https://codereview.chromium.org/2650183002/

+    document().frame()->spellChecker().didEndEditingOnTextField(this);
+  }
 }
 
 bool TextControlElement::lastChangeWasUserEdit() const {