RESOLVED FIXED Bug 62092
setting innerText to an empty string on editable div loses focus 
https://bugs.webkit.org/show_bug.cgi?id=62092
Summary setting innerText to an empty string on editable div loses focus
Una Sabovic
Reported 2011-06-04 10:38:12 PDT
Created attachment 96025 [details] simple page to demo the problem When editable divs innerText is set to an empty string caret is lost. Calling element.focus() on that div doesn't restore the caret since that element is already the focused node. Focus is on the element but the caret can not be restored and typing doesn't do anything. This happens because after setting text to an empty string and removing children nodes selection is set to noselection. Same problem happens when clearing innerHTML, but I'm not sure if that should be fixed too.
Attachments
simple page to demo the problem (1.03 KB, text/html)
2011-06-04 10:38 PDT, Una Sabovic 		no flags
Details
proposed patch (4.16 KB, patch)
2011-06-04 10:47 PDT, Una Sabovic 		rniwa: review-
DetailsFormatted DiffDiff
proposed patch (526.88 KB, patch)
2011-06-07 14:41 PDT, Una Sabovic 		rniwa: review-
DetailsFormatted DiffDiff
proposed patch (531.11 KB, patch)
2011-06-08 15:51 PDT, Una Sabovic 		no flags
DetailsFormatted DiffDiff
proposed patch (531.06 KB, patch)
2011-06-08 18:05 PDT, Una Sabovic 		no flags
DetailsFormatted DiffDiff
proposed patch (531.17 KB, patch)
2011-06-08 20:20 PDT, Una Sabovic 		rniwa: review-
DetailsFormatted DiffDiff
html test file that demonstrates the problem - browser compatible (988 bytes, text/html)
2011-06-10 14:22 PDT, Una Sabovic 		no flags
Details
shouldRemovePositionAfterAdoptingTextReplacement change (1.64 KB, patch)
2011-06-23 15:36 PDT, Una Sabovic 		no flags
DetailsFormatted DiffDiff
proposed patch (535.43 KB, patch)
2011-06-23 21:29 PDT, Una Sabovic 		no flags
DetailsFormatted DiffDiff
proposed patch (535.33 KB, patch)
2011-06-23 22:49 PDT, Una Sabovic 		rniwa: review-
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-01 (1.34 MB, application/zip)
2011-06-23 23:13 PDT, WebKit Review Bot 		no flags
Details
update selection per range mutation spec (10.25 KB, patch)
2011-08-29 15:26 PDT, Una Sabovic 		rniwa: review-
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
layout test with examples from http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation (4.74 KB, text/html)
2011-08-29 15:28 PDT, Una Sabovic 		no flags
Details
expected result (1.00 KB, text/plain)
2011-08-29 15:28 PDT, Una Sabovic 		no flags
Details
proposed patch (1.04 MB, patch)
2011-09-25 16:20 PDT, Una Sabovic 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
proposed patch (988.68 KB, patch)
2011-09-26 11:25 PDT, Una Sabovic 		no flags
DetailsFormatted DiffDiff
proposed patch (988.67 KB, patch)
2011-09-26 11:32 PDT, Una Sabovic 		rniwa: review+
DetailsFormatted DiffDiff
proposed patch (1.04 MB, patch)
2011-09-26 12:45 PDT, Una Sabovic 		no flags
DetailsFormatted DiffDiff
Added Mac rebaselines (1.87 MB, patch)
2011-09-26 15:24 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
platform/mac/editing/selection/paste-and-match-style-selector-event.html (825 bytes, text/html)
2011-09-27 11:34 PDT, Una Sabovic 		no flags
Details
proposed patch (1.04 MB, patch)
2011-09-28 12:08 PDT, Una Sabovic 		rniwa: review+
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-03 (18.85 MB, application/zip)
2011-09-28 12:41 PDT, WebKit Review Bot 		no flags
Details
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.
Una Sabovic
Comment 1 2011-06-04 10:47:51 PDT
Created attachment 96026 [details] proposed patch
Ryosuke Niwa
Comment 2 2011-06-04 17:14:47 PDT
Comment on attachment 96026 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=96026&action=review I'd say r- because there are quite few nits. > Source/WebCore/ChangeLog:7 > + After setting text to empty string and removing children nodes selection will be set to noselection. > + If this is the currently focused element update the focus appearance which will restore the carret. > + https://bugs.webkit.org/show_bug.cgi?id=62092 Missing bug title. a change log entry should be (note there's a blank line between bug url & description): bug title bug url description Also, typo: s/carret/caret/. > Source/WebCore/html/HTMLElement.cpp:456 > + if (document()->focusedNode() == this) > + updateFocusAppearance(true); I'm not sure if this is the right fix. Removing child nodes should have invoked FrameSelection::nodeWillBeRemoved. Why do we need to call updateFocusAppearance here manually? > LayoutTests/ChangeLog:7 > + You should explain what kind of a test you're adding (blank lines before and after the description). > LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:1 > +<html> Missing <!DOCTYPE html>. > LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:22 > + if (event.keyCode === 82) { //r Is it significant that we check for this key code? I'd rather always run the test when we have a keydown event. > LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:31 > +<body onload="test()"> I don't see any reason we have to wait load event. You should just put script in body so that you don't have to wait for load event. > LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:33 > +<p>The test runs only under DumpRenderTree with eventSender; if you test by hand the test result below will say FAIL.</p> It seems like this test can be ran manually by pressing some key down in the editable region below, and observing that the character is inserted. > LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:34 > +<div id="it" style="-webkit-user-modify: read-write;-webkit-user-select: text;cursor: text;" onKeyDown="resetIt(event)">text</div> s/onKeyDown/onkeydown/. Also, are these properties all necessary for reproduction (especially -webkit-user-select: text and cursor: text;)?
Ryosuke Niwa
Comment 3 2011-06-04 17:16:05 PDT
+leviw because he has looked at focus / selection behavior recently.
Una Sabovic
Comment 4 2011-06-07 14:41:09 PDT
Created attachment 96299 [details] proposed patch I wish my first attempted change was simpler but I guess Murphy's law is true after all. This change fails a bunch of tests under editing/ because it changes the way selection is handled when its endpoint nodes are deleted. Before selection was set to null, and now its endpoints are updated to equivalent parent-anchored positions. All the tests I looked at in detail are still valid and have the expected layout/endresult but the "intermediate" output has changed. Where selection was (null) now it's a valid selection. Such as: +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of DIV > BODY > HTML > #document to 0 of DIV > BODY > HTML > #document toDOMRange:range from 9 of #text > FONT > DIV > DIV > BODY > HTML > #document to 9 of #text > FONT > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification -EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 9 of #text > FONT > DIV > DIV > BODY > HTML > #document to 9 of #text > FONT > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE Some tests need to be updated to not consider selection != null a failure after deleting a node. This patch is not final, but I was hoping to get some feedback about the code before fixing all the tests. Also, I ran tests for qt only but I'm guessing that other platforms editing tests also need fixing. Thanks!
WebKit Review Bot
Comment 5 2011-06-07 14:43:37 PDT
Attachment 96299 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/dom/Position.h:105: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 229 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ryosuke Niwa
Comment 6 2011-06-07 15:53:53 PDT
Comment on attachment 96299 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=96299&action=review I'm certain that new test belongs to LayoutTests/editing/selection instead of fast/dom/HTMLElement. > Source/WebCore/dom/Position.cpp:218 > +void Position::nodeWillBeRemoved(Node* node) I'm not sure if the name nodeWillBeRemoved makes sense here. >> Source/WebCore/dom/Position.h:105 >> + void nodeWillBeRemoved(Node* node); > > The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] Please remove the argument name 'node'. > Source/WebCore/editing/FrameSelection.cpp:284 > + // When endpoints are removed move the selection to valid anchor nodes. This comment repeats what code says. I don't think we should be adding this comment. In fact, the function name should convey this if any. > Source/WebCore/editing/FrameSelection.cpp:297 > + if (start.isNotNull() && end.isNotNull()) { > + if (m_selection.isBaseFirst()) > + m_selection.setWithoutValidation(start, end); > + else > + m_selection.setWithoutValidation(end, start); > + } else { I'm pretty sure you'd have to clear render tree selection in this case. See comment below that starts with "If we did nothing here" > LayoutTests/editing/execCommand/4920488-expected.txt:13 > -| "" > +| "<#selection-anchor>" > | <a> > | href="http://www.google.com/" > +| <#selection-focus> This seems wrong. Selection-anchor seems to be at a non-canonicalized position. Then again, we're setting position without validation. I'm not sure how we can fix this. > LayoutTests/editing/selection/character-data-mutation-expected.txt:6 > -PASS: Removing the parent of startContainer cleared selection > -PASS: Replacing nodeValue of startContainer cleared selection > -PASS: Replacing nodeValue of endContainer cleared selection > +FAIL: Removing the parent of startContainer did not clear selection > +FAIL: Replacing nodeValue of startContainer did not clear selection > +FAIL: Replacing nodeValue of endContainer did not clear selection These tests are failing. Why is it okay for these tests to fail? If anything we should be modifying the test so that they pass. But I'd really like to know why these tests are failing and why new behavior is correct. r- because of this. > LayoutTests/fast/dom/HTMLElement/editable-div-clear-on-keydown.html:13 > + if (window.layoutTestController) > + layoutTestController.dumpAsText(); > + There's no point in indenting like this. Please outdent.
Una Sabovic
Comment 7 2011-06-08 15:51:28 PDT
Created attachment 96498 [details] proposed patch LayoutTests/editing/selection/character-data-mutation.html was failing because it expects selection to be set to NULL after selection start/end nodes are removed. This is no longer true. For example the first call to runTestPairs removes the SPAN 0x823e108 and expects resulting selection to be null. Tree before modification: BODY 0x81c00c8 #text 0x81c0118 "\n" #comment 0x81c32d0 #text 0x81c3328 "\n" DIV 0x81c3498 * SPAN 0x823e108 #text 0x81bfe40 "hello" #text 0x823f920 " world" #text 0x81c0150 "\n\n" Tree after modification and new selection: BODY 0x8200880 #text 0x81e0b70 "\n" #comment 0x8200ef8 #text 0x8200f50 "\n" * DIV 0x8204720 #text 0x8209248 " world" #text 0x82053e8 "\n\n" SCRIPT 0x8205420 selection start: position 0 of child 3 {DIV} of body selection end: position 1 of child 3 {DIV} of body I updated the test with new expected results. Can you please double check this test editing/selection/5497643.html? I've changed it to output the render tree since it was explicitly checking for newSelection == null which is no longer true.
Una Sabovic
Comment 8 2011-06-08 18:05:45 PDT
Created attachment 96521 [details] proposed patch Pls see previous comment.
Una Sabovic
Comment 9 2011-06-08 20:20:42 PDT
Created attachment 96534 [details] proposed patch
Una Sabovic
Comment 10 2011-06-08 20:29:53 PDT
Comment on attachment 96534 [details] proposed patch I'm not sure why previous patch failed to apply. I recreated it and it worked now. Pls read my previous comment about layout test updates.
Una Sabovic
Comment 11 2011-06-10 10:43:29 PDT
Hi Ryosuke, I think this last patch is it. See my previous explanation and fix for the failed tests. Can you take a look? Thanks.
Ryosuke Niwa
Comment 12 2011-06-10 13:13:15 PDT
Comment on attachment 96534 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=96534&action=review > Source/WebCore/ChangeLog:9 > + When selection start or end node is being deleted do not clear the selection. > + Instead update the start/end position to an equivalent parent-anchored positions. On my second thought, we should verify that this is what other browsers do.
Una Sabovic
Comment 13 2011-06-10 14:22:05 PDT
Created attachment 96786 [details] html test file that demonstrates the problem - browser compatible Attached is the html demo that demonstrates the problem a little further and is browser compatible. There are two test cases. Both test cases work correctly in Firefox. Problem is present in Chrome & Safari. With the attached patch webkit qt launcher behaves the same as Firefox. The code I changed in FrameSelection.cpp had a FIXME which I believe this patch fixes. FIXME said // FIXME: When endpoints are removed, we should just alter the selection, instead of blowing it away.
WebKit Review Bot
Comment 14 2011-06-10 14:23:39 PDT
Attachment 96786 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ryosuke Niwa
Comment 15 2011-06-10 14:39:21 PDT
(In reply to comment #13) > Attached is the html demo that demonstrates the problem a little further and is browser compatible. > There are two test cases. Both test cases work correctly in Firefox. Problem is present in Chrome & Safari. > With the attached patch webkit qt launcher behaves the same as Firefox. Thanks for the investigation. It seems like new behavior matches that of Firefox.
Ryosuke Niwa
Comment 16 2011-06-10 14:57:48 PDT
Comment on attachment 96534 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=96534&action=review > Source/WebCore/dom/Position.cpp:227 > + *this = positionInParentBeforeNode(node); Assigning to *this is very unusual in WebKit but I can't think of a better alternative. > Source/WebCore/dom/Position.h:104 > + // Updates the anchor node if it is about to be deleted. This comment repeats what the function name says. Please remove. > LayoutTests/editing/selection/5497643-expected.txt:12 > -ALERT: SUCCESS: The selection was cleared. > -This tests to make sure that a selection inside a textarea is cleared when the textarea is removed from the document. Not clearing it led to crashes. > - > - > +layer at (0,0) size 800x600 > + RenderView at (0,0) size 800x600 > +layer at (0,0) size 800x600 > + RenderBlock {HTML} at (0,0) size 800x600 > + RenderBody {BODY} at (8,8) size 784x576 > + RenderBlock {P} at (0,0) size 784x22 > + RenderText {#text} at (0,0) size 735x22 > + text run at (0,0) width 735: "This tests to make sure that a selection is correctly updated when the textarea is removed from the document." > + RenderBlock (anonymous) at (0,38) size 784x0 > + RenderText {#text} at (0,0) size 0x0 > + RenderText {#text} at (0,0) size 0x0 > +caret: position 0 of child 0 {DIV} of {#shadow-root} of document This is not right. render tree dump will be platform-specific. We want to avoid it as much as we can in editing because it doesn't tell us useful information. > LayoutTests/editing/selection/5497643.html:-5 > -if (window.layoutTestController) > - window.layoutTestController.dumpAsText(); Why can't this test be dump as text? > LayoutTests/editing/selection/5497643.html:-12 > -if (window.getSelection().type != "None") > - alert("FAILURE: There shouldn't be a selection.") > -else > - alert("SUCCESS: The selection was cleared.") We should be checking that selection is correctly updated as the test claims. > LayoutTests/editing/selection/character-data-mutation.html:73 > + return 'Removing text in ' + nodeName + ' containing the end point'; }, 0, 1); Why is start offset 0? I'd imagine it should be 1 because we're moving "ello" from "hello" and the selection start is between "he" and "llo". I think you need to modify shouldRemovePositionAfterAdoptingTextReplacement now that removing node will never clear selection. r- because of this.
Una Sabovic
Comment 17 2011-06-21 15:44:51 PDT
Sorry for the delay, I've been busy with work. I'm working on a new patch.
Una Sabovic
Comment 18 2011-06-23 15:36:14 PDT
Created attachment 98421 [details] shouldRemovePositionAfterAdoptingTextReplacement change Ryosuke, I have a question about the expected behavior. By example 1) "hello world" selection is "llo wo". We delete text 1,4 to become "h world". New selection should be " wo"? 2) "hello world" selection is "llo wo". We replace text 1,4 to become "hoyo world". New selection should be "yo wo"? That is selection start shouldn't move? Attached is a diff for shouldRemovePositionAfterAdoptingTextReplacement. I based it on these assumptions: if position is in bounds of the inserted text don't change it. If it's not move it to the beginning or the end of the new text. I couldn't think of a scenario in which we would still return "true" from this function. I ran the tests with this change and more tests would need the expected output update but none failed.
WebKit Review Bot
Comment 19 2011-06-23 15:40:04 PDT
Attachment 98421 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ryosuke Niwa
Comment 20 2011-06-23 15:44:40 PDT
(In reply to comment #18) > 1) > "hello world" selection is "llo wo". We delete text 1,4 to become "h world". > New selection should be " wo"? Yes. > 2) > "hello world" selection is "llo wo". We replace text 1,4 to become "hoyo world". > New selection should be "yo wo"? That is selection start shouldn't move? In this case, are we replacing "ello" by "oyo"? Since selection starts in the replaced text, I'd expect the selection to move to after "o" to result in " wo". > I based it on these assumptions: > if position is in bounds of the inserted text don't change it. > If it's not move it to the beginning or the end of the new text. We need to have a different behavior for start/end points. For start point, when inserting text, should move the position forward when inserting at the boundary. Because user didn't select the text that has been inserted. For end point, you shouldn't move the position forward when a text is inserted at the boundary because doing so will results in selecting the inserted text.
Ryosuke Niwa
Comment 21 2011-06-23 15:48:12 PDT
The general principle I took was to always shrink selection when the replaced text contains or is at either end point.
Una Sabovic
Comment 22 2011-06-23 21:29:42 PDT
Created attachment 98455 [details] proposed patch
Una Sabovic
Comment 23 2011-06-23 22:10:09 PDT
Comment on attachment 98455 [details] proposed patch Failed to apply
Una Sabovic
Comment 24 2011-06-23 22:49:50 PDT
Created attachment 98459 [details] proposed patch
WebKit Review Bot
Comment 25 2011-06-23 23:13:41 PDT
Comment on attachment 98459 [details] proposed patch Attachment 98459 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8933354 New failing tests: editing/deleting/delete-at-paragraph-boundaries-006.html editing/deleting/delete-at-paragraph-boundaries-004.html editing/deleting/delete-3865854-fix.html editing/deleting/delete-at-paragraph-boundaries-001.html editing/deleting/delete-3775172-fix.html editing/execCommand/create-list-with-hr.html editing/execCommand/4641880-2.html editing/deleting/delete-3928305-fix.html editing/deleting/delete-3608462-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-at-paragraph-boundaries-003.html editing/deleting/delete-3959464-fix.html editing/deleting/delete-at-paragraph-boundaries-005.html editing/deleting/delete-at-paragraph-boundaries-002.html editing/deleting/delete-after-span-ws-002.html editing/deleting/delete-3800834-fix.html editing/execCommand/4641880-1.html editing/deleting/4845371.html editing/deleting/delete-after-span-ws-003.html editing/deleting/delete-3857753-fix.html
WebKit Review Bot
Comment 26 2011-06-23 23:13:47 PDT
Created attachment 98461 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Una Sabovic
Comment 27 2011-06-23 23:27:41 PDT
Comment on attachment 98459 [details] proposed patch The tests that have failed are platform specific. I don't see these failures on my qt build. I reran the cr failed tests on qt and they do pass. How can I see the diff for the cr tests that have failed? What is the general procedure for this kind of problem? Am I supposed to fix the failing tests for all platforms?
Ryosuke Niwa
Comment 28 2011-06-23 23:46:33 PDT
(In reply to comment #27) > (From update of attachment 98459 [details]) > The tests that have failed are platform specific. I don't see these failures on my qt build. I reran the cr failed tests on qt and they do pass. > > How can I see the diff for the cr tests that have failed? Download the zip file and open results.html inside.
Una Sabovic
Comment 29 2011-06-24 00:26:06 PDT
These tests failed for qt as well and I updated their expected output but for qt platform only. Test diffs are in the patch. I see that tests didn't even run for qt platform once cr-linux has failed. With the change to move the selection when its start/end are in the text being deleted I believe these failures are expected. Before shouldRemovePositionAfterAdoptingTextReplacement could cause selection to be set to null, but now endpoints are moved to the beginning/end of the newly inserted text per Ryosukes comment. The end result of the tests - final render tree output and final selection didn't change. But they are all failing with diffs such as the following in the intermediate results. (null) has turned into a valid selection. EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of LI > UL > DIV > BODY > HTML > #document to 0 of LI > UL > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of DIV > BODY > HTML > #document to 1 of DIV > BODY > HTML > #document toDOMRange:range from 0 of LI > UL > DIV > BODY > HTML > #document to 0 of LI > UL > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
Una Sabovic
Comment 30 2011-06-24 10:46:09 PDT
Only cr-linux tests has failed. This is because I didn't update the expected output for chromium platform specific tests. What is the next step? Do I need to update the expected output for chrome specific tests?
Ryosuke Niwa
Comment 31 2011-06-24 10:48:46 PDT
(In reply to comment #30) > Only cr-linux tests has failed. This is because I didn't update the expected output for chromium platform specific tests. > > What is the next step? Do I need to update the expected output for chrome specific tests? You'll typically update the expected results after you landed the patch since you can grab new results off of bots. You can make use webkit-patch rebaseline to east your pain.
Una Sabovic
Comment 32 2011-06-24 10:54:29 PDT
Ok, sounds good. Can you please review the last patch? It does have updated test results for the qt platform.
Adam Barth
Comment 33 2011-06-24 10:55:28 PDT
> You'll typically update the expected results after you landed the patch since you can grab new results off of bots. You can make use webkit-patch rebaseline to east your pain. Alternatively, you can pull the actual results out of the zip that the bot attached to the bug, which might help you land your patch without making the tree red.
Ryosuke Niwa
Comment 34 2011-06-27 18:23:52 PDT
Comment on attachment 98459 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=98459&action=review > Source/WebCore/dom/Position.cpp:228 > +void Position::updateForNodeRemoval(Node* node) Following naming conventions in FrameSelection and others, willRemoveNode seems like a better name. The function name should reflect the fact it should be called before the node is actually removed. > Source/WebCore/dom/Position.cpp:246 > + case Position::PositionIsAfterAnchor: > + if (node->contains(anchorNode()) || node->contains(anchorNode()->shadowAncestorNode())) > + *this = positionInParentAfterNode(node); > + break; > + case Position::PositionIsBeforeAnchor: > + if (node->contains(anchorNode()) || node->contains(anchorNode()->shadowAncestorNode())) > + *this = positionInParentBeforeNode(node); > + break; I've recently added PositionIsBeforeChildren and PositionIsAfterChildren. I've modified updatePositionForNodeRemoval in DeleteSelectionCommand accordingly so please update it. > Source/WebCore/editing/FrameSelection.cpp:285 > + if (m_selection.isBaseFirst()) > + m_selection.setWithoutValidation(start, end); > + else > + m_selection.setWithoutValidation(end, start); Can we share code with the code below? > Source/WebCore/editing/FrameSelection.cpp:333 > + if (positionOffset > offset && positionOffset < offset + oldLength) { > + // Shrink selection so that it doesn't include inserted text > + if (type == EndPointIsStart) > + position.moveToOffset(offset + newLength); > + else > + position.moveToOffset(offset); > + } So I wasn't aware of this when I initially wrote this code and commented on this bug but DOM Range spec has a section that specifies how Range should be updated upon DOM mutation: http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation Maybe we want to match our selection behavior to that.
Ryosuke Niwa
Comment 35 2011-08-08 12:38:05 PDT
Comment on attachment 98459 [details] proposed patch r- per my comments.
Una Sabovic
Comment 36 2011-08-29 15:26:18 PDT
Created attachment 105531 [details] update selection per range mutation spec
Una Sabovic
Comment 37 2011-08-29 15:28:09 PDT
Created attachment 105532 [details] layout test with examples from http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation
Una Sabovic
Comment 38 2011-08-29 15:28:45 PDT
Created attachment 105533 [details] expected result
Ryosuke Niwa
Comment 39 2011-08-29 15:29:51 PDT
Could you elaborate on what these files are?
WebKit Review Bot
Comment 40 2011-08-29 15:31:34 PDT
Attachment 105532 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Una Sabovic
Comment 41 2011-08-29 15:39:28 PDT
I uploaded just the code patch and new test case for review. This patch is aiming to update the selection on document mutation per spec that Ryosuke mentioned: http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation This change impacts a bunch of existing layout tests and I wanted to run the expected behavior by a reviewer before fixing up all the tests. One thing in the spec that is little strange is >> Note that when content is inserted at a boundary-point, it is ambiguous as to where the boundary-point should be repositioned if its relative position is to be maintained. There are two possibilities: at the start or at the end of the newly inserted content. We have chosen that in this case neither the container nor offset of the boundary-point is changed. As a result, the boundary-point will be positioned at the start of the newly inserted content. >> I understood this as: If we insert at the beginning of the selection, selection will expand to include the inserted content, but if we insert at the end boundary point, since offset is not supposed to change, new selection will NOT include the inserted content. The difference seems a little strange... >> Combining the two code pieces I'm not sure how to combine the two code pieces since one updates the position itself while other sets the new selection.
Una Sabovic
Comment 42 2011-08-29 15:41:02 PDT
Ryosuke, you're too fast! I was just typing it out. One more thing: I didn't know how to code up deletion example 3 using selection/range api. Help?
Ryosuke Niwa
Comment 43 2011-08-29 15:46:44 PDT
(In reply to comment #41) > >> > Note that when content is inserted at a boundary-point, it is ambiguous as to where the boundary-point should be repositioned if its relative position is to be maintained. There are two possibilities: at the start or at the end of the newly inserted content. We have chosen that in this case neither the container nor offset of the boundary-point is changed. As a result, the boundary-point will be positioned at the start of the newly inserted content. > >> > > I understood this as: If we insert at the beginning of the selection, selection will expand to include the inserted content, but if we insert at the end boundary point, since offset is not supposed to change, new selection will NOT include the inserted content. The difference seems a little strange... I believe your understanding is correct. You may be interested in looking at http://html5.org/specs/dom-range.html#range-behavior-under-document-mutation as well.
Ryosuke Niwa
Comment 44 2011-08-29 15:53:43 PDT
(In reply to comment #37) > Created an attachment (id=105532) [details] > layout test with examples from http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation It appears that deletion case doesn't work and expected results are wrong.
WebKit Review Bot
Comment 45 2011-08-29 15:59:39 PDT
Comment on attachment 105531 [details] update selection per range mutation spec Attachment 105531 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9570060 New failing tests: editing/deleting/delete-after-span-ws-002.html editing/deleting/delete-3959464-fix.html editing/deleting/delete-3865854-fix.html editing/deleting/delete-all-text-in-text-field-assertion.html editing/deleting/delete-3775172-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-3928305-fix.html editing/deleting/5546763.html editing/deleting/delete-and-undo.html editing/deleting/delete-3608462-fix.html editing/deleting/delete-at-paragraph-boundaries-001.html editing/deleting/delete-4038408-fix.html editing/deleting/delete-at-paragraph-boundaries-004.html editing/deleting/delete-at-paragraph-boundaries-005.html editing/deleting/delete-at-paragraph-boundaries-002.html editing/deleting/delete-3608445-fix.html editing/deleting/delete-after-span-ws-003.html editing/deleting/delete-at-paragraph-boundaries-003.html editing/deleting/delete-3857753-fix.html
Una Sabovic
Comment 46 2011-08-29 16:40:28 PDT
>> It appears that deletion case doesn't work and expected results are wrong Test works only with the patch applied. It fails on webkit trunk. I can make it not crash on webkit trunk but it will still fail. Can you apply the patch and try? I generated the expected results file (also attached) with my changes applied.
Aryeh Gregor
Comment 47 2011-08-30 09:37:06 PDT
Standards-wise, http://html5.org/specs/dom-range.html#range-behavior-under-document-mutation is the right place to look. The behavior largely matches DOM 2 Range, but is more precise and differs in some cases. There's a fairly extensive test suite here: http://aryeh.name/spec/dom-range/test/Range-mutations.html Currently all the Selection-related tests fail in WebKit because getRangeAt() returns a copy instead of the original range assigned to it. The behavior should be the same for selections and ranges: whatever happens to a range when you delete the nodes in it should also happen to a selection. Generally this means any boundary point in a removed node will be moved to the removed node's former parent. (In reply to comment #41) > One thing in the spec that is little strange is > > >> > Note that when content is inserted at a boundary-point, it is ambiguous as to where the boundary-point should be repositioned if its relative position is to be maintained. There are two possibilities: at the start or at the end of the newly inserted content. We have chosen that in this case neither the container nor offset of the boundary-point is changed. As a result, the boundary-point will be positioned at the start of the newly inserted content. > >> > > I understood this as: If we insert at the beginning of the selection, selection will expand to include the inserted content, but if we insert at the end boundary point, since offset is not supposed to change, new selection will NOT include the inserted content. The difference seems a little strange... Yes, that's correct. It is a little strange, but it's hard to think of anything better. Anyway, it's how all browsers behave for Ranges, so we can't change it. The newer spec puts it like this: """ For each boundary point whose node is the new parent of the affected node and whose offset is greater than the new index of the affected node, add one to the boundary point's offset. """ http://html5.org/specs/dom-range.html#range-behavior-under-document-mutation If you insert at the beginning of the selection, the offset of the selection start is not greater than the new index of the affected node, it's equal, so the start remains in place -- before the new node, so it's included. If you insert at the end, the offset of the end is also not greater than the new index of the affected node, so the end remains in place -- still before the new node, so it's not included.
Ryosuke Niwa
Comment 48 2011-08-30 09:41:16 PDT
(In reply to comment #47) > Currently all the Selection-related tests fail in WebKit because getRangeAt() returns a copy instead of the original range assigned to it. I don't think we can ever fix this failure because we don't implement selection in terms of ranges. And it's nearly impossible to change that because all embedders depend on this implementation.
Darin Adler
Comment 49 2011-08-30 09:50:03 PDT
(In reply to comment #48) > (In reply to comment #47) > > Currently all the Selection-related tests fail in WebKit because getRangeAt() returns a copy instead of the original range assigned to it. > > I don't think we can ever fix this failure because we don't implement selection in terms of ranges. And it's nearly impossible to change that because all embedders depend on this implementation. I think it’s difficult but not impossible. We could make a special Range flag that means “this range represents the selection” and then add code to make changes to that range turn into selection-changing operations. But I think this technique for changing selections, getting ranges and then manipulating the ranges directly, is skewed towards Mozilla implementation details and probably not good for a cross-browser standard. Does the technique work in IE?
Aryeh Gregor
Comment 50 2011-08-30 11:30:29 PDT
IE9 and later implements Range and Selection more or less per spec, and IE9 does pass some of the selection tests in that file. (It actually passes fewer of the tests overall than WebKit, but because of other bugs.) Opera has issues similar to WebKit. The spec is biased toward Gecko, yes, because 1) Gecko's behavior here is far simpler and easier to understand than WebKit's, and will be easier to converge on; 2) more browsers implement the features that way (IE+Gecko vs. just WebKit or just Opera); 3) Gecko's behavior is older (they made these APIs up!) and has been in standards longer. Maybe this bug isn't the right place to discuss this, though. The whatwg list is a good place to send feedback about DOM Range, or possibly public-webapps.
Ryosuke Niwa
Comment 51 2011-09-14 20:43:02 PDT
Comment on attachment 105531 [details] update selection per range mutation spec Sorry for the delay! r=me. Could you tell me when you can be available on IRC so that we can coordinate to land this patch? It's very likely that we have to rebaseline hundreds of tests so I need your help.
Ryosuke Niwa
Comment 52 2011-09-14 20:44:21 PDT
Comment on attachment 105531 [details] update selection per range mutation spec Oops, wait a minute, this patch is lacking change log entry and a test. Please add those.
Una Sabovic
Comment 53 2011-09-15 09:58:25 PDT
I'll be on IRC tomorrow (Friday) after 2pm. Also I am almost always on gchat: unabuvica@gmail.com if that's more convenient. Will add changelog entries and tests. I'll also try to fix up all failing tests for qt platform but may need help with chromium specific tests.
Una Sabovic
Comment 54 2011-09-25 16:20:40 PDT
Created attachment 108615 [details] proposed patch Proposed patch. Fixed up tests for qt port are included.
WebKit Review Bot
Comment 55 2011-09-25 18:30:06 PDT
Comment on attachment 108615 [details] proposed patch Attachment 108615 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9843714 New failing tests: editing/deleting/delete-at-paragraph-boundaries-006.html editing/deleting/delete-at-paragraph-boundaries-004.html editing/deleting/delete-3865854-fix.html editing/deleting/delete-all-text-in-text-field-assertion.html editing/deleting/delete-3928305-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-3775172-fix.html editing/deleting/delete-and-undo.html editing/deleting/delete-3608462-fix.html editing/deleting/delete-at-paragraph-boundaries-001.html editing/deleting/delete-at-paragraph-boundaries-007.html editing/deleting/delete-3959464-fix.html editing/deleting/delete-at-paragraph-boundaries-005.html editing/deleting/delete-at-paragraph-boundaries-002.html editing/deleting/delete-after-span-ws-002.html editing/deleting/delete-after-span-ws-003.html editing/deleting/delete-at-paragraph-boundaries-003.html editing/deleting/delete-3608445-fix.html editing/deleting/delete-3857753-fix.html
Darin Adler
Comment 56 2011-09-25 19:01:08 PDT
Comment on attachment 108615 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=108615&action=review > Source/WebCore/dom/Position.h:123 > + void willRemoveNode(Node*); This is going to require a bit of thought. It's not good to have this function in a dom class, but not have code to ensure it is called any time a node is removed from its parent. The corresponding function in WebCore::Range is always called by the DOM itself, while this function is only called by editing machinery. There's no easy way to tell that when looking at this class, and in fact it would be reasonable to expect the other approach. The question is whether we are redefining Position objects so they automatically update in this fashion, or if this is a function to be called voluntarily only by editing code, and not part of the DOM itself. Either would be OK, but this current version is half and half. If this function is meant to be called only by editing machinery, it should probably not be in a class in the dom directory, or at the very least should have editing in its name. Although a function for editing that is baked into a basic class is an anti-pattern. The many editing-related functions in Node are a design mistake.
Ryosuke Niwa
Comment 57 2011-09-25 20:14:10 PDT
Comment on attachment 108615 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=108615&action=review Please clean up the test code. The rest of the patch looks good to me. >> Source/WebCore/dom/Position.h:123 >> + void willRemoveNode(Node*); > > This is going to require a bit of thought. > > It's not good to have this function in a dom class, but not have code to ensure it is called any time a node is removed from its parent. The corresponding function in WebCore::Range is always called by the DOM itself, while this function is only called by editing machinery. There's no easy way to tell that when looking at this class, and in fact it would be reasonable to expect the other approach. > > The question is whether we are redefining Position objects so they automatically update in this fashion, or if this is a function to be called voluntarily only by editing code, and not part of the DOM itself. Either would be OK, but this current version is half and half. > > If this function is meant to be called only by editing machinery, it should probably not be in a class in the dom directory, or at the very least should have editing in its name. Although a function for editing that is baked into a basic class is an anti-pattern. The many editing-related functions in Node are a design mistake. This is the reason I was talking about PersistenPosition a couple of months ago. FrameSelection, ReplaceSelectionCommand, and a couple other classes really want to make positions persist over DOM mutatuions but we can't do it at the moment because ending/starting positions used for undo/redo shouldn't automatically be updated. I think it's okay to add this function for now. > LayoutTests/editing/selection/character-data-mutation.html:63 > + runTestPairs(function() { test.removeChild(test.firstChild); return 'Removing the parent of startContainer'; }, 0, undefined, true); You've inserted a space at the end of line here. Please remove. > LayoutTests/editing/selection/document-mutation.html:22 > + var selection = window.getSelection(); It seems like we can make this a global variable. > LayoutTests/editing/selection/document-mutation.html:26 > + selection.setBaseAndExtent(test.firstChild.firstChild, 19, test.firstChild.firstChild, 11); Can we extract as a function like setRange(startContainer, startOffset, endContainer, endOffset) that automatically checks the value of baseIsFirst and call setBaseAndExtent with appropriate arguments? > LayoutTests/editing/selection/document-mutation.html:37 > + '" and expected selection is ' + '(' + expectedStartOffset + ', ' + expectedEndOffset + ')\n'); It seems like this can be extracted as a function. > LayoutTests/editing/selection/editable-div-clear-on-keydown.html:18 > + eventSender.keyDown("a"); I would do this at the very end so that readers can understand the test logic first. > LayoutTests/platform/qt/editing/inserting/typing-003-expected.txt:747 > EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification With 1116 lines of output (or 747 lines after your patch), I really doubt that there's much benefit in dumping editing delegates in this test. We can't possibly make sense of this much output. > LayoutTests/platform/qt/editing/selection/regional-indicators-expected.txt:5 > +FAIL document.getSelection().toString() should be ð¯ðµ. Was . This diff doesn't look good. Maybe you're missing some int'l package in your system?
Darin Adler
Comment 58 2011-09-26 09:28:59 PDT
(In reply to comment #57) > I think it's okay to add this function for now. Sure, but can we keep it in the editing code instead of coding it directly into the DOM?
Ryosuke Niwa
Comment 59 2011-09-26 09:43:15 PDT
(In reply to comment #58) > (In reply to comment #57) > > I think it's okay to add this function for now. > > Sure, but can we keep it in the editing code instead of coding it directly into the DOM? Like in htmlediting.cpp? That might make sense.
Una Sabovic
Comment 60 2011-09-26 11:25:46 PDT
Created attachment 108693 [details] proposed patch Moved updatePositionForNodeRemoval to htmlediting instead of Position. Removed editing delegates from LayoutTests/editing/inserting/typing-003.html Calling runDumpAsTextEditingTest instead of runEditingTest also produced a lot of output so I took the other approach. > LayoutTests/platform/qt/editing/selection/regional-indicators-expected.txt:5 > +FAIL document.getSelection().toString() should be ð¯ðµ. Was . > This diff doesn't look good. Maybe you're missing some int'l package in your system? I was also suspicious about this one but it is the expected behavior. When >> div.innerText = "🇯🇵🇯🇵"; // line 21 The entire content is deleted making selection (start, end) = (0, 0). Then new content is inserted at the endpoints which doesn't move the start or end. So we end up with an empty selection. This test is strange with "FAIL" in its expected output.
WebKit Review Bot
Comment 61 2011-09-26 11:28:07 PDT
Attachment 108693 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/editing/htmlediting.h:162: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/htmlediting.h:162: The parameter name "position" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 305 files If any of these errors are false positives, please file a bug against check-webkit-style.
Una Sabovic
Comment 62 2011-09-26 11:32:41 PDT
Created attachment 108694 [details] proposed patch Fixed style. See previous comment.
Ryosuke Niwa
Comment 63 2011-09-26 11:58:57 PDT
Comment on attachment 108694 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=108694&action=review Please fix below and I'll land it. > LayoutTests/editing/selection/document-mutation.html:40 > + checkResult(selection.getRangeAt(0).startOffset, selection.getRangeAt(0).endOffset, expectedStartOffset, expectedEndOffset); I feel like you can just call selection.getRangeAt(0).startOffset and selection.getRangeAt(0).endOffset in checkResult but that's fine. > LayoutTests/platform/qt/editing/inserting/typing-003-expected.txt:-3 > -EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 3 of DIV > BODY > HTML > #document > -EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification > -EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification !? You need to modify typing-003.html to disable editing delegates if you were to remove dumps in the expected result. > LayoutTests/platform/qt/editing/selection/regional-indicators-expected.txt:5 > -FAIL document.getSelection().toString() should be ð¯ðµ. Was ðµ. > +FAIL document.getSelection().toString() should be ð¯ðµ. Was . I think we just need to call getSelection().removeAllRanges before the second call to getSelection().addRange(afterLastIndicator);
Una Sabovic
Comment 64 2011-09-26 12:45:20 PDT
Created attachment 108709 [details] proposed patch
WebKit Review Bot
Comment 65 2011-09-26 14:21:49 PDT
Comment on attachment 108709 [details] proposed patch Attachment 108709 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9878055 New failing tests: editing/deleting/delete-at-paragraph-boundaries-006.html editing/deleting/delete-at-paragraph-boundaries-004.html editing/deleting/delete-3865854-fix.html editing/deleting/delete-all-text-in-text-field-assertion.html editing/deleting/delete-3928305-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-3775172-fix.html editing/deleting/delete-and-undo.html editing/deleting/delete-3608462-fix.html editing/deleting/delete-at-paragraph-boundaries-001.html editing/deleting/delete-at-paragraph-boundaries-007.html editing/deleting/delete-3959464-fix.html editing/deleting/delete-at-paragraph-boundaries-005.html editing/deleting/delete-at-paragraph-boundaries-002.html editing/deleting/delete-after-span-ws-002.html editing/deleting/delete-after-span-ws-003.html editing/deleting/delete-at-paragraph-boundaries-003.html editing/deleting/delete-3608445-fix.html editing/deleting/delete-3857753-fix.html
Ryosuke Niwa
Comment 66 2011-09-26 14:31:15 PDT
Comment on attachment 108709 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=108709&action=review Thanks for the update. I'll land this patch manually and try to take care of rebaselines. > Source/WebCore/editing/htmlediting.cpp:926 > +void updatePositionForNodeRemoval(Node* node, Position& position) I'd change this function to take Position first before landing since the position is the primary object this function works on.
Ryosuke Niwa
Comment 67 2011-09-26 14:51:06 PDT
Comment on attachment 108709 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=108709&action=review > Source/WebCore/editing/FrameSelection.cpp:347 > +static void updatePositionAfterAdoptingTextReplacement(Position& position, EndPointType type, CharacterData* node, unsigned offset, unsigned oldLength, unsigned newLength) Apparently type is no longer used in this function. I'm getting rid of this argument as I land.
Una Sabovic
Comment 68 2011-09-26 14:59:07 PDT
> Apparently type is no longer used in this function. Strange that I didn't get a compilation warning. Maybe I didn't see it?
Ryosuke Niwa
Comment 69 2011-09-26 15:16:16 PDT
Comment on attachment 108709 [details] proposed patch Apparently this patch regresses platform/mac/editing/pasteboard/paste-and-match-style-selector-event.html so r-.
Ryosuke Niwa
Comment 70 2011-09-26 15:24:02 PDT
Created attachment 108731 [details] Added Mac rebaselines
Ryosuke Niwa
Comment 71 2011-09-26 15:26:25 PDT
Also, I've started to think that maybe we should dispatch WebViewDidChangeSelectionNotification even when we're setting without validation.
Ryosuke Niwa
Comment 72 2011-09-26 15:57:53 PDT
It's very likely that we must be doing: notifyRendererOfSelectionChange(userTriggered); m_frame->editor()->respondToChangedSelection(oldSelection, options); notifyAccessibilityForSelectionChange(); m_frame->document()->enqueueDocumentEvent(Event::create(eventNames().selectionchangeEvent, false, false)); in textWillBeReplaced and respondToNodeModification.
Una Sabovic
Comment 73 2011-09-27 09:46:23 PDT
I'm trying to debug the failing test but am having a problem linking webkit qt debug build today. ld exits with "not enough memory". Usage was at 3.3G when error was reported. Release build links fine. Do you maybe know how I could fix this? Other than installing 64bit linux :) Thanks
Una Sabovic
Comment 74 2011-09-27 11:34:53 PDT
Created attachment 108875 [details] platform/mac/editing/selection/paste-and-match-style-selector-event.html version that works with new changes. If the purpose of this test is to make sure onpaste event is fired
Una Sabovic
Comment 75 2011-09-27 11:40:33 PDT
Comment on attachment 108875 [details] platform/mac/editing/selection/paste-and-match-style-selector-event.html Would this change be ok? It works on qt platform and runs the same on wk trunk and with the new selection changes.
WebKit Review Bot
Comment 76 2011-09-27 11:44:46 PDT
Attachment 108875 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Una Sabovic
Comment 77 2011-09-27 12:45:50 PDT
Ryosuke can you please attach the actual result for platform/mac/editing/selection/paste-and-match-style-selector-event.html? I might have been looking at the wrong thing since this test doesn't work with qt.
Ryosuke Niwa
Comment 78 2011-09-27 13:07:37 PDT
(In reply to comment #77) > Ryosuke can you please attach the actual result for platform/mac/editing/selection/paste-and-match-style-selector-event.html? > > I might have been looking at the wrong thing since this test doesn't work with qt. With your patch, we get SUCCESSFAIL (could have been FAILSUCCESS) instead of SUCCESS.
Una Sabovic
Comment 79 2011-09-27 13:22:34 PDT
I was looking at the wrong thing. Using document.execCommand("PasteAsPlainText"); gives me the same result with qt so I can actually debug this now since root cause is probably the same.
Una Sabovic
Comment 80 2011-09-28 09:50:34 PDT
What happens is that now when the selection is not blown away when start or end node is removed in FrameSelection::respondToNodeModification the text "FAILURE" is actually pasted. So the flow is: 1. "FAILURE" is cut and selection is updated 2. onPasteHandler is called setting div.innerText to "SUCCESS" and selection is updated 3. Editor::pasteAsPlainText() continues executing but canPaste() now returns true since selection is valid 4. Paste inserts "FAILURE" before "SUCCESS" and we end up with "FAILURESUCCESS" with old behavior canPaste() was returning false since selection was null and text "FAILURE" was not pasted at all. If this sounds ok I will update the test.
Una Sabovic
Comment 81 2011-09-28 12:08:44 PDT
Created attachment 109050 [details] proposed patch Updated LayoutTests/platform/mac/editing/pasteboard/paste-and-match-style-selector-event.html
Ryosuke Niwa
Comment 82 2011-09-28 12:32:56 PDT
Comment on attachment 109050 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=109050&action=review > LayoutTests/platform/qt/editing/selection/node-removal-1-expected.txt:24 > +selection start: position 1 of child 0 {#text} of child 5 {DIV} of body > +selection end: position 1 of child 5 {DIV} of body It seems like we should update the test description for this test but we can do it as a follow up.
WebKit Review Bot
Comment 83 2011-09-28 12:40:43 PDT
Comment on attachment 109050 [details] proposed patch Attachment 109050 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9883652 New failing tests: editing/deleting/delete-at-paragraph-boundaries-006.html editing/deleting/delete-at-paragraph-boundaries-004.html editing/deleting/delete-3865854-fix.html editing/deleting/delete-all-text-in-text-field-assertion.html editing/deleting/delete-3928305-fix.html editing/deleting/collapse-whitespace-3587601-fix.html editing/deleting/delete-3800834-fix.html editing/deleting/delete-3775172-fix.html editing/deleting/delete-and-undo.html editing/deleting/delete-3608462-fix.html editing/deleting/delete-at-paragraph-boundaries-001.html editing/deleting/delete-at-paragraph-boundaries-007.html editing/deleting/delete-3959464-fix.html editing/deleting/delete-at-paragraph-boundaries-005.html editing/deleting/delete-at-paragraph-boundaries-002.html editing/deleting/delete-after-span-ws-002.html editing/deleting/delete-after-span-ws-003.html editing/deleting/delete-at-paragraph-boundaries-003.html editing/deleting/delete-3608445-fix.html editing/deleting/delete-3857753-fix.html
WebKit Review Bot
Comment 84 2011-09-28 12:41:17 PDT
Created attachment 109055 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Ryosuke Niwa
Comment 85 2011-09-28 13:50:16 PDT
Committed r96257: <http://trac.webkit.org/changeset/96257>
Ryosuke Niwa
Comment 86 2011-09-28 16:28:01 PDT
Mac rebaselines done in http://trac.webkit.org/changeset/96264. GTK rebaselines done in http://trac.webkit.org/changeset/96266.
Alexey Proskuryakov
Comment 88 2012-04-16 09:28:36 PDT
This caused a large performance regression, bug 83983.
Ryosuke Niwa
Comment 89 2013-02-04 11:33:52 PST
*** Bug 67464 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.