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.
<rdar://problem/9922944>
(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.
Created attachment 145955 [details] Patch
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.
(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.
> 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.
Comment on attachment 145955 [details] Patch r=me as long as tests pass. Please update the change log as advised.
Created attachment 146207 [details] Patch for landing
Comment on attachment 146207 [details] Patch for landing Clearing flags on attachment: 146207 Committed r119705: <http://trac.webkit.org/changeset/119705>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 94192
*** Bug 91325 has been marked as a duplicate of this bug. ***
(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.
(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.
http://trac.webkit.org/changeset/125760