Bug 25539 – spellcheck='' should be the same as spellcheck="true"

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ojan Vafai]

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)."

[Hajime Morrita]

Created attachment 71527 [details]
a repro

Attached a repro for someone's curiosity.

[Hajime Morrita]

Created attachment 71543 [details]
Patch

[Hajime Morrita]

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 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]

Created attachment 71705 [details]
Patch

[Hajime Morrita]

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)]

Attachment 71705 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4609068

[WebKit Review Bot]

Attachment 71705 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4704070

[Tony Chang]

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]

> 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]

Committed r70512: <http://trac.webkit.org/changeset/70512>

[Hajime Morrita]

Reverted (Bug 48314.)

[Hajime Morrita]

Created attachment 71858 [details]
Patch

[Hajime Morrita]

> Created an attachment (id=71858) [details]
> Patch
Last patch missed a null check on Editor::isSpellCheckingEnabledInFocusedNode().
Added it, tested.

[Tony Chang]

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]

> 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]

Committed r70598: <http://trac.webkit.org/changeset/70598>