Bug 25539 - spellcheck='' should be the same as spellcheck="true"
Summary: spellcheck='' should be the same as spellcheck="true"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 14552 48314
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-04 00:07 PDT by Ojan Vafai
Modified: 2010-10-26 18:57 PDT (History)
4 users (show)

See Also:


Attachments
a repro (1.45 KB, text/html)
2010-10-21 22:49 PDT, Hajime Morrita
no flags Details
Patch (7.62 KB, patch)
2010-10-22 03:28 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (10.56 KB, patch)
2010-10-24 19:06 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2010-10-26 03:44 PDT, Hajime Morrita
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 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)."
Comment 1 Hajime Morrita 2010-10-21 22:49:26 PDT
Created attachment 71527 [details]
a repro

Attached a repro for someone's curiosity.
Comment 2 Hajime Morrita 2010-10-22 03:28:11 PDT
Created attachment 71543 [details]
Patch
Comment 3 Hajime Morrita 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.
Comment 4 Tony Chang 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?
Comment 5 Hajime Morrita 2010-10-24 19:06:27 PDT
Created attachment 71705 [details]
Patch
Comment 6 Hajime Morrita 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.
Comment 7 Eric Seidel (no email) 2010-10-24 20:32:30 PDT
Attachment 71705 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4609068
Comment 8 WebKit Review Bot 2010-10-25 05:28:41 PDT
Attachment 71705 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4704070
Comment 9 Tony Chang 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.
Comment 10 Hajime Morrita 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.
Comment 11 Hajime Morrita 2010-10-25 21:45:54 PDT
Committed r70512: <http://trac.webkit.org/changeset/70512>
Comment 12 Hajime Morrita 2010-10-26 01:30:30 PDT
Reverted (Bug 48314.)
Comment 13 Hajime Morrita 2010-10-26 03:44:37 PDT
Created attachment 71858 [details]
Patch
Comment 14 Hajime Morrita 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.
Comment 15 Tony Chang 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.
Comment 16 Hajime Morrita 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...
Comment 17 Hajime Morrita 2010-10-26 18:57:12 PDT
Committed r70598: <http://trac.webkit.org/changeset/70598>