Bug 49744

Summary: Undo moves caret to invalid position
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: HTML EditingAssignee: Levi Weintraub <leviw>
Status: ASSIGNED ---    
Severity: Normal CC: abarth, commit-queue, darin, eric, leviw, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.6   
Bug Depends on: 55886    
Bug Blocks:    
Attachments:
Description Flags
Undo moves caret
none
Proposed patch
none
Patch
none
Patch for landing
none
Patch
none
Patch for landing
commit-queue: commit-queue-
regression demo none

Description Levi Weintraub 2010-11-18 10:33:08 PST
Created attachment 74253 [details]
Undo moves caret

Pasting text into a contenteditable element then undoing the paste causes the caret to be moved semi-permanently to an invalid position before the padding (and within the border) of the editable element.

Steps to reproduce:
1) Open the attached document in Safari or Chrome (tested with Chrome 9 and Safari 5.0.2)
2) Highlight and copy the text at the top
3) Place the caret in the editable content -- note the position of the caret
4) Paste the text.
5) Undo the paste.

Expected result: Caret is back where it was at the beginning

Actual result: Caret has jumped to within the div's border!
Comment 1 Levi Weintraub 2010-11-18 12:04:23 PST
In the fail case, the DIV has an empty RenderText as a child. This causes RenderBlock::localCaretRect to follow a different path, using RenderBox::localCaretRect which has this at the beginning -- FIXME: What about border and padding?

What indeed :)
Comment 2 Levi Weintraub 2010-11-18 14:53:47 PST
Created attachment 74293 [details]
Proposed patch
Comment 3 Darin Adler 2010-11-18 15:13:42 PST
Comment on attachment 74293 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74293&action=review

> WebCore/rendering/RenderBox.cpp:2926
> -    // FIXME: What about border and padding?
> -    IntRect rect(x(), y(), caretWidth, height());
> +    IntRect rect(x() + borderLeft() + paddingLeft(), y() + borderTop() + paddingTop(), caretWidth, height());
>      bool ltr = box ? box->isLeftToRightDirection() : style()->isLeftToRightDirection();

This won’t do the right thing in RTL cases. We need RTL coverage in a test case, and code that adds the appropriate right side border and padding.

> LayoutTests/editing/selection/caret-painting-after-paste-undo.html:22
>  \ No newline at end of file

Normally we put newlines at the ends of files like this one.
Comment 4 Levi Weintraub 2010-11-18 16:34:58 PST
Created attachment 74320 [details]
Patch
Comment 5 Levi Weintraub 2010-11-18 16:45:16 PST
Thanks for the feedback! I think I managed to address your concerns.
Comment 6 Levi Weintraub 2010-11-22 12:19:53 PST
Created attachment 74582 [details]
Patch for landing
Comment 7 Darin Adler 2010-11-22 13:43:54 PST
Setting commit-queue+ only works if you area committer. If you are not a committer, you should set commit-queue? and then a committer will change it to +.
Comment 8 WebKit Commit Bot 2010-11-22 21:40:38 PST
Comment on attachment 74582 [details]
Patch for landing

Rejecting patch 74582 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
......................................................................................................................................................................................................................................................
editing/deleting ...........
editing/deleting/4922367.html -> failed

Exiting early after 1 failures. 4365 tests run.
82.86s total testing time

4364 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6355009
Comment 9 Levi Weintraub 2010-11-29 11:27:42 PST
Something doesn't seem right here. This change only affects rendering, and should therefor not break layout. I suspect flakiness.
Comment 10 Eric Seidel (no email) 2010-12-14 01:32:14 PST
Comment on attachment 74582 [details]
Patch for landing

I doubt it's flakiness, but we can try again.  The cq has a bunch of code to detect flaky tests.
Comment 11 WebKit Commit Bot 2010-12-14 03:12:34 PST
Comment on attachment 74582 [details]
Patch for landing

Rejecting attachment 74582 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
......................................................................................................................................................................................................................................................
editing/deleting ...........
editing/deleting/4922367.html -> failed

Exiting early after 1 failures. 4366 tests run.
72.18s total testing time

