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!
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 :)
Created attachment 74293 [details] Proposed patch
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.
Created attachment 74320 [details] Patch
Thanks for the feedback! I think I managed to address your concerns.
Created attachment 74582 [details] Patch for landing
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 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
Something doesn't seem right here. This change only affects rendering, and should therefor not break layout. I suspect flakiness.
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 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
Attachment 74582 [details] was posted by a committer and has review+, assigning to Levi Weintraub for commit.
Created attachment 78053 [details] Patch
(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 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?
(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 :(
Committed r76625: <http://trac.webkit.org/changeset/76625>
Created attachment 80111 [details] Patch for landing
Committed r76839: <http://trac.webkit.org/changeset/76839>
http://trac.webkit.org/changeset/76839 might have broken Chromium Mac Release
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.
Committed r76911: <http://trac.webkit.org/changeset/76911>
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 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
Created bug 55886 that better represents the dependency to land this properly.