Bug 25536 - WebKit needs to expose an Element::spellcheck attribute to javascript
Summary: WebKit needs to expose an Element::spellcheck attribute to javascript
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.whatwg.org/specs/web-apps/...
Keywords:
Depends on: 14552
Blocks: 48411
  Show dependency treegraph
 
Reported: 2009-05-03 23:49 PDT by Eric Seidel (no email)
Modified: 2010-11-04 08:13 PDT (History)
2 users (show)

See Also:


Attachments
Patch (16.14 KB, patch)
2010-10-27 04:06 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (18.34 KB, patch)
2010-10-28 00:48 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (18.33 KB, patch)
2010-10-28 01:26 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (22.96 KB, patch)
2010-10-28 02:41 PDT, Hajime Morrita
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-05-03 23:49:11 PDT
WebKit needs to expose an Element::spellcheck attribute to javascript

Per HTML5:
http://www.whatwg.org/specs/web-apps/current-work/#attr-spellcheck
Comment 1 Hajime Morrita 2010-10-27 04:06:43 PDT
Created attachment 72002 [details]
Patch
Comment 2 Hajime Morrita 2010-10-27 04:13:29 PDT
Dynamic spellcheck behavior change is not implemented at the patch. 
It is filed separately on Bug 48411.
Comment 3 Kent Tamura 2010-10-27 05:24:14 PDT
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::.
Comment 4 Sam Weinig 2010-10-27 10:03:09 PDT
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
Comment 5 Hajime Morrita 2010-10-28 00:48:29 PDT
Created attachment 72154 [details]
Patch
Comment 6 Hajime Morrita 2010-10-28 01:26:47 PDT
Created attachment 72158 [details]
Patch
Comment 7 Hajime Morrita 2010-10-28 01:29:58 PDT
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.
Comment 8 Kent Tamura 2010-10-28 01:42:11 PDT
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", ...
Comment 9 Hajime Morrita 2010-10-28 02:41:12 PDT
Created attachment 72164 [details]
Patch
Comment 10 Hajime Morrita 2010-10-28 02:42:22 PDT
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.
Comment 11 Kent Tamura 2010-10-28 02:45:39 PDT
Comment on attachment 72164 [details]
Patch

ok
Comment 12 Hajime Morrita 2010-10-28 03:42:18 PDT
Committed r70763: <http://trac.webkit.org/changeset/70763>
Comment 13 Sam Weinig 2010-11-02 04:12:40 PDT
> > 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?
Comment 14 Hajime Morrita 2010-11-04 01:56:07 PDT
> 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.)
Comment 15 Sam Weinig 2010-11-04 08:13:28 PDT
(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.