4365 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6936128
Comment 12 Eric Seidel (no email) 2010-12-14 15:12:31 PST
Attachment 74582 [details] was posted by a committer and has review+, assigning to Levi Weintraub for commit.
Comment 13 Levi Weintraub 2011-01-05 15:16:19 PST
Created attachment 78053 [details]
Patch
Comment 14 Levi Weintraub 2011-01-05 15:18:56 PST
(In reply to comment #13)
> Created an attachment (id=78053) [details]
> Patch

Positions before and after Table elements shouldn't consider the Table's border and padding, as they're not considered inside the Table in editing (unlike all other positions at the beginning and end up RenderBlocks). Fixing the original patch to only add border and padding when not dealing with Tables.
Comment 15 Ryosuke Niwa 2011-01-05 15:24:43 PST
Comment on attachment 78053 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78053&action=review

> WebCore/rendering/RenderBox.cpp:2951
> +    if (isTable()) {
> +        if ((!caretOffset) ^ ltr)
> +            rect.move(IntSize(width() - caretWidth, 0));
> +    } else if ((!caretOffset) ^ ltr)

Wait, I thought we discussed that we should fix the selection rather than taking care of it in rendering side?  Was it not possible in this patch?
Comment 16 Levi Weintraub 2011-01-05 15:27:56 PST
(In reply to comment #15)
> (From update of attachment 78053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78053&action=review
> 
> > WebCore/rendering/RenderBox.cpp:2951
> > +    if (isTable()) {
> > +        if ((!caretOffset) ^ ltr)
> > +            rect.move(IntSize(width() - caretWidth, 0));
> > +    } else if ((!caretOffset) ^ ltr)
> 
> Wait, I thought we discussed that we should fix the selection rather than taking care of it in rendering side?  Was it not possible in this patch?

I gave that the old college try, but creating new positions at [Table, PositionBeforeAnchor] and [Parent, offsetOfTable] end up with the wrong rendering plus introducing other editing bugs :(
Comment 17 Levi Weintraub 2011-01-25 12:17:24 PST
Committed r76625: <http://trac.webkit.org/changeset/76625>
Comment 18 Levi Weintraub 2011-01-25 13:29:36 PST
Created attachment 80111 [details]
Patch for landing
Comment 19 Levi Weintraub 2011-01-27 14:38:25 PST
Committed r76839: <http://trac.webkit.org/changeset/76839>
Comment 20 WebKit Review Bot 2011-01-27 14:54:06 PST
http://trac.webkit.org/changeset/76839 might have broken Chromium Mac Release
Comment 21 Ryosuke Niwa 2011-01-27 19:11:09 PST
Created attachment 80401 [details]
regression demo

This caused a regression.  Take a look at the attached png.  This is the screen shot of TOT WebKit rendering editing/pasteboard/input-field-1.html.  The caret is placed AFTER text field but rendered as if it's inside overlapping on input element.  This can't be right.  I'm sorry but I'm rolling out the patch again.
Comment 22 Ryosuke Niwa 2011-01-27 19:30:39 PST
Committed r76911: <http://trac.webkit.org/changeset/76911>
Comment 23 Levi Weintraub 2011-01-31 15:52:19 PST
The regression Ryosuke noted is the result of certain legacy positions. In this case, we have a position at [INPUT, 1] inside an editable DIV. This is meant to represent a position "after" the input, but this rendering code actually has no way of knowing that's what this means!

Here's an example to illustrate the problem:

<div contenteditable=true>
   <span>
      <pre contenteditable=false>Foo</pre>
   </span>
</div>

Editing code would create a position at [SPAN, 1] to represent the position directly following the non-editable PRE element (to respect editability), but it would *also* use this to represent the position directly following the SPAN. If the SPAN has border and padding, it's impossible for the rendering code to know which position is correct without our "new" positions. Therefor I'm adding 52099 as a blocking bug, but for the record, I think in its current state this still fixes more than it breaks.
Comment 24 WebKit Commit Bot 2011-01-31 18:56:38 PST
Comment on attachment 80111 [details]
Patch for landing

Rejecting attachment 80111 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 2

Last 500 characters of output:
-mac/fast/text/justify-ideograph-simple-expected.png
	M	LayoutTests/ChangeLog
r77190 = 887765de93f4e24ddcd1537a90f9c1cef99d93bd (refs/remotes/trunk)
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/rendering/RenderObject.h
	M	Source/WebCore/rendering/RenderObjectChildList.cpp
	M	Source/WebCore/rendering/RenderObjectChildList.h
r77191 = bf291a4df693f9fa6fd5a7afb2cd3c686380ed5d (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/7680675
Comment 25 Levi Weintraub 2011-03-07 11:27:20 PST
Created bug 55886 that better represents the dependency to land this properly.