Changeset 192641 in webkit


Ignore:
Timestamp:
Nov 19, 2015 11:29:59 AM (8 years ago)
Author:
BJ Burg
Message:

REGRESSION(r8780): Backwards delete by word incorrectly appends deleted text to kill ring, should be prepend
​https://bugs.webkit.org/show_bug.cgi?id=151300

Reviewed by Darin Adler.

Source/WebCore:

Over 11 years ago, someone was in a big hurry to fix a bunch
of emacs keybindings bugs, and accidentally regressed the kill ring
behavior for backwards-delete-word. It should prepend to the beginning.

This patch fixes the regression and cleans up the kill ring-related
code in Editor and commands. It also adds some tests to cover the
regressed code a bit better.

Tests: editing/pasteboard/emacs-killring-alternating-append-prepend.html

editing/pasteboard/emacs-killring-backward-delete-prepend.html

  • editing/Editor.cpp:

Use more explicit names for insertion mode parameters and member variables.

(WebCore::Editor::deleteWithDirection):
(WebCore::Editor::performDelete):
(WebCore::Editor::addRangeToKillRing):
(WebCore::Editor::addTextToKillRing):

Only one call site for now, but another will be added in a dependent fix.

(WebCore::Editor::addToKillRing): Deleted.

  • editing/Editor.h:
  • editing/TypingCommand.cpp:

(WebCore::TypingCommand::TypingCommand):
(WebCore::TypingCommand::deleteKeyPressed):
(WebCore::TypingCommand::forwardDeleteKeyPressed):
(WebCore::TypingCommand::doApply):

  • editing/TypingCommand.h:
  • platform/mac/KillRingMac.mm:

(WebCore::KillRing::append):
(WebCore::KillRing::prepend):

It turns out that the native API implicitly clears the kill sequence when
alternating between prepend and append operations. Its behavior does not match
what Sublime Text or Emacs do in this case. Clear the previous operation flag
to prevent this behavior from happening.

LayoutTests:

  • editing/pasteboard/emacs-killring-alternating-append-prepend-expected.txt: Added.
  • editing/pasteboard/emacs-killring-alternating-append-prepend.html: Added.
  • editing/pasteboard/emacs-killring-backward-delete-prepend-expected.txt: Added.
  • editing/pasteboard/emacs-killring-backward-delete-prepend.html: Added.
