Remove spelling markers in disabled and readonly form elements

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 19 matching lines @@
 #include "core/dom/Document.h"
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/Text.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/editing/EditingUtilities.h"
 #include "core/editing/Editor.h"
 #include "core/editing/FrameSelection.h"
 #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"
 #include "core/html/HTMLBRElement.h"
 #include "core/html/parser/HTMLParserIdioms.h"
 #include "core/html/shadow/ShadowElementNames.h"
 #include "core/layout/LayoutBlock.h"
 #include "core/layout/LayoutBlockFlow.h"
 #include "core/layout/LayoutTheme.h"
 #include "core/page/FocusController.h"
@@ skipping 656 matching lines @@
   if (params.name == autocapitalizeAttr)
     UseCounter::count(document(), UseCounter::AutocapitalizeAttribute);
 
   if (params.name == placeholderAttr) {
     updatePlaceholderText();
     updatePlaceholderVisibility();
     UseCounter::count(document(), UseCounter::PlaceholderAttribute);
   } 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 {
   if (!isTextControl())
     return false;
   return m_lastChangeWasUserEdit;
 }
 
 Node* TextControlElement::createPlaceholderBreakElement() const {
   return HTMLBRElement::create(document());
@@ skipping 175 matching lines @@
 
 void TextControlElement::copyNonAttributePropertiesFromElement(
     const Element& source) {
   const TextControlElement& sourceElement =
       static_cast<const TextControlElement&>(source);
   m_lastChangeWasUserEdit = sourceElement.m_lastChangeWasUserEdit;
   HTMLFormControlElement::copyNonAttributePropertiesFromElement(source);
 }
 
 }  // namespace blink