Bug 137712 – registerUndoWithTarget: leaves undo chain out of order

• Component: WebKit API
• Status: NEW

[David Gatwood]

The WebKit implementation of registerUndoWithTarget: can leave the undo chain out of order in certain situations.


Steps to reproduce:

1.  Create an application action that:
    a. obtains the current selection
    b. inserts a new element at the current position
    c. places the insertion point inside that new element by calling setSelectedDOMRange:affinity:
    d. calls registerUndoWithTarget: to register an undo handler to destroy the element.
2.  Perform the action, then type text.
3.  Hit undo.


Expected results:

I expected the first undo to remove the text, and the second undo to remove the element.


Actual results:

The first undo removes the element, and the second undo removes the text.


Additional information:

If I call [webview moveRight:] prior to calling setSelectedDOMRange:affinity:, the problem goes away, so the problem is caused by neither setSelectedDOMRange:affinity: nor registerUndoWithTarget: properly closing out any in-progress text editing actions.  I'm inclined to suggest that this should happen automatically when you register an undo action, prior to registering the new undo handler, because I suspect in-progress text editing actions should always get closed out in that situation, whereas ostensibly it may or may not be appropriate to do so when changing the current selection, depending on why you're changing the selection.


Example snippet to reproduce:

- (DOMElement *)wrapOrInsertElementWithElement:(NSString *)tagName
{
    DOMDocument *document = [[self.webView mainFrame] DOMDocument];
    DOMRange *selRange = [self.webView selectedDOMRange];
    
    DOMElement *newElt = [document createElement:tagName];
    NSString *uuid = [self ElementUUID];
    [newElt setAttribute:@"id" value:uuid];
    
    if ([selRange collapsed]) {
        /* It's an insertion point.  Just insert the node. */
        [selRange insertNode:newElt];
        [selRange setStart:newElt offset:0];
        
        /* setSelectedDOMRange doesn't properly close out any existing text editing
           undo chain entries, so if we don't do an explicit move, the undo chain
           ends up out of order. This isn't ideal, and might even be completely wrong
           in RTL content (not sure), but it works well enough for now.
         */
        [self.webView moveRight:self];
        [self.webView setSelectedDOMRange:selRange affinity:NSSelectionAffinityDownstream];
    } else {
        [selRange surroundContents:newElt];
    }
    
    [[self.webView undoManager] registerUndoWithTarget:self selector:@selector(unwrapElementWithXPath:) object:[self XPathForElement:newElt]];

    [self synchronizeData];
    
    return newElt;
}

[David Gatwood]

Just to clarify, the above snippet contains the workaround.  Remove the call to moveRight: to reproduce.