⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 201490 in webkit


Ignore:
Timestamp:
May 28, 2016, 12:18:48 PM (10 years ago)
Author:
rniwa@webkit.org
Message:

Autocorrection makes it hard to type "doesn't" and to type @ in email addresses
​https://bugs.webkit.org/show_bug.cgi?id=158177
.:

Reviewed by Darin Adler.

Fixed manual tests for autocorrection panels and added a manual test for . Most of changes are fixing up the path to LayoutTests/editing/editing.js.

Also wrap many steps to type in a space or delete a character inside setTimeout since autocorrection happens on a timer
and the fact WebKit2 communicates with NSSpellChecker via IPC makes the behavior even more indeterministic.

  • ManualTests/autocorrection/autocorrection-at-mark.html: Added.
  • ManualTests/autocorrection/autocorrection-cancelled-by-ESC.html:
  • ManualTests/autocorrection/autocorrection-cancelled-by-typing-1.html:
  • ManualTests/autocorrection/autocorrection-contraction-2.html: Added.
  • ManualTests/autocorrection/autocorrection-contraction.html:
  • ManualTests/autocorrection/autocorrection-in-iframe.html:
  • ManualTests/autocorrection/close-window-when-correction-is-shown.html:
  • ManualTests/autocorrection/continue-typing-to-dismiss-reversion.html:
  • ManualTests/autocorrection/delete-to-dismiss-reversion.html:
  • ManualTests/autocorrection/delete-to-end-of-word-to-show-reversion.html:
  • ManualTests/autocorrection/dismiss-multiple-guesses.html:
  • ManualTests/autocorrection/document-for-iframe-test.html: Removed.
  • ManualTests/autocorrection/move-to-end-of-word-to-show-reversion.html: Type a space and move care in setTimeout as

the reversion panel wouldn't show up otherwise.

  • ManualTests/autocorrection/remove-misspelling-marker-after-appending-letter.html: Delay the typing of a space as well as

deleting letters since autocorrection panel wouldn't show up in time otherwise, and deleting character immediately would
reject the autocorrection instead of accepting it. Also removed the steps to add back the spellchecking marker and extracted
it as a separate test.

  • ManualTests/autocorrection/removing-misspelling-marker-after-appending-letter-2.html: Copied. This test continues the full

scenario in the previous test by typing a space and deleting the character, thereby bringing up spellchecking marker.

  • ManualTests/autocorrection/resources: Added.
  • ManualTests/autocorrection/resources/document-for-iframe-test.html: Moved from ManualTests/autocorrection/.
  • ManualTests/autocorrection/select-from-multiple-guesses.html: Added a missing instruction.
  • ManualTests/autocorrection/spell-checking-after-reversion.html:
  • ManualTests/autocorrection/type-whitespace-to-dismiss-reversion.html: Delay the typing of a space and moving the selection

since the reversion panel wouldn't show up otherwise.

  • ManualTests/autocorrection/undo-autocorrection-2.html: Copied. Automated most of steps in the second test case.
  • ManualTests/autocorrection/undo-autocorrection.html:

Source/WebCore:

<rdar://problem/20490862>
<rdar://problem/24707954>

Reviewed by Darin Adler.

When the user had typed "doesn'", some unified spellchecker may try to autocorrect it to "doesn't" or "does"
but we should ignore this for a moment until the next character is typed by the user. The code to deal with
this situation which checks the existence of an "ambiguous boundary character" was not robust when the
replacement text was longer than the corrected text.

Fixed this bug by fixing the logic to detect this case. Also added '@' as an ambiguous boundary character
since autocorrecting letters that appear right before '@' would not be useful in many cases.

Tests: ManualTests/autocorrection/autocorrection-at-mark.html

ManualTests/autocorrection/autocorrection-contraction-2.html

  • editing/AlternativeTextController.cpp:

(WebCore::AlternativeTextController::timerFired): Fixed a bug that we can show an empty reversion panel.

  • editing/Editor.cpp:

(WebCore::Editor::markAndReplaceFor): When the user had typed "doesn'" and our autocorrection result is
"doesn't", resultEndLocation (the end of "doesn't") is larger than selectionOffset (the end of "doesn'").
When the correction is "does", resultEndLocation (the end of "does") is one less than selectionOffset.
Updated the condition to deal with both of these conditions as well as cases where the correction result
contains more than one letter after '.

  • editing/htmlediting.cpp:

