Bug 48418 – @spellcheck attribute at the child of contentEditable node is ignored.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 72494 [details]
Patch

[Kent Tamura]

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

[Ryosuke Niwa]

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.

[Hajime Morrita]

Created attachment 72618 [details]
Patch

[Hajime Morrita]

Oops, I've missed Ryosuke's comment. I'll address it soon.

[Hajime Morrita]

Created attachment 72619 [details]
Patch

[Hajime Morrita]

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.

[Ryosuke Niwa]

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.

[Hajime Morrita]

Created attachment 72623 [details]
removed 'parent' from test

[Ryosuke Niwa]

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

[Hajime Morrita]

Created attachment 72629 [details]
Patch

[Hajime Morrita]

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

[Kent Tamura]

Comment on attachment 72629 [details]
Patch

ok

[Hajime Morrita]

Committed r71101: <http://trac.webkit.org/changeset/71101>