Bug 62092 - setting innerText to an empty string on editable div loses focus
Summary: setting innerText to an empty string on editable div loses focus
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Normal
Assignee: Nobody
URL:
Keywords:
: 67464 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-06-04 10:38 PDT by Una Sabovic
Modified: 2013-02-04 11:33 PST (History)
12 users (show)

See Also:


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-
Details | Formatted Diff | Diff
proposed patch (526.88 KB, patch)
2011-06-07 14:41 PDT, Una Sabovic
rniwa: review-
Details | Formatted Diff | Diff
proposed patch (531.11 KB, patch)
2011-06-08 15:51 PDT, Una Sabovic
no flags Details | Formatted Diff | Diff
proposed patch (531.06 KB, patch)
2011-06-08 18:05 PDT, Una Sabovic
no flags Details | Formatted Diff | Diff
proposed patch (531.17 KB, patch)
2011-06-08 20:20 PDT, Una Sabovic
rniwa: review-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
proposed patch (535.43 KB, patch)
2011-06-23 21:29 PDT, Una Sabovic
no flags Details | Formatted Diff | Diff
proposed patch (535.33 KB, patch)
2011-06-23 22:49 PDT, Una Sabovic
rniwa: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
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-
Details | Formatted Diff | Diff
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-
Details | Formatted Diff | Diff
proposed patch (988.68 KB, patch)
2011-09-26 11:25 PDT, Una Sabovic
no flags Details | Formatted Diff | Diff
proposed patch (988.67 KB, patch)
2011-09-26 11:32 PDT, Una Sabovic
rniwa: review+
Details | Formatted Diff | Diff
proposed patch (1.04 MB, patch)
2011-09-26 12:45 PDT, Una Sabovic
no flags Details | Formatted Diff | Diff
Added Mac rebaselines (1.87 MB, patch)
2011-09-26 15:24 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
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-
Details | Formatted Diff | Diff
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

Note You need to log in before you can comment on or make changes to this bug.
Description Una Sabovic 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.
Comment 1 Una Sabovic 2011-06-04 10:47:51 PDT
Created attachment 96026 [details]
proposed patch
Comment 2 Ryosuke Niwa 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;)?
Comment 3 Ryosuke Niwa 2011-06-04 17:16:05 PDT
+leviw because he has looked at focus / selection behavior recently.
Comment 4 Una Sabovic 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!
Comment 5 WebKit Review Bot 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.
Comment 6 Ryosuke Niwa 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.
Comment 7 Una Sabovic 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.
Comment 8 Una Sabovic 2011-06-08 18:05:45 PDT
Created attachment 96521 [details]
proposed patch

Pls see previous comment.
Comment 9 Una Sabovic 2011-06-08 20:20:42 PDT
Created attachment 96534 [details]
proposed patch
Comment 10 Una Sabovic 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.
Comment 11 Una Sabovic 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.
Comment 12 Ryosuke Niwa 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.
Comment 13 Una Sabovic 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.
Comment 14 WebKit Review Bot 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.
Comment 15 Ryosuke Niwa 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.
Comment 16 Ryosuke Niwa 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.
Comment 17 Una Sabovic 2011-06-21 15:44:51 PDT
Sorry for the delay, I've been busy with work. I'm working on a new patch.
Comment 18 Una Sabovic 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.
Comment 19 WebKit Review Bot 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.
Comment 20 Ryosuke Niwa 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.
Comment 21 Ryosuke Niwa 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.
Comment 22 Una Sabovic 2011-06-23 21:29:42 PDT
Created attachment 98455 [details]
proposed patch
Comment 23 Una Sabovic 2011-06-23 22:10:09 PDT
Comment on attachment 98455 [details]
proposed patch

