Bug 25172 - Extra partial layout during the first keypress in an empty block
Summary: Extra partial layout during the first keypress in an empty block
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-04-13 19:43 PDT by Justin Garcia
Modified: 2009-04-14 11:48 PDT (History)
5 users (show)

See Also:


Attachments
patch (15.05 KB, patch)
2009-04-13 22:17 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (15.14 KB, patch)
2009-04-13 22:19 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (15.07 KB, patch)
2009-04-14 11:08 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (15.24 KB, patch)
2009-04-14 11:35 PDT, Justin Garcia
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 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>
Comment 1 Justin Garcia 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...
Comment 2 Justin Garcia 2009-04-13 22:17:44 PDT
Created attachment 29458 [details]
patch
Comment 3 Justin Garcia 2009-04-13 22:19:26 PDT
Created attachment 29459 [details]
patch
Comment 4 Justin Garcia 2009-04-14 11:08:04 PDT
Created attachment 29476 [details]
patch

updated changelog
Comment 5 Eric Seidel (no email) 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?
Comment 6 Justin Garcia 2009-04-14 11:35:55 PDT
Created attachment 29477 [details]
patch

addresses eric's comments
Comment 7 Eric Seidel (no email) 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+
Comment 8 Justin Garcia 2009-04-14 11:48:58 PDT
trac.webkit.org/changeset/42507