RESOLVED FIXED 65902
Spell-checking doesn't recognize word boundaries on contests inserted by execCommand('insertHTML')
https://bugs.webkit.org/show_bug.cgi?id=65902
Summary Spell-checking doesn't recognize word boundaries on contests inserted by exec...
Hajime Morrita
Reported 2011-08-09 01:35:08 PDT
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
Attachments
Patch (263.26 KB, patch)
2011-08-11 04:34 PDT, Hajime Morrita
no flags
Fixing gtk build (263.71 KB, patch)
2011-08-11 04:53 PDT, Hajime Morrita
no flags
Patch (269.49 KB, patch)
2011-08-11 23:52 PDT, Hajime Morrita
no flags
Patch (269.50 KB, patch)
2011-08-17 00:21 PDT, Hajime Morrita
no flags
Patch (269.66 KB, patch)
2011-08-17 22:40 PDT, Hajime Morrita
no flags
Patch (269.58 KB, patch)
2011-08-19 00:15 PDT, Hajime Morrita
no flags
Patch (269.57 KB, patch)
2011-08-19 00:18 PDT, Hajime Morrita
no flags
Patch (269.56 KB, patch)
2011-08-19 02:24 PDT, Hajime Morrita
no flags
Ryosuke Niwa
Comment 1 2011-08-10 21:23:36 PDT
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
Comment 2 2011-08-11 04:34:42 PDT
Collabora GTK+ EWS bot
Comment 3 2011-08-11 04:40:21 PDT
Hajime Morrita
Comment 4 2011-08-11 04:45:06 PDT
(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
Comment 5 2011-08-11 04:49:12 PDT
> > 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
Comment 6 2011-08-11 04:53:29 PDT
Created attachment 103601 [details] Fixing gtk build
Gustavo Noronha (kov)
Comment 7 2011-08-11 05:00:24 PDT
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 8 2011-08-11 05:17:23 PDT
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
Comment 9 2011-08-11 23:52:06 PDT
Hajime Morrita
Comment 10 2011-08-15 23:28:09 PDT
Anyone? All bots are happy!
Ryosuke Niwa
Comment 11 2011-08-15 23:43:13 PDT
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
Comment 12 2011-08-17 00:21:47 PDT
Hajime Morrita
Comment 13 2011-08-17 00:26:03 PDT
> > 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 14 2011-08-17 00:42:16 PDT
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
Comment 15 2011-08-17 22:40:07 PDT
Hajime Morrita
Comment 16 2011-08-17 22:43:06 PDT
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 17 2011-08-18 07:16:22 PDT
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
Comment 18 2011-08-19 00:15:01 PDT
Hajime Morrita
Comment 19 2011-08-19 00:18:19 PDT
WebKit Review Bot
Comment 20 2011-08-19 01:34:46 PDT
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
Comment 21 2011-08-19 02:24:34 PDT
WebKit Review Bot
Comment 22 2011-08-19 02:43:41 PDT
Comment on attachment 104481 [details] Patch Clearing flags on attachment: 104481 Committed r93392: <http://trac.webkit.org/changeset/93392>
WebKit Review Bot
Comment 23 2011-08-19 02:43:50 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 24 2011-08-19 20:12:56 PDT
The test added by this patch is failing on Qt :(
Ryosuke Niwa
Comment 25 2011-08-19 20:36:24 PDT
Oh man, this is also failing on Windows.
Note You need to log in before you can comment on or make changes to this bug.