Failed to apply
Comment 24 Una Sabovic 2011-06-23 22:49:50 PDT
Created attachment 98459 [details]
proposed patch
Comment 25 WebKit Review Bot 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
Comment 26 WebKit Review Bot 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
Comment 27 Una Sabovic 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?
Comment 28 Ryosuke Niwa 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.
Comment 29 Una Sabovic 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
Comment 30 Una Sabovic 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?
Comment 31 Ryosuke Niwa 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.
Comment 32 Una Sabovic 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.
Comment 33 Adam Barth 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.
Comment 34 Ryosuke Niwa 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.
Comment 35 Ryosuke Niwa 2011-08-08 12:38:05 PDT
Comment on attachment 98459 [details]
proposed patch

r- per my comments.
Comment 36 Una Sabovic 2011-08-29 15:26:18 PDT
Created attachment 105531 [details]
update selection per range mutation spec
Comment 37 Una Sabovic 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
Comment 38 Una Sabovic 2011-08-29 15:28:45 PDT
Created attachment 105533 [details]
expected result
Comment 39 Ryosuke Niwa 2011-08-29 15:29:51 PDT
Could you elaborate on what these files are?
Comment 40 WebKit Review Bot 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.
Comment 41 Una Sabovic 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.
Comment 42 Una Sabovic 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?
Comment 43 Ryosuke Niwa 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.
Comment 44 Ryosuke Niwa 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.
Comment 45 WebKit Review Bot 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
Comment 46 Una Sabovic 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.
Comment 47 Aryeh Gregor 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.
Comment 48 Ryosuke Niwa 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.
Comment 49 Darin Adler 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?
Comment 50 Aryeh Gregor 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.
Comment 51 Ryosuke Niwa 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.
Comment 52 Ryosuke Niwa 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.
Comment 53 Una Sabovic 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.
Comment 54 Una Sabovic 2011-09-25 16:20:40 PDT
Created attachment 108615 [details]
proposed patch

Proposed patch. Fixed up tests for qt port are included.
Comment 55 WebKit Review Bot 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
Comment 56 Darin Adler 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.
Comment 57 Ryosuke Niwa 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?
Comment 58 Darin Adler 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?
Comment 59 Ryosuke Niwa 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.
Comment 60 Una Sabovic 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.
Comment 61 WebKit Review Bot 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.
Comment 62 Una Sabovic 2011-09-26 11:32:41 PDT
Created attachment 108694 [details]
proposed patch

Fixed style. See previous comment.
Comment 63 Ryosuke Niwa 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);
Comment 64 Una Sabovic 2011-09-26 12:45:20 PDT
Created attachment 108709 [details]
proposed patch
Comment 65 WebKit Review Bot 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
Comment 66 Ryosuke Niwa 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.
Comment 67 Ryosuke Niwa 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.
Comment 68 Una Sabovic 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?
Comment 69 Ryosuke Niwa 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-.
Comment 70 Ryosuke Niwa 2011-09-26 15:24:02 PDT
Created attachment 108731 [details]
Added Mac rebaselines
Comment 71 Ryosuke Niwa 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.
Comment 72 Ryosuke Niwa 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.
Comment 73 Una Sabovic 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
Comment 74 Una Sabovic 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
Comment 75 Una Sabovic 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.
Comment 76 WebKit Review Bot 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.
Comment 77 Una Sabovic 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.
Comment 78 Ryosuke Niwa 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.
Comment 79 Una Sabovic 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.
Comment 80 Una Sabovic 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.
Comment 81 Una Sabovic 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
Comment 82 Ryosuke Niwa 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.
Comment 83 WebKit Review Bot 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
Comment 84 WebKit Review Bot 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
Comment 85 Ryosuke Niwa 2011-09-28 13:50:16 PDT
Committed r96257: <http://trac.webkit.org/changeset/96257>
Comment 86 Ryosuke Niwa 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.
Comment 88 Alexey Proskuryakov 2012-04-16 09:28:36 PDT
This caused a large performance regression, bug 83983.
Comment 89 Ryosuke Niwa 2013-02-04 11:33:52 PST
*** Bug 67464 has been marked as a duplicate of this bug. ***