http://www.whatwg.org/specs/web-apps/current-work/#attr-spellcheck "The spellcheck attribute is an enumerated attribute whose keywords are the empty string, true and false. The empty string and the true keyword map to the true state. The false keyword maps to the false state. In addition, there is a third state, the inherit state, which is the missing value default (and the invalid value default)."
Created attachment 71527 [details] a repro Attached a repro for someone's curiosity.
Created attachment 71543 [details] Patch
The patch might apeear overly crafted, but it is also a starting point for Bug 25536 so I think it makes sense.
Comment on attachment 71543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71543&action=review > WebCore/dom/Element.h:327 > + SpellcheckAttributeState spellcheckAttributeState() const; Can this method be private? > WebCore/dom/Element.h:328 > + bool spellCheckingEnabled() const; I would name this isSpellCheckingEnabled to match similar methods. > WebCore/editing/Editor.cpp:2551 > + const Element* focusedElement = node->isElementNode() ? toElement(node) : node->parentElement(); Is it possible that node->parentElement() is null, but node->parent()->parentElement() is true?
Created attachment 71705 [details] Patch
Hi Tony, Thank you for taking a look! I updated the patch. (In reply to comment #4) > (From update of attachment 71543 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71543&action=review > > > WebCore/dom/Element.h:327 > > + SpellcheckAttributeState spellcheckAttributeState() const; > > Can this method be private? Fixed. > > > WebCore/dom/Element.h:328 > > + bool spellCheckingEnabled() const; > > I would name this isSpellCheckingEnabled to match similar methods. Renamed. I also renamed Editor::spellCheckingEnabledInFocusedNode as well. > > > WebCore/editing/Editor.cpp:2551 > > + const Element* focusedElement = node->isElementNode() ? toElement(node) : node->parentElement(); > > Is it possible that node->parentElement() is null, but node->parent()->parentElement() is true? I'm sorry but I could not get the point. My intention here is to ensure that we check against Element class because node might be Text. And Text might be orphaned.
Attachment 71705 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4609068
Attachment 71705 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4704070
Comment on attachment 71705 [details] Patch The cr-linux compile error looks real. There are 2 calls to spellCheckingEnabledInFocusedNode() in WebKit/chromium. Fix those, and r=me. > > WebCore/editing/Editor.cpp:2551 > > + const Element* focusedElement = node->isElementNode() ? toElement(node) : node->parentElement(); > > Is it possible that node->parentElement() is null, but node->parent()->parentElement() is true? I'm sorry but I could not get the point. My intention here is to ensure that we check against Element class because node might be Text. And Text might be orphaned. The previous code looped over node->parent(), but the new code uses node->parentElement(). I was wondering if we were missing cases if parent() is not an element, but parent()->parentElement() is an element. I don't think that's possible, so this seems fine.
> The previous code looped over node->parent(), but the new code uses node->parentElement(). I was wondering if we were missing cases if parent() is not an element, but parent()->parentElement() is an element. I don't think that's possible, so this seems fine. I got it! Thank you for explanation. According to Node.cpp, The XML spec allows EntityReference node but WebKit doesn't support it. I'll land this after fixing chromium build break.
Committed r70512: <http://trac.webkit.org/changeset/70512>
Reverted (Bug 48314.)
Created attachment 71858 [details] Patch
> Created an attachment (id=71858) [details] > Patch Last patch missed a null check on Editor::isSpellCheckingEnabledInFocusedNode(). Added it, tested.
Comment on attachment 71858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71858&action=review Is it possible to add a layout test for the !node crash? > WebCore/editing/Editor.cpp:2552 > + if (!node) > + return false; Note that the previous code returned true if !node, but it doesn't seem to matter.
> Is it possible to add a layout test for the !node crash? This did crash some existing tests, that is why the patch was rolled out...
Committed r70598: <http://trac.webkit.org/changeset/70598>