Bug 49744 – Undo moves caret to invalid position

• Status: ASSIGNED
• Component: HTML Editing

[Levi Weintraub]

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!

[Levi Weintraub]

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 :)

[Levi Weintraub]

Created attachment 74293 [details]
Proposed patch

[Darin Adler]

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.

[Levi Weintraub]

Created attachment 74320 [details]
Patch

[Levi Weintraub]

Thanks for the feedback! I think I managed to address your concerns.

[Levi Weintraub]

Created attachment 74582 [details]
Patch for landing

[Darin Adler]

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 +.

[WebKit Commit Bot]

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

[Levi Weintraub]

Something doesn't seem right here. This change only affects rendering, and should therefor not break layout. I suspect flakiness.

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Attachment 74582 [details] was posted by a committer and has review+, assigning to Levi Weintraub for commit.

[Levi Weintraub]

Created attachment 78053 [details]
Patch

[Levi Weintraub]

(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.

[Ryosuke Niwa]

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?

[Levi Weintraub]

(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 :(

[Levi Weintraub]

Committed r76625: <http://trac.webkit.org/changeset/76625>

[Levi Weintraub]

Created attachment 80111 [details]
Patch for landing

[Levi Weintraub]

Committed r76839: <http://trac.webkit.org/changeset/76839>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76839 might have broken Chromium Mac Release

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r76911: <http://trac.webkit.org/changeset/76911>

[Levi Weintraub]

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.

[WebKit Commit Bot]

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

[Levi Weintraub]

Created bug 55886 that better represents the dependency to land this properly.