RESOLVED FIXED 25172
Extra partial layout during the first keypress in an empty block
https://bugs.webkit.org/show_bug.cgi?id=25172
Summary Extra partial layout during the first keypress in an empty block
Justin Garcia
Reported 2009-04-13 19:43:28 PDT
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>
Attachments
patch (15.05 KB, patch)
2009-04-13 22:17 PDT, Justin Garcia
no flags
patch (15.14 KB, patch)
2009-04-13 22:19 PDT, Justin Garcia
no flags
patch (15.07 KB, patch)
2009-04-14 11:08 PDT, Justin Garcia
no flags
patch (15.24 KB, patch)
2009-04-14 11:35 PDT, Justin Garcia
eric: review+
Justin Garcia
Comment 1 2009-04-13 19:45:27 PDT
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...
Justin Garcia
Comment 2 2009-04-13 22:17:44 PDT
Justin Garcia
Comment 3 2009-04-13 22:19:26 PDT
Justin Garcia
Comment 4 2009-04-14 11:08:04 PDT
Created attachment 29476 [details] patch updated changelog
Eric Seidel (no email)
Comment 5 2009-04-14 11:11:54 PDT
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?
Justin Garcia
Comment 6 2009-04-14 11:35:55 PDT
Created attachment 29477 [details] patch addresses eric's comments
Eric Seidel (no email)
Comment 7 2009-04-14 11:43:16 PDT
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+
Justin Garcia
Comment 8 2009-04-14 11:48:58 PDT
trac.webkit.org/changeset/42507
Note You need to log in before you can comment on or make changes to this bug.