| Summary: | REGRESSION: Crash when deleting text after textarea's value is modified on input event | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Martin Wittemann <martin.wittemann> | ||||||||||||
| Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | adele, ap, darin, enrica, jonnytrap, justin.garcia, leviw, mjs, ojan, rniwa, tkent, tony | ||||||||||||
| Priority: | P1 | Keywords: | Regression | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Martin Wittemann
2010-11-23 02:17:17 PST
Is this really a regression? Created attachment 75307 [details]
reduction
Doesn't crash Safari 5.0.2. (In reply to comment #3) > Doesn't crash Safari 5.0.2. I see. Thanks for the clarification. The problem appears to be that the endingSelection() and form's selections are pointing at different nodes. Namely, in enabledDelete, selection returned by frame->editor()->selectionForCommand is pointing at new text node replaced by the input event handler ("b" in my reduced test case) but endingSelection used in TypingCommand::deleteKeyPressed is pointing at the old node ("a" in my reduced test case). The problem is that EditCommand::endingSelection() is just retrieving the stored value. We should update m_endingSelection or stop using it whenever we invoke the last edit command's m_endingSelection may be out of date. I'm not even sure if we should be using the last typing command if the selection has changed. It seems like we should create a new typing command. Consider the following case: 1. User type "hello " into input element, and delete the last space. 2. Script modifies it to "world" 3. User undo In this case, undo isn't even going to work. Created attachment 75328 [details]
fixes the crash
(In reply to comment #7) > Created an attachment (id=75328) [details] > fixes the crash I'm not sure if this is the correct fix for the problem because always updating the selection breaks undo. As I wrote on the previous comment, I feel like we should be closing the typing command when we're updating the selection in the event handler. However, setSelection is called before the typing command corresponding to InsertLineBreak concludes, and this prevents us from closing the type command because the command hasn't been added to the undo stack. Furthermore, in some cases, we need to let typing command and its child commands update selection without closing itself. Could someone familiar with typing command comment on this issue? Comment on attachment 75328 [details] fixes the crash View in context: https://bugs.webkit.org/attachment.cgi?id=75328&action=review I’m not sure this fix is quite right. It seems to me that a node involved in an editing operation might be removed for multiple reasons. Maybe there’s a better way to cope with it than changing the selection. > WebCore/editing/TypingCommand.cpp:97 > + VisibleSelection lastSelection = lastTypingCommand->endingSelection(); > + VisibleSelection currentSelection = frame->selection()->selection(); > + if (lastSelection != currentSelection) { I don’t think you need the lastSelection local here. Might read better without it. (In reply to comment #9) > I’m not sure this fix is quite right. It seems to me that a node involved in an editing operation might be removed for multiple reasons. Right. But because those changes happen in the event handler, there are basically two options: 1. Update the selection of the previous typing command 2. Start a new typing command; i.e. close the last typing command. But I'm not sure what is the correct timing to close the typing command if we chose option 2. I chose option 1 because there's already code that does very similar thing in TypingCommand::inputText. > > WebCore/editing/TypingCommand.cpp:97 > > + VisibleSelection lastSelection = lastTypingCommand->endingSelection(); > > + VisibleSelection currentSelection = frame->selection()->selection(); > > + if (lastSelection != currentSelection) { > > I don’t think you need the lastSelection local here. Might read better without it. Good point. Will fix later. The similar selection changes made in TypingCommand::insertText is made by http://trac.webkit.org/changeset/19313. (In reply to comment #11) > The similar selection changes made in TypingCommand::insertText is made by http://trac.webkit.org/changeset/19313. This change is addressing a slightly different issue though, which is to use the selection for insertion when it differs from what selection controller has. But I can't think of why this should ever be the case because we update the frame's selection after modifying form text control's selection range. Could someone tell me how this condition may arise? Created attachment 75438 [details]
demo for insertText case
The same problem exists for insertText as well, and here's a demo. We can fix this bug by the following change:
Index: WebCore/editing/InsertTextCommand.h
===================================================================
--- WebCore/editing/InsertTextCommand.h (revision 73113)
+++ WebCore/editing/InsertTextCommand.h (working copy)
@@ -54,6 +54,8 @@
bool performTrivialReplace(const String&, bool selectInsertedText);
unsigned m_charactersAdded;
+
+ friend class TypingCommand;
};
} // namespace WebCore
Index: WebCore/editing/TypingCommand.cpp
===================================================================
--- WebCore/editing/TypingCommand.cpp (revision 73113)
+++ WebCore/editing/TypingCommand.cpp (working copy)
@@ -163,12 +163,12 @@
RefPtr<EditCommand> lastEditCommand = frame->editor()->lastEditCommand();
if (isOpenForMoreTypingCommand(lastEditCommand.get())) {
TypingCommand* lastTypingCommand = static_cast<TypingCommand*>(lastEditCommand.get());
- if (changeSelection) {
+ if (lastTypingCommand->endingSelection() != selectionForInsertion) {
lastTypingCommand->setStartingSelection(selectionForInsertion);
lastTypingCommand->setEndingSelection(selectionForInsertion);
}
lastTypingCommand->insertText(newText, selectInsertedText);
- if (changeSelection) {
+ if (lastTypingCommand->endingSelection() != selectionForInsertion) {
lastTypingCommand->setEndingSelection(currentSelection);
frame->selection()->setSelection(currentSelection);
}
@@ -371,6 +371,10 @@
command = InsertTextCommand::create(document());
applyCommandToComposite(command);
}
+ if (endingSelection() != command->endingSelection()) {
+ command->setStartingSelection(endingSelection());
+ command->setEndingSelection(endingSelection());
+ }
command->input(text, selectInsertedText);
typingAddedToOpenCommand(InsertText);
}
Created attachment 75469 [details]
fixes the bug for good
Comment on attachment 75469 [details]
fixes the bug for good
Seems OK. It’s not good that the selection handling is so intimately tied in with the editing commands; at some point I think we can improve this greatly by refactoring and breaking the selection management away from the DOM mutation itself.
(In reply to comment #15) > (From update of attachment 75469 [details]) > Seems OK. It’s not good that the selection handling is so intimately tied in with the editing commands; at some point I think we can improve this greatly by refactoring and breaking the selection management away from the DOM mutation itself. Yeah, we need a better way of managing position in editing commands instead of using selection. Ideally, we can make editing commands agnostic of what current selection is except when they're first instantiated. Thanks for the review, Darin. I'll be landing it shortly. Committed r73279: <http://trac.webkit.org/changeset/73279> |