Location:
trunk
Files:
4 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r192639 r192641  
     12015-11-19  Brian Burg  <bburg@apple.com>
     2
     3        REGRESSION(r8780): Backwards delete by word incorrectly appends deleted text to kill ring, should be prepend
     4        https://bugs.webkit.org/show_bug.cgi?id=151300
     5
     6        Reviewed by Darin Adler.
     7
     8        * editing/pasteboard/emacs-killring-alternating-append-prepend-expected.txt: Added.
     9        * editing/pasteboard/emacs-killring-alternating-append-prepend.html: Added.
     10        * editing/pasteboard/emacs-killring-backward-delete-prepend-expected.txt: Added.
     11        * editing/pasteboard/emacs-killring-backward-delete-prepend.html: Added.
     12
    1132015-11-19  Myles C. Maxfield  <mmaxfield@apple.com>
    214

  • trunk/Source/WebCore/ChangeLog

    r192639 r192641  
     12015-11-19  Brian Burg  <bburg@apple.com>
     2
     3        REGRESSION(r8780): Backwards delete by word incorrectly appends deleted text to kill ring, should be prepend
     4        https://bugs.webkit.org/show_bug.cgi?id=151300
     5
     6        Reviewed by Darin Adler.
     7
     8        Over 11 years ago, someone was in a big hurry to fix a bunch
     9        of emacs keybindings bugs, and accidentally regressed the kill ring
     10        behavior for backwards-delete-word. It should prepend to the beginning.
     11
     12        This patch fixes the regression and cleans up the kill ring-related
     13        code in Editor and commands. It also adds some tests to cover the
     14        regressed code a bit better.
     15
     16        Tests: editing/pasteboard/emacs-killring-alternating-append-prepend.html
     17               editing/pasteboard/emacs-killring-backward-delete-prepend.html
     18
     19        * editing/Editor.cpp:
     20
     21            Use more explicit names for insertion mode parameters and member variables.
     22
     23        (WebCore::Editor::deleteWithDirection):
     24        (WebCore::Editor::performDelete):
     25        (WebCore::Editor::addRangeToKillRing):
     26        (WebCore::Editor::addTextToKillRing):
     27
     28            Only one call site for now, but another will be added in a dependent fix.
     29
     30        (WebCore::Editor::addToKillRing): Deleted.
     31        * editing/Editor.h:
     32        * editing/TypingCommand.cpp:
     33        (WebCore::TypingCommand::TypingCommand):
     34        (WebCore::TypingCommand::deleteKeyPressed):
     35        (WebCore::TypingCommand::forwardDeleteKeyPressed):
     36        (WebCore::TypingCommand::doApply):
     37        * editing/TypingCommand.h:
     38        * platform/mac/KillRingMac.mm:
     39        (WebCore::KillRing::append):
     40        (WebCore::KillRing::prepend):
     41
     42            It turns out that the native API implicitly clears the kill sequence when
     43            alternating between prepend and append operations. Its behavior does not match
     44            what Sublime Text or Emacs do in this case. Clear the previous operation flag
     45            to prevent this behavior from happening.
     46
    1472015-11-19  Myles C. Maxfield  <mmaxfield@apple.com>
    248

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

    r192354 r192641  
    331331}
    332332
    333 bool Editor::deleteWithDirection(SelectionDirection direction, TextGranularity granularity, bool killRing, bool isTypingAction)
     333bool Editor::deleteWithDirection(SelectionDirection direction, TextGranularity granularity, bool shouldAddToKillRing, bool isTypingAction)
    334334{
    335335    if (!canEdit())
    … …  
    341341            revealSelectionAfterEditingOperation();
    342342        } else {
    343             if (killRing)
    344                 addToKillRing(selectedRange().get(), false);
     343            if (shouldAddToKillRing)
     344                addRangeToKillRing(*selectedRange().get(), KillRingInsertionMode::AppendText);
    345345            deleteSelectionWithSmartDelete(canSmartCopyOrDelete());
    346346            // Implicitly calls revealSelectionAfterEditingOperation().
    … …  
    350350        if (canSmartCopyOrDelete())
    351351            options |= TypingCommand::SmartDelete;
    352         if (killRing)
    353             options |= TypingCommand::KillRing;
     352        if (shouldAddToKillRing)
     353            options |= TypingCommand::AddsToKillRing;
    354354        switch (direction) {
    355355        case DirectionForward:
    … …  
    368368    // clear the "start new kill ring sequence" setting, because it was set to true
    369369    // when the selection was updated by deleting the range
    370     if (killRing)
     370    if (shouldAddToKillRing)
    371371        setStartNewKillRingSequence(false);
    372372
    … …  
    10961096    : m_frame(frame)
    10971097    , m_ignoreCompositionSelectionChange(false)
    1098     , m_shouldStartNewKillRingSequence(false)
    10991098    // This is off by default, since most editors want this behavior (this matches IE but not FF).
    11001099    , m_shouldStyleWithCSS(false)
    … …  
    13351334    }
    13361335
    1337     addToKillRing(selectedRange().get(), false);
     1336    addRangeToKillRing(*selectedRange().get(), KillRingInsertionMode::AppendText);
    13381337    deleteSelectionWithSmartDelete(canSmartCopyOrDelete());
    13391338
    … …  
    28482847}
    28492848
    2850 void Editor::addToKillRing(Range* range, bool prepend)
     2849void Editor::addRangeToKillRing(const Range& range, KillRingInsertionMode mode)
     2850{
     2851    addTextToKillRing(plainText(&range), mode);
     2852}
     2853
     2854void Editor::addTextToKillRing(const String& text, KillRingInsertionMode mode)
    28512855{
    28522856    if (m_shouldStartNewKillRingSequence)
    28532857        killRing().startNewSequence();
    28542858
    2855     String text = plainText(range);
    2856     if (prepend)
     2859    m_shouldStartNewKillRingSequence = false;
     2860
     2861    // If the kill was from a backwards motion, prepend to the kill ring.
     2862    // This will ensure that alternating forward and backward kills will
     2863    // build up the original string in the kill ring without permuting it.
     2864    switch (mode) {
     2865    case KillRingInsertionMode::PrependText:
    28572866        killRing().prepend(text);
    2858     else
     2867        break;
     2868    case KillRingInsertionMode::AppendText:
    28592869        killRing().append(text);
    2860     m_shouldStartNewKillRingSequence = false;
     2870        break;
     2871    }
    28612872}
    28622873

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

    r191671 r192641  
    316316    bool ignoreCompositionSelectionChange() const { return m_ignoreCompositionSelectionChange; }
    317317
    318     void setStartNewKillRingSequence(bool);
    319 
    320318    WEBCORE_EXPORT PassRefPtr<Range> rangeForPoint(const IntPoint& windowPoint);
    321319
    … …  
    339337#endif
    340338   
    341     void addToKillRing(Range*, bool prepend);
     339    enum class KillRingInsertionMode { PrependText, AppendText };
     340    void addRangeToKillRing(const Range&, KillRingInsertionMode);
     341    void addTextToKillRing(const String&, KillRingInsertionMode);
     342    void setStartNewKillRingSequence(bool);
    342343
    343344    void startAlternativeTextUITimer();
    … …  
    502503    Vector<CompositionUnderline> m_customCompositionUnderlines;
    503504    bool m_ignoreCompositionSelectionChange;
    504     bool m_shouldStartNewKillRingSequence;
     505    bool m_shouldStartNewKillRingSequence {false};
    505506    bool m_shouldStyleWithCSS;
    506507    const std::unique_ptr<KillRing> m_killRing;

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

    r183368 r192641  
    8484    , m_granularity(granularity)
    8585    , m_compositionType(compositionType)
    86     , m_killRing(options & KillRing)
     86    , m_shouldAddToKillRing(options & AddsToKillRing)
    8787    , m_openedByBackwardDelete(false)
    8888    , m_shouldRetainAutocorrectionIndicator(options & RetainAutocorrectionIndicator)
    … …  
    115115            updateSelectionIfDifferentFromCurrentSelection(lastTypingCommand.get(), document.frame());
    116116            lastTypingCommand->setShouldPreventSpellChecking(options & PreventSpellChecking);
    117             lastTypingCommand->deleteKeyPressed(granularity, options & KillRing);
     117            lastTypingCommand->deleteKeyPressed(granularity, options & AddsToKillRing);
    118118            return;
    119119        }
    … …  
    131131            updateSelectionIfDifferentFromCurrentSelection(lastTypingCommand.get(), frame);
    132132            lastTypingCommand->setShouldPreventSpellChecking(options & PreventSpellChecking);
    133             lastTypingCommand->forwardDeleteKeyPressed(granularity, options & KillRing);
     133            lastTypingCommand->forwardDeleteKeyPressed(granularity, options & AddsToKillRing);
    134134            return;
    135135        }
    … …  
    264264        return;
    265265    case DeleteKey:
    266         deleteKeyPressed(m_granularity, m_killRing);
     266        deleteKeyPressed(m_granularity, m_shouldAddToKillRing);
    267267        return;
    268268    case ForwardDeleteKey:
    269         forwardDeleteKeyPressed(m_granularity, m_killRing);
     269        forwardDeleteKeyPressed(m_granularity, m_shouldAddToKillRing);
    270270        return;
    271271    case InsertLineBreak:
    … …  
    434434}
    435435
    436 void TypingCommand::deleteKeyPressed(TextGranularity granularity, bool killRing)
     436void TypingCommand::deleteKeyPressed(TextGranularity granularity, bool shouldAddToKillRing)
    437437{
    438438    Frame& frame = this->frame();
    … …  
    459459        selection.setSelection(endingSelection());
    460460        selection.modify(FrameSelection::AlterationExtend, DirectionBackward, granularity);
    461         if (killRing && selection.isCaret() && granularity != CharacterGranularity)
     461        if (shouldAddToKillRing && selection.isCaret() && granularity != CharacterGranularity)
    462462            selection.modify(FrameSelection::AlterationExtend, DirectionBackward, CharacterGranularity);
    463463
    … …  
    531531        return;
    532532   
    533     if (killRing)
    534         frame.editor().addToKillRing(selectionToDelete.toNormalizedRange().get(), false);
     533    if (shouldAddToKillRing)
     534        frame.editor().addRangeToKillRing(*selectionToDelete.toNormalizedRange().get(), Editor::KillRingInsertionMode::PrependText);
    535535    // Make undo select everything that has been deleted, unless an undo will undo more than just this deletion.
    536536    // FIXME: This behaves like TextEdit except for the case where you open with text insertion and then delete
    … …  
    543543}
    544544
    545 void TypingCommand::forwardDeleteKeyPressed(TextGranularity granularity, bool killRing)
     545void TypingCommand::forwardDeleteKeyPressed(TextGranularity granularity, bool shouldAddToKillRing)
    546546{
    547547    Frame& frame = this->frame();
    … …  
    566566        selection.setSelection(endingSelection());
    567567        selection.modify(FrameSelection::AlterationExtend, DirectionForward, granularity);
    568         if (killRing && selection.isCaret() && granularity != CharacterGranularity)
     568        if (shouldAddToKillRing && selection.isCaret() && granularity != CharacterGranularity)
    569569            selection.modify(FrameSelection::AlterationExtend, DirectionForward, CharacterGranularity);
    570570
    … …  
    629629        return;
    630630       
    631     if (killRing)
    632         frame.editor().addToKillRing(selectionToDelete.toNormalizedRange().get(), false);
     631    if (shouldAddToKillRing)
     632        frame.editor().addRangeToKillRing(*selectionToDelete.toNormalizedRange().get(), Editor::KillRingInsertionMode::AppendText);
    633633    // make undo select what was deleted
    634634    setStartingSelection(selectionAfterUndo);

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

    r177733 r192641  
    5151    enum Option {
    5252        SelectInsertedText = 1 << 0,
    53         KillRing = 1 << 1,
     53        AddsToKillRing = 1 << 1,
    5454        RetainAutocorrectionIndicator = 1 << 2,
    5555        PreventSpellChecking = 1 << 3,
    … …  
    7676    void insertParagraphSeparatorInQuotedContent();
    7777    void insertParagraphSeparator();
    78     void deleteKeyPressed(TextGranularity, bool killRing);
    79     void forwardDeleteKeyPressed(TextGranularity, bool killRing);
     78    void deleteKeyPressed(TextGranularity, bool shouldAddToKillRing);
     79    void forwardDeleteKeyPressed(TextGranularity, bool shouldAddToKillRing);
    8080    void deleteSelection(bool smartDelete);
    8181    void setCompositionType(TextCompositionType type) { m_compositionType = type; }
    … …  
    132132    TextGranularity m_granularity;
    133133    TextCompositionType m_compositionType;
    134     bool m_killRing;
     134    bool m_shouldAddToKillRing;
    135135    bool m_preservesTypingStyle;
    136136   

  • trunk/Source/WebCore/platform/mac/KillRingMac.mm

    r161589 r192641  
    3939void _NSNewKillRingSequence();
    4040void _NSSetKillRingToYankedState();
     41void _NSResetKillRingOperationFlag();
    4142
    4243}
    … …  
    5455{
    5556    initializeKillRingIfNeeded();
     57    // Necessary to prevent an implicit new sequence if the previous command was NSPrependToKillRing.
     58    _NSResetKillRingOperationFlag();
    5659    _NSAppendToKillRing(string);
    5760}
    … …  
    6063{
    6164    initializeKillRingIfNeeded();
     65    // Necessary to prevent an implicit new sequence if the previous command was NSAppendToKillRing.
     66    _NSResetKillRingOperationFlag();
    6267    _NSPrependToKillRing(string);
    6368}
Note: See TracChangeset for help on using the changeset viewer.