Bug 25536 – WebKit needs to expose an Element::spellcheck attribute to javascript

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric Seidel (no email)]

WebKit needs to expose an Element::spellcheck attribute to javascript

Per HTML5:
http://www.whatwg.org/specs/web-apps/current-work/#attr-spellcheck

[Hajime Morrita]

Created attachment 72002 [details]
Patch

[Hajime Morrita]

Dynamic spellcheck behavior change is not implemented at the patch. 
It is filed separately on Bug 48411.

[Kent Tamura]

Comment on attachment 72002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72002&action=review

> LayoutTests/editing/spelling/script-tests/spelling-attribute-change.js:25
> +    shouldBe("layoutTestController.hasSpellingMarker(6, 2)", enabled ? "true" : "false");

Some ports has no hasSpellingMarker() implementation, right? We need to skip the test on them.

> WebCore/html/HTMLElement.cpp:735
> +    setAttribute(HTMLNames::spellcheckAttr, enable ? "true" : "false");

Need no HTMLNames::.

[Sam Weinig]

Comment on attachment 72002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72002&action=review

> WebCore/html/HTMLElement.idl:65
> +                 attribute boolean spellcheck;

This should probably use the [Reflect] extended attribute which would mean you don't need the implementation of spellcheck/setSpellcheck in HTMLElement.h/cpp

[Hajime Morrita]

Created attachment 72154 [details]
Patch

[Hajime Morrita]

Created attachment 72158 [details]
Patch

[Hajime Morrita]

Hi, Kent-san, Sam, thank you for taking a look!
I updated the patch.

> > LayoutTests/editing/spelling/script-tests/spelling-attribute-change.js:25
> > +    shouldBe("layoutTestController.hasSpellingMarker(6, 2)", enabled ? "true" : "false");
> 
> Some ports has no hasSpellingMarker() implementation, right? We need to skip the test on them.
Yes, only Mac, windows and chromium-mac support spellchecking.
Added skip entries for other ports.

> 
> > WebCore/html/HTMLElement.cpp:735
> > +    setAttribute(HTMLNames::spellcheckAttr, enable ? "true" : "false");
> 
> Need no HTMLNames::.
Removed.

> > WebCore/html/HTMLElement.idl:65
> > +                 attribute boolean spellcheck;
> 
> This should probably use the [Reflect] extended attribute which would mean you don't need the implementation of spellcheck/setSpellcheck in HTMLElement.h/cpp
Oh, I didn't notice this. Thank you pointing this out.
In this case, I'd use explicit implementation
because spellcheck has nontrivial semantics like inheritance.

[Kent Tamura]

Comment on attachment 72158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72158&action=review

> LayoutTests/fast/dom/HTMLElement/script-tests/spellcheck.js:50
> +testFor("false", false, "false", true, "true");

Need more tests for other initialAttribute variations such as "TRUE" "FALSE", "", "foobar", "1", "0", ...

[Hajime Morrita]

Created attachment 72164 [details]
Patch

[Hajime Morrita]

Hi, thank you for another review!

> Need more tests for other initialAttribute variations such as "TRUE" "FALSE", "", "foobar", "1", "0", ...
Agreed and added more test cases.

[Kent Tamura]

Comment on attachment 72164 [details]
Patch

ok

[Hajime Morrita]

Committed r70763: <http://trac.webkit.org/changeset/70763>

[Sam Weinig]

> > This should probably use the [Reflect] extended attribute which would mean you don't need the implementation of spellcheck/setSpellcheck in HTMLElement.h/cpp
> Oh, I didn't notice this. Thank you pointing this out.
> In this case, I'd use explicit implementation
> because spellcheck has nontrivial semantics like inheritance.

I don't understand what you mean here.  Can you explain what you mean by this?

[Hajime Morrita]

> I don't understand what you mean here.  Can you explain what you mean by this?

In my understanding, @spellcheck properety returns the spellcheck availability of the element,
instead of attribute value itself.
 
For example, Given:
<div spellcheck="true">
    <div id="target">foo</div>
</div>

with:
var target = document.getElementById("target");

In this case, target.spellcheck returns true. 
With [Reflect], this would return false because a missing attribute turns into false.
(... is right? I'm not sure if I understand [Reflect] behavior precisely.)

[Sam Weinig]

(In reply to comment #14)
> > I don't understand what you mean here.  Can you explain what you mean by this?
> 
> In my understanding, @spellcheck properety returns the spellcheck availability of the element,
> instead of attribute value itself.
> 
> For example, Given:
> <div spellcheck="true">
>     <div id="target">foo</div>
> </div>
> 
> with:
> var target = document.getElementById("target");
> 
> In this case, target.spellcheck returns true. 
> With [Reflect], this would return false because a missing attribute turns into false.
> (... is right? I'm not sure if I understand [Reflect] behavior precisely.)

I see, thanks for the info.