Within InsertTextCommand::input, it looks like on every normal keypress, we do one layout. On the first keypress, we will layout twice. On a normal keypress, the progression is normally: insertTextIntoNode(textNode, offset, text) => setNeedsLayout setEndingSelection(Selection(endingSelection().end(), endingSelection().affinity()))) => updateLayout On the first keypress, this is now: startPosition = prepareForTextInsertion(startPosition) => setNeedsLayout removePlaceholderAt(VisiblePosition(startPosition)); => updateLayout then insertTextIntoNode(textNode, offset, text) => setNeedsLayout setEndingSelection(Selection(endingSelection().end(), endingSelection().affinity()))) => updateLayout <rdar://problem/6777744>
We shouldn't create a VisiblePosition when we remove the placeholder, since at that time there is a pending layout that will be triggered by the creation. We should find out if we need to remove a placeholder early in input(), and then remove it after we've prepared the block for insertion. If we remove the placeholder too early, the block will collapse...
Created attachment 29458 [details] patch
Created attachment 29459 [details] patch
Created attachment 29476 [details] patch updated changelog
Comment on attachment 29459 [details] patch You should use the new Position functions. position.node()->hasTagName(brTag) should be anchorNode() and position.m_offset == 0 should be isAtFirstEditingPosition() (or whatever it's called) The other two position.m_offset calls in that function can be position.offsetInContainerNode() calls. Why is: + if (p.node()->hasTagName(brTag)) { needed if you ASSERTED that the position before was a br? also .node() should be .anchorNode() or containerNode() here. Otherwise this looks fine. Is there no way for us to test this? I assume this only affects performance? not any functional change?
Created attachment 29477 [details] patch addresses eric's comments
Comment on attachment 29477 [details] patch Looks fine. Please add a comment to the ChangeLog about why there is no test. :) I can see why you said that you thought offsetInContainerNode() should be renamed offsetInAnchorNode(). Now that containerNode() got renamed computeContainerNode() you end up using anchorNode() instead. Maybe we want a containerNode() accessor which ASSERTS like offsetInContainerNode() does. Anyway, that's a discussion for another bug (or email thread). r+
trac.webkit.org/changeset/42507