Bug 65902 – Spell-checking doesn't recognize word boundaries on contests inserted by execCommand('insertHTML')

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hajime Morrita]

For example, considering following HTML
<p contentEditable>food</p><p contentEditable>food</p>"
Then if we insert "o<br>fo" between "<p contentEditable>fo" and "od</p>" to
create "<p contentEditable>foo<br>food</p>", where "foo" should be marked as misspelling.
But it isn't marked.

This bugs makes pixel expectations for following test wrong:

  editing/pasteboard/merge-after-delete-1.html                                                                                                                                                                                                                                                                                                          
  editing/pasteboard/merge-after-delete-2.html
  editing/pasteboard/merge-after-delete.html
  editing/pasteboard/merge-end-blockquote.html
  editing/pasteboard/merge-end-list.html
  editing/pasteboard/merge-end-table.html

[Ryosuke Niwa]

I don't quite get what you mean by spellcheck "doesn't care word boundary".  Do you mean that spellcheck doesn't work on the contents inserted by execCommand('insertedHTML') if the contents includes BR?

[Hajime Morrita]

Created attachment 103599 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 103599 [details]
Patch

Attachment 103599 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9357168

[Hajime Morrita]

(In reply to comment #1)
> I don't quite get what you mean by spellcheck "doesn't care word boundary".  Do you mean that spellcheck doesn't work on the contents inserted by execCommand('insertedHTML') if the contents includes BR?

No.
For example, inserting " foo" to "bed", between "be" and "d", the result is "be food".
Both "be" and "food" is correct word. But because current code check spelling against " foo",
the word "foo" is marked. But the checks should be done against "be" and "food".

[Hajime Morrita]

> 
> No.
> For example, inserting " foo" to "bed", between "be" and "d", the result is "be food".
> Both "be" and "food" is correct word. But because current code check spelling against " foo",
> the word "foo" is marked. But the checks should be done against "be" and "food".
With this patch, the checks are "be" and "food".
Note that even If "food" was a wrong word, this patch doesn't mark "food",
because Editor class considers only markers inside inserted text (" foo").
This is not good. But fixing it needs bigger change. I'd like to attack it with a separate change.

[Hajime Morrita]

Created attachment 103601 [details]
Fixing gtk build

[Gustavo Noronha (kov)]

Comment on attachment 103601 [details]
Fixing gtk build

Attachment 103601 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9353153

[WebKit Review Bot]

Comment on attachment 103601 [details]
Fixing gtk build

Attachment 103601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9346175

New failing tests:
editing/spelling/spelling-insert-html.html

[Hajime Morrita]

Created attachment 103744 [details]
Patch

[Hajime Morrita]

Anyone? All bots are happy!

[Ryosuke Niwa]

Comment on attachment 103744 [details]
Patch

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

Sorry r- for various nits.

> ChangeLog:4
> +        Spell-checking against execCommand() inserted HTML doesn't care word boundary.
> +        https://bugs.webkit.org/show_bug.cgi?id=65902

Please update the bug title.

> LayoutTests/ChangeLog:8
> +        Existing expectation was wrong because some markers spans a part of words.

maybe 'some markers on substrings of words'?

> LayoutTests/ChangeLog:9
> +        With this fix, now Editor rejects such invalid markers.

Editor now rejects such markers.

> LayoutTests/editing/spelling/spelling-insert-html.html:14
> +description("The spellchecker shouldn't check words on the paste border but check inside it.");

What does "paste border" mean?

> LayoutTests/editing/spelling/spelling-insert-html.html:35
> +    shouldBe("markedText", "'zz'");

Use shouldBeEqualToString instead?

> LayoutTests/editing/spelling/spelling-insert-html.html:40
> +    layoutTestController.notifyDone();

Why are we calling this?  By the way, should we wait until the page loads?  If I remember correctly, some of spell-checking stuff won't run until the page is fully loaded and we've had some flakiness in our tests because of that.

> Source/WebCore/ChangeLog:9
> +        But these are low-level APIand caller should take care of word boundary if input.

Nit: API and.
if input what?

> Source/WebCore/editing/Editor.cpp:1946
> +    markMisspellingsAndBadGrammar(movingSelection, markGrammar, movingSelection);

This is all we have to do to fix this bug!?  That's quite amazing.  I guess you had a really good insight into what refactoring we should be doing.

> Source/WebCore/testing/Internals.cpp:184
> +unsigned Internals::markedSizeOf(Node* node, ExceptionCode& ec)

I don't really understand what this function does.  Can we come up with a more descriptive name?

> Source/WebKit2/win/WebKit2CFLite.def:142
> +	?create@Range@WebCore@@SA?AV?$PassRefPtr@VRange@WebCore@@@WTF@@V?$PassRefPtr@VDocument@WebCore@@@4@V?$PassRefPtr@VNode@WebCore@@@4@H1H@Z

Wrong indentation?

[Hajime Morrita]

Created attachment 104156 [details]
Patch

[Hajime Morrita]

> 
> Sorry r- for various nits.
Tanks for reviewing anyway ;-)

> 
> > ChangeLog:4
> > +        Spell-checking against execCommand() inserted HTML doesn't care word boundary.
> > +        https://bugs.webkit.org/show_bug.cgi?id=65902
> 
> Please update the bug title.
Fixed.

> 
> > LayoutTests/ChangeLog:8
> > +        Existing expectation was wrong because some markers spans a part of words.
> 
> maybe 'some markers on substrings of words'?
Sure. Fixed.

> 
> > LayoutTests/ChangeLog:9
> > +        With this fix, now Editor rejects such invalid markers.
> 
> Editor now rejects such markers.
Fixed.

