| Summary: | @spellcheck attribute at the child of contentEditable node is ignored. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||||
| Component: | HTML Editing | Assignee: | Hajime Morrita <morrita> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | eric, jiapu.mail, ojan | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 72494 [details]
Patch
Comment on attachment 72494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72494&action=review > LayoutTests/editing/spelling/spelling-attribute-at-child-expected.txt:14 > +PASS layoutTestController.hasSpellingMarker(1, 2) is true The readability of the test result is not good. Every test looks to check the same thing, layoutTestController.hasSpellingMarker(1, 2). How about testSpellCheckingEnabled() returns the boolean result of hasSpellingMarker(), and call shouldBe() at the top level like shouldBeFalse('testSpellCheckingEnabled(undefined, false))? > WebCore/editing/Editor.cpp:2130 > + return isSpellCheckingEnabledFor(m_frame->selection()->start().node()); Why m_frame->selection()->start().node() instead of document()->focusedNode() ? Comment on attachment 72494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72494&action=review > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:15 > + root.innerHTML = "<div id='parent' contentEditable>Foo <span id='child'>[]</span> Baz</div>"; > + var parent = document.getElementById("parent"); > + if (undefined != parentValue) > + parent.spellcheck = parentValue; > + var child = document.getElementById("child"); > + if (undefined != childValue) > + child.spellcheck = childValue; I think it's cleaner to pass the entire markup to this function rather than passing boolean values and modifying DOM by script like this. You can then remove id attributes (you can just make a contract that there's exactly one span and do var text = document.elementsByTagName('span')[0].firstChild. > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:22 > + shouldBe("layoutTestController.hasSpellingMarker(1, 2)", expectedEnabled ? "true" : "false"); I don't think this is a really nice usage of shouldBe. I personally like calling testPassed / testFailed manually with more descriptive message. See See http://trac.webkit.org/browser/trunk/LayoutTests/editing/execCommand/script-tests/query-command-state.js for an example. >> LayoutTests/editing/spelling/spelling-attribute-at-child-expected.txt:14 >> +PASS layoutTestController.hasSpellingMarker(1, 2) is true > > The readability of the test result is not good. Every test looks to check the same thing, layoutTestController.hasSpellingMarker(1, 2). > > How about testSpellCheckingEnabled() returns the boolean result of hasSpellingMarker(), and call shouldBe() at the top level like shouldBeFalse('testSpellCheckingEnabled(undefined, false))? I agree with Kent. These results look cryptic. We need to look at the actual script to know what the test is doing. Created attachment 72618 [details]
Patch
Oops, I've missed Ryosuke's comment. I'll address it soon. Created attachment 72619 [details]
Patch
Hi Kent-san, Ryosuke, thank you for reviewing this! > The readability of the test result is not good. Every test looks to check the same thing, layoutTestController.hasSpellingMarker(1, 2). > > How about testSpellCheckingEnabled() returns the boolean result of hasSpellingMarker(), and call shouldBe() at the top level like shouldBeFalse('testSpellCheckingEnabled(undefined, false))? Sounds better. Passing markup itself looks bette too as Ryosuke mentioned. I rewrote to such. > > > WebCore/editing/Editor.cpp:2130 > > + return isSpellCheckingEnabledFor(m_frame->selection()->start().node()); > > Why m_frame->selection()->start().node() instead of document()->focusedNode() ? This is the point of this fix. For <input> or <text> focusNode() is OK. For contentEditable nodes, the focus is on editable root, instead of where the caret is. And such node could have @spellcheck attribute. I think it is OK to call it isSpellCheckingEnabledInFocusedNode() because the caret node is a child of actual focused node and it's a detail of WebCore. So WebKit layer, who is calling this API, doesn't need to know that. Comment on attachment 72619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72619&action=review > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:10 > + var parent = document.getElementById("parent"); parent is never used. > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:30 > +shouldBeFalse("testSpellChecking(\"<div id='parent' contentEditable>Foo <span spellcheck='false' id='child'>[]</span> Baz</div>\")"); > +shouldBeTrue("testSpellChecking(\"<div id='parent' contentEditable>Foo <span id='child'>[]</span> Baz</div>\")"); > +shouldBeTrue("testSpellChecking(\"<div id='parent' contentEditable>Foo <span spellcheck='true' id='child'>[]</span> Baz</div>\")"); > +shouldBeFalse("testSpellChecking(\"<div id='parent' spellcheck='false' contentEditable>Foo <span spellcheck='false' id='child'>[]</span> Baz</div>\")"); > +shouldBeFalse("testSpellChecking(\"<div id='parent' spellcheck='false' contentEditable>Foo <span id='child'>[]</span> Baz</div>\")"); > +shouldBeTrue("testSpellChecking(\"<div id='parent' spellcheck='false' contentEditable>Foo <span spellcheck='true' id='child'>[]</span> Baz</div>\")"); > +shouldBeFalse("testSpellChecking(\"<div id='parent' spellcheck='true' contentEditable>Foo <span spellcheck='false' id='child'>[]</span> Baz</div>\")"); > +shouldBeTrue("testSpellChecking(\"<div id='parent' spellcheck='true' contentEditable>Foo <span id='child'>[]</span> Baz</div>\")"); > +shouldBeTrue("testSpellChecking(\"<div id='parent' spellcheck='true' contentEditable>Foo <span spellcheck='true' id='child'>[]</span> Baz</div>\")"); You don't need parent here. You don't even need id='child' if you do what I said in the earlier comment but I used to put id="test" all the time so I guess it's up to you. Created attachment 72623 [details]
removed 'parent' from test
Comment on attachment 72623 [details] removed 'parent' from test View in context: https://bugs.webkit.org/attachment.cgi?id=72623&action=review > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:11 > + var child = document.getElementById("child"); > + var text = child.firstChild; Sorry, I keep adding comments but you can rewrite this just in one line: var text = document.getElementById('child').firstChild. > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:12 > + sel.setBaseAndExtent(text, 1, text, 1); You can do window.getSelection().setPosition(text, 1) here as well. Then you can get rid of "sel" variable. > LayoutTests/editing/spelling/spelling-attribute-at-child-expected.txt:6 > +PASS testSpellChecking("<div contentEditable>Foo <span spellcheck='false' id='child'>[]</span> Baz</div>") is false Maybe rename testSpellChecking to childHasSpellingMarker so that the test reads as: PASS childHasSpellingMarker(~~) is true/false Created attachment 72629 [details]
Patch
> > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:11 > > + var child = document.getElementById("child"); > > + var text = child.firstChild; > > Sorry, I keep adding comments but you can rewrite this just in one line: var text = document.getElementById('child').firstChild. Done. > > > LayoutTests/editing/spelling/script-tests/spelling-attribute-at-child.js:12 > > + sel.setBaseAndExtent(text, 1, text, 1); > > You can do window.getSelection().setPosition(text, 1) here as well. Then you can get rid of "sel" variable. > Done. > > LayoutTests/editing/spelling/spelling-attribute-at-child-expected.txt:6 > > +PASS testSpellChecking("<div contentEditable>Foo <span spellcheck='false' id='child'>[]</span> Baz</div>") is false > > Maybe rename testSpellChecking to childHasSpellingMarker so that the test reads as: PASS childHasSpellingMarker(~~) is true/false Done. Wow, the code goes further concise than original version. Comment on attachment 72629 [details]
Patch
ok
Committed r71101: <http://trac.webkit.org/changeset/71101> |
Created attachment 72013 [details] repro @spellcheck attribute is ignored when it is given at the node ascendant of the contentEditable element (not the contentEditable element itself.) <div contentEditable="true"> <div>Here is spellcheck=true: [<span spellcheck="true" >test should be checked here.</span>]</div> <div>Here is spellcheck=false: [<span spellcheck="false">text should NOT be checked here, but does.</span>]</div> </div> Currently @spellcheck is checked against focused node, which is an editable root at the case of rich text editing (not form.) We should check the node under the selection(caret) at that case.