Remove spelling markers when element is not editable

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2004-2008, 2013, 2014 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2011 Motorola Mobility. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 28 matching lines @@
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/NodeTraversal.h"
 #include "core/dom/StyleChangeReason.h"
 #include "core/dom/Text.h"
 #include "core/dom/shadow/ElementShadow.h"
 #include "core/dom/shadow/FlatTreeTraversal.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/editing/EditingUtilities.h"
 #include "core/editing/serializers/Serialization.h"
+#include "core/editing/spellcheck/SpellChecker.h"
 #include "core/events/EventListener.h"
 #include "core/events/KeyboardEvent.h"
 #include "core/frame/Settings.h"
 #include "core/frame/UseCounter.h"
 #include "core/html/HTMLBRElement.h"
 #include "core/html/HTMLDimension.h"
 #include "core/html/HTMLFormElement.h"
 #include "core/html/HTMLInputElement.h"
 #include "core/html/HTMLMenuElement.h"
 #include "core/html/HTMLTemplateElement.h"
@@ skipping 370 matching lines @@
 void HTMLElement::attributeChanged(const AttributeModificationParams& params) {
   Element::attributeChanged(params);
   if (params.reason != AttributeModificationReason::kDirectly)
     return;
   // adjustedFocusedElementInTreeScope() is not trivial. We should check
   // attribute names, then call adjustedFocusedElementInTreeScope().
   if (params.name == hiddenAttr && !params.newValue.isNull()) {
     if (adjustedFocusedElementInTreeScope() == this)
       blur();
   } else if (params.name == contenteditableAttr) {
+    if (params.newValue.isNull() || equalIgnoringCase(params.newValue, "false"))
+      document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this);

[yosin_UTC9, 2017/01/25 02:31:41]

Do we really need to remove markers synchronously?
I think we can do it in idle time since spelling markers are set in
asynchronously.
More specifically, we can remove markers in idle time like idle time spell
checker[1],
e.g. IdleTimeSpellingMarkerCallback to handle both add and removal of markers.

WDYT?

[1] http://crrev.com/2578493002 Add a skeleton of idle time spell checker

[Xiaocheng, 2017/01/25 02:56:57]

I think synchronous marker removal is fine, as we are already doing that for
<input> on blur. However, there are some tricky cases, for example:

<div contenteditable="false">
  <span contenteditable>misspelllling.</span>
</div>

When removing contenteditable from <div>, this patch incorrectly removes the
marker in <span>.

[Manuel Rego, 2017/01/25 08:03:10]

Good catch! Probably I should use isEditable() to remove or not the markers.

     if (adjustedFocusedElementInTreeScope() != this)
       return;
     // The attribute change may cause supportsFocus() to return false
     // for the element which had focus.
     //
     // TODO(tkent): We should avoid updating style.  We'd like to check only
     // DOM-level focusability here.
     document().updateStyleAndLayoutTreeForNode(this);
     if (!supportsFocus())
       blur();
   }
 }
 
 void HTMLElement::parseAttribute(const AttributeModificationParams& params) {
   if (params.name == tabindexAttr || params.name == XMLNames::langAttr)
     return Element::parseAttribute(params);
 
   if (params.name == dirAttr) {
     dirAttributeChanged(params.newValue);
   } else if (params.name == langAttr) {
     pseudoStateChanged(CSSSelector::PseudoLang);
+  } else if (params.name == contenteditableAttr) {
+    if (params.newValue.isNull() ||
+        equalIgnoringCase(params.newValue, "false")) {
+      // document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this);

[yosin_UTC9, 2017/01/25 02:31:41]

Could you remove this comment?

[Manuel Rego, 2017/01/25 08:03:10]

Sure this is a leftover from a previous version of the patch,
this method parseAttribute() shouldn't be modified at all.
Sorry about that.

+    }
   } else {
     const AtomicString& eventName = eventNameForAttributeName(params.name);
     if (!eventName.isNull()) {
       setAttributeEventListener(
           eventName,
           createAttributeEventListener(this, params.name, params.newValue,
                                        eventParameterName()));
     }
   }
 }
@@ skipping 745 matching lines @@
 
 #ifndef NDEBUG
 
 // For use in the debugger
 void dumpInnerHTML(blink::HTMLElement*);
 
 void dumpInnerHTML(blink::HTMLElement* element) {
   printf("%s\n", element->innerHTML().ascii().data());
 }
 #endif