RESOLVED FIXED 25539
spellcheck='' should be the same as spellcheck="true"
https://bugs.webkit.org/show_bug.cgi?id=25539
Summary spellcheck='' should be the same as spellcheck="true"
Ojan Vafai
Reported 2009-05-04 00:07:02 PDT
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)."
Attachments
a repro (1.45 KB, text/html)
2010-10-21 22:49 PDT, Hajime Morrita
no flags
Patch (7.62 KB, patch)
2010-10-22 03:28 PDT, Hajime Morrita
no flags
Patch (10.56 KB, patch)
2010-10-24 19:06 PDT, Hajime Morrita
no flags
Patch (12.84 KB, patch)
2010-10-26 03:44 PDT, Hajime Morrita
tony: review+
Hajime Morrita
Comment 1 2010-10-21 22:49:26 PDT
Created attachment 71527 [details] a repro Attached a repro for someone's curiosity.
Hajime Morrita
Comment 2 2010-10-22 03:28:11 PDT
Hajime Morrita
Comment 3 2010-10-22 03:58:01 PDT
The patch might apeear overly crafted, but it is also a starting point for Bug 25536 so I think it makes sense.
Tony Chang
Comment 4 2010-10-22 10:17:25 PDT
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?
Hajime Morrita
Comment 5 2010-10-24 19:06:27 PDT
Hajime Morrita
Comment 6 2010-10-24 20:15:33 PDT
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.
Eric Seidel (no email)
Comment 7 2010-10-24 20:32:30 PDT
WebKit Review Bot
Comment 8 2010-10-25 05:28:41 PDT
Tony Chang
Comment 9 2010-10-25 10:34:05 PDT
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.
Hajime Morrita
Comment 10 2010-10-25 18:39:23 PDT
> 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.
Hajime Morrita
Comment 11 2010-10-25 21:45:54 PDT
Hajime Morrita
Comment 12 2010-10-26 01:30:30 PDT
Reverted (Bug 48314.)
Hajime Morrita
Comment 13 2010-10-26 03:44:37 PDT
Hajime Morrita
Comment 14 2010-10-26 03:46:10 PDT
> Created an attachment (id=71858) [details] > Patch Last patch missed a null check on Editor::isSpellCheckingEnabledInFocusedNode(). Added it, tested.
Tony Chang
Comment 15 2010-10-26 09:19:35 PDT
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.
Hajime Morrita
Comment 16 2010-10-26 18:48:48 PDT
> 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...
Hajime Morrita
Comment 17 2010-10-26 18:57:12 PDT
Note You need to log in before you can comment on or make changes to this bug.