(WebCore::isAmbiguousBoundaryCharacter): Moved from the header file since this is not a hot function and
doesn't need to be inlined everywhere. Added '@' as an ambiguous boundary character.

  • editing/htmlediting.h:

(WebCore::isAmbiguousBoundaryCharacter): Moved to the cpp file.

Location:
trunk
Files:
3 added
21 edited
1 copied
1 moved

Legend:

Unmodified
Added
Removed

  • trunk/ChangeLog

    r201413 r201490  
     12016-05-28  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Autocorrection makes it hard to type "doesn't" and to type @ in email addresses
     4        https://bugs.webkit.org/show_bug.cgi?id=158177
     5
     6        Reviewed by Darin Adler.
     7
     8        Fixed manual tests for autocorrection panels and added a manual test for . Most of changes are fixing up the path to LayoutTests/editing/editing.js.
     9
     10        Also wrap many steps to type in a space or delete a character inside setTimeout since autocorrection happens on a timer
     11        and the fact WebKit2 communicates with NSSpellChecker via IPC makes the behavior even more indeterministic.
     12
     13        * ManualTests/autocorrection/autocorrection-at-mark.html: Added.
     14        * ManualTests/autocorrection/autocorrection-cancelled-by-ESC.html:
     15        * ManualTests/autocorrection/autocorrection-cancelled-by-typing-1.html:
     16        * ManualTests/autocorrection/autocorrection-contraction-2.html: Added.
     17        * ManualTests/autocorrection/autocorrection-contraction.html:
     18        * ManualTests/autocorrection/autocorrection-in-iframe.html:
     19        * ManualTests/autocorrection/close-window-when-correction-is-shown.html:
     20        * ManualTests/autocorrection/continue-typing-to-dismiss-reversion.html:
     21        * ManualTests/autocorrection/delete-to-dismiss-reversion.html:
     22        * ManualTests/autocorrection/delete-to-end-of-word-to-show-reversion.html:
     23        * ManualTests/autocorrection/dismiss-multiple-guesses.html:
     24        * ManualTests/autocorrection/document-for-iframe-test.html: Removed.
     25        * ManualTests/autocorrection/move-to-end-of-word-to-show-reversion.html: Type a space and move care in setTimeout as
     26        the reversion panel wouldn't show up otherwise.
     27        * ManualTests/autocorrection/remove-misspelling-marker-after-appending-letter.html: Delay the typing of a space as well as
     28        deleting letters since autocorrection panel wouldn't show up in time otherwise, and deleting character immediately would
     29        reject the autocorrection instead of accepting it. Also removed the steps to add back the spellchecking marker and extracted
     30        it as a separate test.
     31        * ManualTests/autocorrection/removing-misspelling-marker-after-appending-letter-2.html: Copied. This test continues the full
     32        scenario in the previous test by typing a space and deleting the character, thereby bringing up spellchecking marker.
     33        * ManualTests/autocorrection/resources: Added.
     34        * ManualTests/autocorrection/resources/document-for-iframe-test.html: Moved from ManualTests/autocorrection/.
     35        * ManualTests/autocorrection/select-from-multiple-guesses.html: Added a missing instruction.
     36        * ManualTests/autocorrection/spell-checking-after-reversion.html:
     37        * ManualTests/autocorrection/type-whitespace-to-dismiss-reversion.html: Delay the typing of a space and moving the selection
     38        since the reversion panel wouldn't show up otherwise.
     39        * ManualTests/autocorrection/undo-autocorrection-2.html: Copied. Automated most of steps in the second test case.
     40        * ManualTests/autocorrection/undo-autocorrection.html:
     41
    1422016-05-25  Konstantin Tokarev  <annulen@yandex.ru>
    243

  • trunk/ManualTests/autocorrection/autocorrection-cancelled-by-ESC.html

    r77577 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/autocorrection-cancelled-by-typing-1.html

    r77577 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/autocorrection-contraction.html

    r80226 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/autocorrection-in-iframe.html

    r80408 r201490  
    1111<br>
    1212<div style="-webkit-transform: translate(100px, 100px) rotate(20deg)">
    13   <iframe src="document-for-iframe-test.html"></iframe>
     13  <iframe src="resources/document-for-iframe-test.html"></iframe>
    1414</div>
    1515</body>

  • trunk/ManualTests/autocorrection/close-window-when-correction-is-shown.html

    r81847 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/continue-typing-to-dismiss-reversion.html

    r77577 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/delete-to-dismiss-reversion.html

    r77577 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/delete-to-end-of-word-to-show-reversion.html

    r86406 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>
    … …  
    2323    typeCharacterCommand('g');
    2424    typeCharacterCommand('e');
    25     typeCharacterCommand(' ');
    26     execDeleteCommand();
     25    setTimeout(function () {
     26        typeCharacterCommand(' ');
     27        setTimeout(function () {
     28            execDeleteCommand();
     29        }, 100);
     30    }, 100);
    2731}
    2832

  • trunk/ManualTests/autocorrection/dismiss-multiple-guesses.html

    r77577 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/move-to-end-of-word-to-show-reversion.html

    r86406 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>
    … …  
    2323    typeCharacterCommand('g');
    2424    typeCharacterCommand('e');
    25     typeCharacterCommand(' ');
    26     execMoveSelectionBackwardByCharacterCommand();
     25    setTimeout(function () {
     26        typeCharacterCommand(' ');
     27            setTimeout(function () {
     28                execMoveSelectionBackwardByCharacterCommand();
     29            }, 100);
     30    }, 100);
    2731}
    2832

  • trunk/ManualTests/autocorrection/remove-misspelling-marker-after-appending-letter.html

    r84448 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>
    … …  
    1717    textarea.focus();
    1818    document.execCommand("InsertText", false, "this brougt");
    19     typeCharacterCommand(' ');
    20     document.execCommand("Delete");
    21     document.execCommand("Delete");
    22     typeCharacterCommand(' ');
    23    
    24     // Now the word "brough" should carry misspelling marker.
    25    
    26     document.execCommand("Delete");
     19    setTimeout(function () {
     20        typeCharacterCommand(' ');
     21        setTimeout(function () {
     22            document.execCommand("Delete");
     23            document.execCommand("Delete");
     24        }, 100);
     25    }, 100);
    2726}
    2827
    … …  
    3231</head>
    3332<body>
    34 <div>The test verified that misspelling marker is removed after appending new letter to misspelled word. You should see the phrase "this brought" without red underline.</div>
    35 <p  style="color:green">Note, this test can fail due to user specific spell checking data. If the user has previously dismissed 'brought' as the correct spelling of 'brougth' several times, the spell checker will not provide 'notational' as a suggestion anymore. To fix this, remove all files in ~/Library/Spelling.</p>
     33<div>The test verified that misspelling marker is removed after accepting a correction. You should see the phrase "this brough" without red underline.</div>
     34<p  style="color:green">Note, this test can fail due to user specific spell checking data. If the user has previously dismissed 'brought' as the correct spelling of 'brougt' several times, the spell checker will not provide 'notational' as a suggestion anymore. To fix this, remove all files in ~/Library/Spelling.</p>
    3635<textarea id="test" cols="80" rows="10"></textarea>
    3736</div>

  • trunk/ManualTests/autocorrection/removing-misspelling-marker-after-appending-letter-2.html

    r201489 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>
    … …  
    1616    textarea = document.getElementById('test');
    1717    textarea.focus();
     18
    1819    document.execCommand("InsertText", false, "this brougt");
    19     typeCharacterCommand(' ');
    20     document.execCommand("Delete");
    21     document.execCommand("Delete");
    22     typeCharacterCommand(' ');
    23    
    24     // Now the word "brough" should carry misspelling marker.
    25    
    26     document.execCommand("Delete");
     20    setTimeout(function () {
     21        typeCharacterCommand(' ');
     22        setTimeout(function () {
     23            document.execCommand("Delete");
     24            document.execCommand("Delete");
     25            setTimeout(function () {
     26                typeCharacterCommand(" ");
     27                document.execCommand("Delete");
     28            }, 100);
     29        }, 100);
     30    }, 100);
    2731}
    2832

  • trunk/ManualTests/autocorrection/resources/document-for-iframe-test.html

    r201489 r201490  
    99</style>
    1010
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/select-from-multiple-guesses.html

    r77577 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>
    … …  
    3535<body>
    3636<div><p>This test verifies that multiple guesses on mispelled word is working correctly</p>
    37 <p>After seeing the panel with multiple candidates, use left and/or right arrow key to select the desired candidate, and then use return key to confirm. You can also use this test to test that pressing ESC key will dismiss the panel.</p>
     37<p>After seeing the panel with multiple candidates, use down arrow key to select the first candidate, and then use left and/or right arrow key to select the desired candidate, and then use return key to confirm. You can also use this test to test that pressing ESC key will dismiss the panel.</p>
    3838<p  style="color:green">Note, this test can fail due to user specific spell checking data. To fix this, remove all files in ~/Library/Spelling, then kill AppleSpell.service process.</p>
    3939<div contenteditable id="root" class="editing">

  • trunk/ManualTests/autocorrection/spell-checking-after-reversion.html

    r86295 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>

  • trunk/ManualTests/autocorrection/type-whitespace-to-dismiss-reversion.html

    r80023 r201490  
    99}
    1010</style>
    11 <script src=../../../../LayoutTests/editing/editing.js language="JavaScript" type="text/JavaScript" ></script>
     11<script src="../../LayoutTests/editing/editing.js"></script>
    1212
    1313<script>
    … …  
    2323    typeCharacterCommand('g');
    2424    typeCharacterCommand('e');
    25     typeCharacterCommand(' ');
    26     execMoveSelectionBackwardByCharacterCommand();
     25    setTimeout(function () {
     26        typeCharacterCommand(' ');
     27        setTimeout(function () {
     28            execMoveSelectionBackwardByCharacterCommand();
     29        }, 100);
     30    }, 100);
    2731}
    2832

  • trunk/ManualTests/autocorrection/undo-autocorrection.html

    r78632 r201490  
    1414<body>
    1515<div><p>This test verifies the behavior of undoing autocorrection.</p>
    16 <p><b>Test 1:</b> Type "the mesage". After seeing the correction panel, type whitespace to accept the correction, then select undo. You should see the second word being reverted to "message".</p>
    17 <p><b>Test 2:</b> Type "the ". Set font to bold, then type "me". Set font to regular, then type "sage". After seeing the correction panel, type whitespace to accept the correction, then select undo. You should see the second word being reverted to "message" with "me" in bold font.</p>
    18 <p  style="color:green">Note, this test can fail due to user specific spell checking data. If the user has previously frequently revert 'message' to 'mesage'. To fix this, remove all files in ~/Library/Spelling, then kill AppleSpell.service process.</p>
    19 <div contenteditable id="root" class="editing">
     16<p>Type "the mesage". After seeing the correction panel, type whitespace to accept the correction, then select undo. You should see the second word being reverted to "message".</p>
     17<div contenteditable id="root1" class="editing">
    2018<span id="test"></span>
    2119</div>
    22 
    2320</body>
    2421</html>

  • trunk/Source/WebCore/ChangeLog

    r201486 r201490  
     12016-05-28  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Autocorrection makes it hard to type "doesn't" and to type @ in email addresses
     4        https://bugs.webkit.org/show_bug.cgi?id=158177
     5        <rdar://problem/20490862>
     6        <rdar://problem/24707954>
     7
     8        Reviewed by Darin Adler.
     9
     10        When the user had typed "doesn'", some unified spellchecker may try to autocorrect it to "doesn't" or "does"
     11        but we should ignore this for a moment until the next character is typed by the user. The code to deal with
     12        this situation which checks the existence of an "ambiguous boundary character" was not robust when the
     13        replacement text was longer than the corrected text.
     14
     15        Fixed this bug by fixing the logic to detect this case. Also added '@' as an ambiguous boundary character
     16        since autocorrecting letters that appear right before '@' would not be useful in many cases.
     17
     18        Tests: ManualTests/autocorrection/autocorrection-at-mark.html
     19               ManualTests/autocorrection/autocorrection-contraction-2.html
     20
     21        * editing/AlternativeTextController.cpp:
     22        (WebCore::AlternativeTextController::timerFired): Fixed a bug that we can show an empty reversion panel.
     23        * editing/Editor.cpp:
     24        (WebCore::Editor::markAndReplaceFor): When the user had typed "doesn'" and our autocorrection result is
     25        "doesn't", resultEndLocation (the end of "doesn't") is larger than selectionOffset (the end of "doesn'").
     26        When the correction is "does", resultEndLocation (the end of "does") is one less than selectionOffset.
     27        Updated the condition to deal with both of these conditions as well as cases where the correction result
     28        contains more than one letter after '.
     29        * editing/htmlediting.cpp:
     30        (WebCore::isAmbiguousBoundaryCharacter): Moved from the header file since this is not a hot function and
     31        doesn't need to be inlined everywhere. Added '@' as an ambiguous boundary character.
     32        * editing/htmlediting.h:
     33        (WebCore::isAmbiguousBoundaryCharacter): Moved to the cpp file.
     34
    1352016-05-28  Konstantin Tokarev  <annulen@yandex.ru>
    236

  • trunk/Source/WebCore/editing/AlternativeTextController.cpp

    r200922 r201490  
    341341        break;
    342342    case AlternativeTextTypeReversion: {
    343         if (!m_alternativeTextInfo.rangeWithAlternative)
     343        auto* details = static_cast<const AutocorrectionAlternativeDetails*>(m_alternativeTextInfo.details.get());
     344        if (!m_alternativeTextInfo.rangeWithAlternative || !details || details->replacementString().isEmpty())
    344345            break;
    345346        m_alternativeTextInfo.isActive = true;
    … …  
    347348        FloatRect boundingBox = rootViewRectForRange(m_alternativeTextInfo.rangeWithAlternative.get());
    348349        if (!boundingBox.isEmpty()) {
    349             const AutocorrectionAlternativeDetails* details = static_cast<const AutocorrectionAlternativeDetails*>(m_alternativeTextInfo.details.get());
    350350            if (AlternativeTextClient* client = alternativeTextClient())
    351351                client->showCorrectionAlternative(m_alternativeTextInfo.type, boundingBox, m_alternativeTextInfo.originalText, details->replacementString(), Vector<String>());

  • trunk/Source/WebCore/editing/Editor.cpp

    r200922 r201490  
    25252525        const int resultEndLocation = resultLocation + resultLength;
    25262526        const String& replacement = results[i].replacement;
    2527         const bool resultEndsAtAmbiguousBoundary = useAmbiguousBoundaryOffset && resultEndLocation == selectionOffset - 1;
     2527        const bool resultEndsAtAmbiguousBoundary = useAmbiguousBoundaryOffset && selectionOffset - 1 <= resultEndLocation;
    25282528
    25292529        // Only mark misspelling if:

  • trunk/Source/WebCore/editing/htmlediting.cpp

    r201205 r201490  
    390390}
    391391
     392bool isAmbiguousBoundaryCharacter(UChar character)
     393{
     394    // These are characters that can behave as word boundaries, but can appear within words.
     395    // If they are just typed, i.e. if they are immediately followed by a caret, we want to delay text checking until the next character has been typed.
     396    // FIXME: this is required until <rdar://problem/6853027> is fixed and text checking can do this for us.
     397    return character == '\'' || character == '@' || character == rightSingleQuotationMark || character == hebrewPunctuationGershayim;
     398}
     399
    392400String stringWithRebalancedWhitespace(const String& string, bool startIsStartOfParagraph, bool endIsEndOfParagraph)
    393401{

  • trunk/Source/WebCore/editing/htmlediting.h

    r200931 r201490  
    216216}
    217217
    218 inline bool isAmbiguousBoundaryCharacter(UChar character)
    219 {
    220     // These are characters that can behave as word boundaries, but can appear within words.
    221     // If they are just typed, i.e. if they are immediately followed by a caret, we want to delay text checking until the next character has been typed.
    222     // FIXME: this is required until 6853027 is fixed and text checking can do this for us.
    223     return character == '\'' || character == rightSingleQuotationMark || character == hebrewPunctuationGershayim;
    224 }
     218bool isAmbiguousBoundaryCharacter(UChar);
    225219
    226220inline bool editingIgnoresContent(const Node& node)
Note: See TracChangeset for help on using the changeset viewer.