> 
> > LayoutTests/editing/spelling/spelling-insert-html.html:14
> > +description("The spellchecker shouldn't check words on the paste border but check inside it.");
> 
> What does "paste border" mean?
Now "The spellchecker shouldn't mark substrings of words after pasting."

> 
> > LayoutTests/editing/spelling/spelling-insert-html.html:35
> > +    shouldBe("markedText", "'zz'");
> 
> Use shouldBeEqualToString instead?
Oh, I didn't know that. Thanks for pointing. 

> 
> > LayoutTests/editing/spelling/spelling-insert-html.html:40
> > +    layoutTestController.notifyDone();
> 
> Why are we calling this?  By the way, should we wait until the page loads?  If I remember correctly, some of spell-checking stuff won't run until the page is fully loaded and we've had some flakiness in our tests because of that.
Just wrong in this case. Removed.

> 
> > Source/WebCore/ChangeLog:9
> > +        But these are low-level APIand caller should take care of word boundary if input.
> 
> Nit: API and.
> if input what?
Whoa, completely broken sentence. Fixed.

> 
> > Source/WebCore/editing/Editor.cpp:1946
> > +    markMisspellingsAndBadGrammar(movingSelection, markGrammar, movingSelection);
> 
> This is all we have to do to fix this bug!?  That's quite amazing.  I guess you had a really good insight into what refactoring we should be doing.
Well, that's because I wrote the original code and it was simply wrong...
By the way, this is a preparation more meaningful fix of Bug 65849.

> 
> > Source/WebCore/testing/Internals.cpp:184
> > +unsigned Internals::markedSizeOf(Node* node, ExceptionCode& ec)
> 
> I don't really understand what this function does.  Can we come up with a more descriptive name?
> 
Renamed to markerCountOf().

> > Source/WebKit2/win/WebKit2CFLite.def:142
> > +	?create@Range@WebCore@@SA?AV?$PassRefPtr@VRange@WebCore@@@WTF@@V?$PassRefPtr@VDocument@WebCore@@@4@V?$PassRefPtr@VNode@WebCore@@@4@H1H@Z
> 
> Wrong indentation?
Fixed.

[Ryosuke Niwa]

Comment on attachment 104156 [details]
Patch

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

> LayoutTests/editing/spelling/spelling-insert-html.html:1
> +<html>

missing DOCTYPE?

> LayoutTests/editing/spelling/spelling-insert-html.html:34
> +    // The first "foo" isn't checked because it crosses the pasted and base html.

Is this a bug?  If also, you should say that "this demonstrates a bug X" where X is the bug number in the description above.

> Source/WebCore/editing/Editor.cpp:1945
>      bool markSpelling = isContinuousSpellCheckingEnabled();
>      bool markGrammar = markSpelling && isGrammarCheckingEnabled();

It appears that we don't need these booleans anymore, especially markSpelling.

> Source/WebCore/testing/Internals.cpp:184
> +unsigned Internals::markerCountOf(Node* node, ExceptionCode& ec)

I think we should include "Node" in the name since this is a ES5 function and ES5 is duck-typed; i.e. can't tell what the argument is.

How about markerCountForNode?

> Source/WebCore/testing/Internals.cpp:194
> +PassRefPtr<Range> Internals::markerRangeAt(Node* node, unsigned index, ExceptionCode& ec)

"RangeAt" sounds like you're obtaining a marker at a particular DOM position.  How about just markerForNode?

[Hajime Morrita]

Created attachment 104305 [details]
Patch

[Hajime Morrita]

HI Ryosuke, tahnk you for another round!

(In reply to comment #14)
> (From update of attachment 104156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104156&action=review
> 
> > LayoutTests/editing/spelling/spelling-insert-html.html:1
> > +<html>
> 
> missing DOCTYPE?
> 
Added.

> > LayoutTests/editing/spelling/spelling-insert-html.html:34
> > +    // The first "foo" isn't checked because it crosses the pasted and base html.
> 
> Is this a bug?  If also, you should say that "this demonstrates a bug X" where X is the bug number in the description above.
Filed Bug 66450.

> 
> > Source/WebCore/editing/Editor.cpp:1945
> >      bool markSpelling = isContinuousSpellCheckingEnabled();
> >      bool markGrammar = markSpelling && isGrammarCheckingEnabled();
> 
> It appears that we don't need these booleans anymore, especially markSpelling.
Sure. Inlined.

> 
> > Source/WebCore/testing/Internals.cpp:184
> > +unsigned Internals::markerCountOf(Node* node, ExceptionCode& ec)
> 
> I think we should include "Node" in the name since this is a ES5 function and ES5 is duck-typed; i.e. can't tell what the argument is.
> 
> How about markerCountForNode?
Done.

> 
> > Source/WebCore/testing/Internals.cpp:194
> > +PassRefPtr<Range> Internals::markerRangeAt(Node* node, unsigned index, ExceptionCode& ec)
> 
> "RangeAt" sounds like you're obtaining a marker at a particular DOM position.  How about just markerForNode?
Renamed to markerRangeForNode.

[WebKit Review Bot]

Comment on attachment 104305 [details]
Patch

Rejecting attachment 104305 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected text diff mismatch : (1)
  editing/spelling/spelling-insert-html.html = TEXT



Full output: http://queues.webkit.org/results/9424602

[Hajime Morrita]

Created attachment 104470 [details]
Patch

[Hajime Morrita]

Created attachment 104471 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104471 [details]
Patch

Rejecting attachment 104471 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 1

Last 500 characters of output:
hangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Source/WebKit2/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9439160

[Hajime Morrita]

Created attachment 104481 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104481 [details]
Patch

Clearing flags on attachment: 104481

Committed r93392: <http://trac.webkit.org/changeset/93392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The test added by this patch is failing on Qt :(

[Ryosuke Niwa]

Oh man, this is also failing on Windows.