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
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?
Created attachment 103599 [details] Patch
Comment on attachment 103599 [details] Patch Attachment 103599 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9357168
(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".
> > 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.
Created attachment 103601 [details] Fixing gtk build
Comment on attachment 103601 [details] Fixing gtk build Attachment 103601 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9353153
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
Created attachment 103744 [details] Patch
Anyone? All bots are happy!
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?
Created attachment 104156 [details] Patch
> > 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.
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?
Created attachment 104305 [details] Patch
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.
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
Created attachment 104470 [details] Patch
Created attachment 104471 [details] Patch
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
Created attachment 104481 [details] Patch
Comment on attachment 104481 [details] Patch Clearing flags on attachment: 104481 Committed r93392: <http://trac.webkit.org/changeset/93392>
All reviewed patches have been landed. Closing bug.
The test added by this patch is failing on Qt :(
Oh man, this is also failing on Windows.