Bug 25536

Created attachment 72002 [details] Patch

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

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

Created attachment 72154 [details] Patch

Created attachment 72158 [details] Patch

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

Created attachment 72164 [details] Patch

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 on attachment 72164 [details] Patch ok

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

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

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

(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.