Attachment 243435 Details for Bug 133939 – Fix for grammar-edit-word.html

[patch] Fix for grammar-edit-word.html

bug-133939-20141217163827.patch (text/plain), 11.42 KB, created by Grzegorz Czajkowski on 2014-12-17 07:37:37 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Grzegorz Czajkowski

Created: 2014-12-17 07:37:37 PST

Size: 11.42 KB

patch

obsolete

>Subversion Revision: 177443
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 655a7fceb972bd10102c260d2b595a1af7013fda..0f466a1110588da0822671646c0e6e0de24d5af9 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2014-12-16  Grzegorz Czajkowski  <g.czajkowski@gmail.com>
>+
>+        shouldBecomeEqual() behaves as shouldBe() if the testing expression returns the expected value
>+        https://bugs.webkit.org/show_bug.cgi?id=133939
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Since programmatically selection change triggers spellchecker asynchronously
>+        call updateEditorUINowIfScheduled() in Internals::hasGrammarMarker() to apply
>+        pending document markers.
>+        Internals::hasSpellingMarker() already does it.
>+
>+        No new tests, covered by editing/spelling/grammar-edit-word.html
>+
>+        * testing/Internals.cpp:
>+        (WebCore::Internals::hasGrammarMarker):
>+        Call updateEditorUINowIfScheduled() to update document markers.
>+
> 2014-12-17  Zan Dobersek  <zdobersek@igalia.com>
> 
>         [TexMap] Sprinkle range-based for-loops where still possible
>diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
>index ab241e6fc9636f6cc99a1d74299b36400f68250d..87f6df6e3939ab348e10b0efbd6a251b518f5346 100644
>--- a/Source/WebCore/testing/Internals.cpp
>+++ b/Source/WebCore/testing/Internals.cpp
>@@ -1563,6 +1563,8 @@ bool Internals::hasGrammarMarker(int from, int length, ExceptionCode&)
>     if (!document || !document->frame())
>         return 0;
> 
>+    updateEditorUINowIfScheduled();
>+
>     return document->frame()->editor().selectionStartHasMarkerFor(DocumentMarker::Grammar, from, length);
> }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e9303fe7b7e7c800152bb271a574d38012cea6fe..badc63bac401d8da356e0b33a1d2908a27770c81 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,53 @@
>+2014-12-15  Grzegorz Czajkowski  <g.czajkowski@samsung.com>
>+
>+        shouldBecomeEqual() behaves as shouldBe() if the testing expression returns the expected value
>+        https://bugs.webkit.org/show_bug.cgi?id=133939
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        First call of eval(expression) inside shouldBecomeEqual was always
>+        made synchronously. If the testing expression returns the same value
>+        as expected one then shouldBecomeEqual() will immediately report PASS,
>+        for example,
>+
>+        shouldBecomeEqual(internals.hasSpellingMarker("wellcome"), false);
>+
>+        in consequence, assuming asynchronous path of spellchecking, spelling markers
>+        may appear after a while.
>+
>+        The bug was caused by checking a condition at the beginning of shouldBecomeEqual(),
>+        before calling a timer. As a result, queued asynchronous events doesn't effect
>+        this checking.
>+
>+        * editing/spelling/editing-multiple-words-with-markers.html:
>+        Since this tests if spelling markers do not appear after a selection change
>+        after r164401 we need to call updateEditorUINowIfScheduled(). Without it editor
>+        would report old position of markers. Additionally, add timeout to make things
>+        testable in the browser.
>+        Without proposed change in shouldBecomeEqual, this bug was not revealed.
>+
>+        * editing/spelling/script-tests/spelling-backspace-between-lines.js:
>+        Replace magic number 100 with newly defined value.
>+        * editing/spelling/spellcheck-attribute.html: Ditto.
>+        * editing/spelling/spelling-backspace-between-lines.html: Ditto.
>+
>+        * editing/spelling/grammar-edit-word.html: Ditto.
>+        The reason why updateEditorUINowIfScheduled() is not mandatory here is that
>+        the test uses editing commands which already call updateEditorUINowIfScheduled().
>+        However, timeout needs to be added as while grammar checking
>+        Editor::respondToChangedSelection() is invoked in some circumstances
>+        and causes asynchronous spellchecker updates.
>+        This is probably a new bug that was not revealed yet.
>+
>+        * editing/spelling/resources/util.js: Ditto.
>+        Introduce the timeout value for pending selection updates that are
>+        required by spellchecker.
>+
>+        * resources/js-test-pre.js:
>+        (shouldBecomeEqual):
>+        (shouldBecomeDifferent):
>+        Always check a condition on timer.
>+
> 2014-12-16  Shivakumar JM  <shiva.jm@samsung.com>
> 
>         text node should not be created, On setting document.title to the empty string.
>diff --git a/LayoutTests/editing/spelling/editing-multiple-words-with-markers.html b/LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
>index 50096bfb634a751c8f7e80a6da17697f7420b920..28eedaa82b92960dc9f8fe274314899d193402e5 100644
>--- a/LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
>+++ b/LayoutTests/editing/spelling/editing-multiple-words-with-markers.html
>@@ -1,5 +1,6 @@
> <html>
> <head>
>+<script src="resources/util.js"></script>
> <script src="../editing.js"></script>
> <title>Editing multiple words with markers test</title>
> </head>
>@@ -67,7 +68,9 @@ function checkSpellingMarkerAfterDeleteingSelection(selectionRange, spellingMark
>     resetText();
>     if (selectionRange) {
>         document.getElementById('testElement').setSelectionRange(selectionRange[0], selectionRange[1]);
>-        execDeleteCommand();
>+        if (window.internals)
>+            internals.updateEditorUINowIfScheduled();
>+        setTimeout(execDeleteCommand, timeoutToApplyPendingProgramaticSelectionChanges);
>     }
> 
>     spellingPositions = spellingMarkerPositions;
>diff --git a/LayoutTests/editing/spelling/grammar-edit-word.html b/LayoutTests/editing/spelling/grammar-edit-word.html
>index ebfe276a101b63bf3e43a4258aa9896398e7a1f1..5a9f69388d4d6e0431c1ee9e844122566d12f2ab 100644
>--- a/LayoutTests/editing/spelling/grammar-edit-word.html
>+++ b/LayoutTests/editing/spelling/grammar-edit-word.html
>@@ -5,6 +5,7 @@
> </head> 
> <body>
> <script src="../../resources/js-test-pre.js"></script>
>+<script src="resources/util.js"></script>
> <div id="src" contenteditable="true" spellcheck="true"></div><br/>
> <script language="javascript">
> description('Test if WebKit removes grammar markers when we edit a grammatically-incorrect word. '
>@@ -18,13 +19,12 @@ if (window.internals) {
>     internals.settings.setAsynchronousSpellCheckingEnabled(true);
> }
> 
>-function editSentenceAndCheckMarker()
>-{
>-    debug('Delete the end of this sentence until it becomes "You ha".');
>-    for (var i = 0; i < 12; ++i)
>+function editSentenceAsyncAndCheckMarker() {
>+    if (target.innerText != "You ha") {
>         evalAndLog('testRunner.execCommand("DeleteBackward")');
>-
>-    shouldBecomeEqual('internals.hasGrammarMarker(4, 2)', 'false', finishJSTest);
>+        setTimeout(editSentenceAsyncAndCheckMarker, timeoutToApplyPendingProgramaticSelectionChanges);
>+    } else
>+        shouldBecomeEqual('internals.hasGrammarMarker(4, 2)', 'false', finishJSTest);
> }
> 
> var target = document.getElementById('src');
>@@ -32,7 +32,10 @@ target.focus();
> evalAndLog('document.execCommand("InsertText", false, "You has the right.")');
> 
> if (window.internals)
>-    shouldBecomeEqual('internals.hasGrammarMarker(4, 3)', 'true', editSentenceAndCheckMarker);
>+    shouldBecomeEqual('internals.hasGrammarMarker(4, 3)', 'true', function() {
>+        debug('Delete the end of this sentence until it becomes "You ha".')
>+        editSentenceAsyncAndCheckMarker();
>+    });
> </script>
> <script src="../../resources/js-test-post.js"></script>
> </body>
>diff --git a/LayoutTests/editing/spelling/resources/util.js b/LayoutTests/editing/spelling/resources/util.js
>index c6fa747aee75769aa6cf121cee79bae22daf67f4..87dd4073e0a84145e382f8b1600d0193d48ce5fc 100644
>--- a/LayoutTests/editing/spelling/resources/util.js
>+++ b/LayoutTests/editing/spelling/resources/util.js
>@@ -1,3 +1,5 @@
>+const timeoutToApplyPendingProgramaticSelectionChanges = 50;
>+
> function log(msg)
> {
>     document.getElementById("console").innerHTML += (msg + "\n");
>diff --git a/LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js b/LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
>index 5e4f47e3464dbb4cbddb6215250bbbf9766a1dd2..96766e3310a356b2ba0aaa09b7f4850c3291cde9 100644
>--- a/LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
>+++ b/LayoutTests/editing/spelling/script-tests/spelling-backspace-between-lines.js
>@@ -45,7 +45,7 @@ function testTwoLinesMisspellings()
>             shouldBecomeEqual("internals.hasSpellingMarker(3, 2)", "true", done);
>         else
>             done();
>-    }, 100);
>+    }, timeoutToApplyPendingProgramaticSelectionChanges);
> }
> 
> function testMisspellingsAfterLineMergeUsingDelete()
>diff --git a/LayoutTests/editing/spelling/spellcheck-attribute.html b/LayoutTests/editing/spelling/spellcheck-attribute.html
>index 99a0b9e12764726716ecf841be4e3d28e0fd11e1..387999a6b9270ddcb3466d5d71b5113838460e64 100644
>--- a/LayoutTests/editing/spelling/spellcheck-attribute.html
>+++ b/LayoutTests/editing/spelling/spellcheck-attribute.html
>@@ -59,7 +59,7 @@ function testMarkerForMisspelledWord(parentId, shouldBeMisspelled, id, type, spe
>             shouldBecomeEqual('internals.hasSpellingMarker(0, 2)', shouldBeMisspelled ? 'true' : 'false', done);
>         else
>             done();
>-    }, 10);
>+    }, timeoutToApplyPendingProgramaticSelectionChanges);
> }
> 
> function logMarkup(id, spellcheckValue, shouldCloseTag)
>diff --git a/LayoutTests/editing/spelling/spelling-backspace-between-lines.html b/LayoutTests/editing/spelling/spelling-backspace-between-lines.html
>index 36d8923ceebd77e4963ec71bf22ef48689c30d15..c5b4d70c658083cf44d82493dd6f133e9f1459a8 100644
>--- a/LayoutTests/editing/spelling/spelling-backspace-between-lines.html
>+++ b/LayoutTests/editing/spelling/spelling-backspace-between-lines.html
>@@ -3,6 +3,7 @@
> <head>
> <script src="../../resources/js-test-pre.js"></script>
> <script src="resources/js-test-selection-shared.js"></script>
>+<script src="resources/util.js"></script>
> </head>
> <body>
> <p id="description"></p>
>diff --git a/LayoutTests/resources/js-test-pre.js b/LayoutTests/resources/js-test-pre.js
>index e5812cb9dd91bc5e56b7d7fa6ada498b6300524c..2e3d15b4a0c72251bf7a428b9f5dcca16a518689 100644
>--- a/LayoutTests/resources/js-test-pre.js
>+++ b/LayoutTests/resources/js-test-pre.js
>@@ -289,7 +289,7 @@ function shouldBecomeEqual(_a, _b, completionHandler)
>   if (typeof _a != "string" || typeof _b != "string")
>     debug("WARN: shouldBecomeEqual() expects string arguments");
> 
>-  var condition = function() {
>+  function condition() {
>     var exception;
>     var _av;
>     try {
>@@ -305,8 +305,8 @@ function shouldBecomeEqual(_a, _b, completionHandler)
>       return true;
>     }
>     return false;
>-  };
>-  _waitForCondition(condition, completionHandler);
>+  }
>+  setTimeout(_waitForCondition, 0, condition, completionHandler);
> }
> 
> function shouldBecomeEqualToString(value, reference, completionHandler)
>@@ -401,7 +401,7 @@ function shouldBecomeDifferent(_a, _b, completionHandler)
>   if (typeof _a != "string" || typeof _b != "string")
>     debug("WARN: shouldBecomeDifferent() expects string arguments");
> 
>-  var condition = function() {
>+  function condition() {
>     var exception;
>     var _av;
>     try {
>@@ -417,8 +417,8 @@ function shouldBecomeDifferent(_a, _b, completionHandler)
>       return true;
>     }
>     return false;
>-  };
>-  _waitForCondition(condition, completionHandler);
>+  }
>+  setTimeout(_waitForCondition, 0, condition, completionHandler);
> }
> 
> function shouldBeTrue(_a) { shouldBe(_a, "true"); }

Actions: View | Formatted Diff | Diff

Attachments on bug 133939: 239473 | 239477 | 239725 | 239727 | 243435 | 243623