Bug 25172 – Extra partial layout during the first keypress in an empty block

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Justin Garcia]

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>

[Justin Garcia]

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]

Created attachment 29458 [details]
patch

[Justin Garcia]

Created attachment 29459 [details]
patch

[Justin Garcia]

Created attachment 29476 [details]
patch

updated changelog

[Eric Seidel (no email)]

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]

Created attachment 29477 [details]
patch

addresses eric's comments

[Eric Seidel (no email)]

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]

trac.webkit.org/changeset/42507