Bug 66120

Summary:

Incorrect data retrieved in calls to get selection data in client->textWillBeReplaced() from CharacterData::setDataAndUpdate

Product:

WebKit

Reporter:

Jon Lee <jonlee>

Component:

HTML Editing

Assignee:

Shinya Kawanaka <shinyak>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

enrica, inferno, mitz, rniwa, shinyak, webkit-bug-importer, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.7

Bug Depends on:

94192

Bug Blocks:

54535

Attachments:

Description	Flags
Patch	none
Patch for landing	none

Jon Lee

Reported 2011-08-11 17:58:46 PDT

The call to textWillBeReplaced() in CharacterData::setDataAndUpdate() is performed before the text node gets updated, and before the document is notified that text has been removed. textWillBeReplaced() updates the selection, which in turn notifies the client. If the client decides to retrieve the selection, or create a Range based on the data or selection, the client would retrieve incorrect data for the following reasons: 1) Since the DOM node has not been updated yet, the desired selection/caret might be outside of the available DOM range. (For example, if the user types a character) 2) Any ranges created by the client into the document would get adjusted by the textRemoved() call. It would seem to make sense to move the textWillBeReplaced() call to after the textRemoved() call, but I would appreciate some input as to why this call is made when it is made.

Attachments
Patch (4.28 KB, patch) 2012-06-06 00:55 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Patch for landing (4.34 KB, patch) 2012-06-06 23:31 PDT, Shinya Kawanaka	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Jon Lee

Comment 1 2011-08-11 17:59:14 PDT

<rdar://problem/9922944>

Ryosuke Niwa

Comment 2 2011-08-11 18:11:57 PDT

(In reply to comment #0) > It would seem to make sense to move the textWillBeReplaced() call to after the textRemoved() call, but I would appreciate some input as to why this call is made when it is made. If we do this, then you'd have to update the code in textWillBeReplaced so that it'll update selection accordingly. The current code relies on the fact, it's called before the node is updated.

Shinya Kawanaka

Comment 3 2012-06-06 00:55:50 PDT

Created attachment 145955 [details] Patch

Ryosuke Niwa

Comment 4 2012-06-06 00:58:34 PDT

Comment on attachment 145955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145955&action=review Did you run all the layout tests with this change? I'd expect hundreds of tests to fail. > Source/WebCore/ChangeLog:12 > + No new tests, refactoring. This is definitely not true. This change WILL change the webkit's behavior. A better justification will be that we can't test this in layout tests.

Shinya Kawanaka

Comment 5 2012-06-06 02:29:59 PDT

(In reply to comment #4) > (From update of attachment 145955 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145955&action=review > > Did you run all the layout tests with this change? I'd expect hundreds of tests to fail. As far as I tried, only around 10 tests failed. I'm not sure it's regression or not though... I have to run tests in ToT again... > > > Source/WebCore/ChangeLog:12 > > + No new tests, refactoring. > > This is definitely not true. This change WILL change the webkit's behavior. > A better justification will be that we can't test this in layout tests. OK.

Shinya Kawanaka

Comment 6 2012-06-06 02:30:59 PDT

> As far as I tried, only around 10 tests failed. I'm not sure it's regression or not though... > I have to run tests in ToT again... Actually I hoped EWS to test them... But chromium bot is down now. Too bad.

Ryosuke Niwa

Comment 7 2012-06-06 12:23:56 PDT

Comment on attachment 145955 [details] Patch r=me as long as tests pass. Please update the change log as advised.

Shinya Kawanaka

Comment 8 2012-06-06 23:31:30 PDT

Created attachment 146207 [details] Patch for landing

WebKit Review Bot

Comment 9 2012-06-07 04:45:36 PDT

Comment on attachment 146207 [details] Patch for landing Clearing flags on attachment: 146207 Committed r119705: <http://trac.webkit.org/changeset/119705>

WebKit Review Bot

Comment 10 2012-06-07 04:45:41 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 11 2012-08-16 00:02:31 PDT

Re-opened since this is blocked by 94192

Abhishek Arya

Comment 12 2012-08-16 00:07:37 PDT

*** Bug 91325 has been marked as a duplicate of this bug. ***

Abhishek Arya

Comment 13 2012-08-16 00:08:43 PDT

(In reply to comment #12) > *** Bug 91325 has been marked as a duplicate of this bug. *** Please do see the testcase in 91325 which reproduces 100% reliably on asanified version of chromium linux and explanation of the problem in c#13.

Ryosuke Niwa

Comment 14 2012-08-16 01:45:10 PDT

(In reply to comment #13) > (In reply to comment #12) > > *** Bug 91325 has been marked as a duplicate of this bug. *** > > Please do see the testcase in 91325 which reproduces 100% reliably on asanified version of chromium linux and explanation of the problem in c#13. This is crazy. Why was the patch rolled out without any prior notice or a formal review.

Abhishek Arya

Comment 15 2012-08-16 07:06:23 PDT

http://trac.webkit.org/changeset/125760

Note You need to log in before you can comment on or make changes to this bug.