Created attachment 196976 [details] Demo allowing reproduction of bug (including "workaround" by selecting with drag or reseting range programmatically) If the user selects a word by double-clicking it, *any* modifications to the underlying text node cause the user's selection to collapse. This does not happen if the user selects the word by dragging across it, nor if the selected range is fetched and applied before the text node modification. I believe this may be related to an old thread here https://bugs.webkit.org/show_bug.cgi?id=35625 as the main difference I can see in the results has to do with the anchor/focus/extent values. When a word is drag-selected: baseOffset: 24 extentOffset: 28 focusOffset: 28 When a word is double-clicked: baseOffset: 25* extentOffset: 25* focusOffset: 28 * actually dependent on where the user double-clicked. It seems the selection update code must be relying on the unreliable base/extent information rather than the range information.
Created attachment 196978 [details] Fixed version of previous attachment (removed temporary test stuff interfering with intended demo)
Created attachment 214493 [details] work in progress
Comment on attachment 214493 [details] work in progress Attachment 214493 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4099095 New failing tests: editing/execCommand/4641880-2.html editing/deleting/delete-ws-fixup-002.html editing/selection/character-data-mutation.html editing/style/style-3690704-fix.html editing/execCommand/4641880-1.html editing/deleting/smart-delete-003.html editing/deleting/smart-delete-004.html editing/style/style-3681552-fix-001.html
Created attachment 214503 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 214493 [details] work in progress Attachment 214493 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4151007 New failing tests: editing/execCommand/4641880-2.html editing/deleting/delete-ws-fixup-002.html editing/selection/character-data-mutation.html editing/style/style-3690704-fix.html editing/execCommand/4641880-1.html editing/deleting/smart-delete-003.html editing/deleting/smart-delete-004.html editing/style/style-3681552-fix-001.html
Created attachment 214509 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 214493 [details] work in progress Attachment 214493 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4111533 New failing tests: editing/execCommand/4641880-2.html editing/deleting/delete-ws-fixup-002.html editing/selection/character-data-mutation.html editing/style/style-3690704-fix.html editing/execCommand/4641880-1.html editing/deleting/smart-delete-003.html editing/deleting/smart-delete-004.html editing/style/style-3681552-fix-001.html
Created attachment 214519 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 214653 [details] Patch
Comment on attachment 214653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214653&action=review > Source/WebCore/ChangeLog:8 > + Before this patch When selection is done by double-click(start and base remain Nit: "When" and "double-click(". > Source/WebCore/ChangeLog:15 > + the selection. Since in double-click case start/end contain the > + correct selection, same should be used after dom modification to > + update selection. I don't think always using start & end is correct. > Source/WebCore/ChangeLog:21 > + (WebCore::FrameSelection::textWasReplaced): use start/end to update > + selection Why is that always correct in cases where selection was not made by double click?
Created attachment 214663 [details] Patch
> > Source/WebCore/ChangeLog:15 > > + the selection. Since in double-click case start/end contain the > > + correct selection, same should be used after dom modification to > > + update selection. > > I don't think always using start & end is correct. probably you are right > > > Source/WebCore/ChangeLog:21 > > + (WebCore::FrameSelection::textWasReplaced): use start/end to update > > + selection > > Why is that always correct in cases where selection was not made by double click? I also think we should not apply start/end in all cases. it correctness is guaranteed in double-click case only.
Comment on attachment 214663 [details] Patch Attachment 214663 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6438035 New failing tests: editing/style/style-3690704-fix.html
Created attachment 214667 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 214671 [details] Patch
Comment on attachment 214671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214671&action=review > Source/WebCore/editing/FrameSelection.cpp:470 > - newSelection.setWithoutValidation(base, extent); > + if (base != extent) > + newSelection.setWithoutValidation(base, extent); > + else { > + if (m_selection.isBaseFirst()) > + newSelection.setWithoutValidation(start, end); > + else > + newSelection.setWithoutValidation(end, start); > + } Looks like we also need to call setIsDirectional here. > LayoutTests/editing/selection/double-click-selection-with-dom-mutation-expected.txt:3 > +before: Why is "before" not capitalized? > LayoutTests/editing/selection/double-click-selection-with-dom-mutation.html:4 > + <meta charset="utf-8"> Do we really need this? > LayoutTests/editing/selection/double-click-selection-with-dom-mutation.html:26 > + if (workaroundPreserveEnd) { // the original node should contain its original end > + newNode.data = container.data.slice(0, offset); > + container.deleteData(0, offset); > + container.parentNode.insertBefore(newNode, container); > + } else { // the original node should contain its original beginning We don't normally align comments like.
Created attachment 214701 [details] Patch
> > > LayoutTests/editing/selection/double-click-selection-with-dom-mutation.html:4 > > + <meta charset="utf-8"> > > Do we really need this? >yes, to display arrow around "this". > > LayoutTests/editing/selection/double-click-selection-with-dom-mutation.html:26 > > + if (workaroundPreserveEnd) { // the original node should contain its original end > > + newNode.data = container.data.slice(0, offset); > > + container.deleteData(0, offset); > > + container.parentNode.insertBefore(newNode, container); > > + } else { // the original node should contain its original beginning > > We don't normally align comments like. cleaned.. @rniwa: Please review further..
Comment on attachment 214701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214701&action=review > Source/WebCore/ChangeLog:10 > + same) and DOM is modified then selection gets vanished, this does not > + happen when selection is done by dragging mouse. This happens because This is a run-on sentence. Please either end the first sentence "~gets vanished" with a period or a semicolon. > Source/WebCore/ChangeLog:21 > + (WebCore::FrameSelection::textWasReplaced): use start/end to update > + selection in case double click selection. We need a better description as so why we check base != extent, etc... > Source/WebCore/editing/FrameSelection.cpp:463 > + if (base != extent || !m_selection.isRange()) Where did !m_selection.isRange() come from? > Source/WebCore/editing/FrameSelection.cpp:465 > + else if (m_selection.isDirectional() && !m_selection.isBaseFirst()) This is not want I meant. I meant that you need to do newSelectino.setIsDirectional(m_selection.isDirectional());
(In reply to comment #19) > (From update of attachment 214701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214701&action=review > > > Source/WebCore/ChangeLog:10 > > + same) and DOM is modified then selection gets vanished, this does not > > + happen when selection is done by dragging mouse. This happens because > > This is a run-on sentence. Please either end the first sentence "~gets vanished" with a period or a semicolon. > > > Source/WebCore/ChangeLog:21 > > + (WebCore::FrameSelection::textWasReplaced): use start/end to update > > + selection in case double click selection. > > We need a better description as so why we check base != extent, etc... Will take care of this. thanks > > Source/WebCore/editing/FrameSelection.cpp:463 > > + if (base != extent || !m_selection.isRange()) > > Where did !m_selection.isRange() come from? I was trying to put better check here. In case of caret selection also base == range. So in case of caret selection i wanted to use selection update by base/extent. I mean i wanted to update the selection by start/end only if base == extent and its range selection. Isn't it ok?? > > Source/WebCore/editing/FrameSelection.cpp:465 > > + else if (m_selection.isDirectional() && !m_selection.isBaseFirst()) > > This is not want I meant. I meant that you need to do newSelectino.setIsDirectional(m_selection.isDirectional()); I got. yeah this should be done. Thanks for pointing.
Comment on attachment 214701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214701&action=review >>> Source/WebCore/editing/FrameSelection.cpp:463 >>> + if (base != extent || !m_selection.isRange()) >> >> Where did !m_selection.isRange() come from? > > I was trying to put better check here. > In case of caret selection also base == range. So in case of caret selection i wanted to use selection update by base/extent. > > I mean i wanted to update the selection by start/end only if base == extent and its range selection. > > Isn't it ok?? But why?
(In reply to comment #21) > (From update of attachment 214701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214701&action=review > > >>> Source/WebCore/editing/FrameSelection.cpp:463 > >>> + if (base != extent || !m_selection.isRange()) > >> > >> Where did !m_selection.isRange() come from? > > > > I was trying to put better check here. > > In case of caret selection also base == range. So in case of caret selection i wanted to use selection update by base/extent. > > > > I mean i wanted to update the selection by start/end only if base == extent and its range selection. > > > > Isn't it ok?? > > But why? I wanted my case(update selection start/end ) to be handled only in double-click case. Without !m_selection.isRange() Below code will be executed in two case : 1. double-click selection and dom modification and 2. caret selection and dom updation. I just wanted keep old behavior for caret selection. else if (m_selection.isDirectional() && !m_selection.isBaseFirst()) newSelection.setWithoutValidation(end, start); else newSelection.setWithoutValidation(start, end); But now i think start/end is better even in case of caret selection. So will remove the !m_selection.isRange()
Created attachment 214981 [details] Patch
Comment on attachment 214981 [details] Patch Attachment 214981 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/10308003 New failing tests: editing/deleting/non-smart-delete.html editing/deleting/delete-ws-fixup-004.html editing/pasteboard/cut-text-001.html editing/pasteboard/4076267-2.html editing/deleting/delete-at-paragraph-boundaries-008.html editing/deleting/delete-ws-fixup-003.html editing/deleting/delete-contiguous-ws-001.html editing/deleting/delete-and-undo.html editing/deleting/paragraph-in-preserveNewline.html editing/deleting/delete-trailing-ws-001.html editing/pasteboard/4076267.html editing/deleting/whitespace-pre-1.html editing/deleting/delete-selection-001.html
Created attachment 214989 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 214981 [details] Patch Attachment 214981 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/10318013 New failing tests: editing/deleting/non-smart-delete.html editing/deleting/delete-ws-fixup-004.html editing/pasteboard/cut-text-001.html editing/pasteboard/4076267-2.html editing/deleting/delete-at-paragraph-boundaries-008.html editing/deleting/delete-ws-fixup-003.html editing/deleting/delete-contiguous-ws-001.html editing/deleting/delete-and-undo.html editing/deleting/paragraph-in-preserveNewline.html editing/deleting/delete-trailing-ws-001.html editing/pasteboard/4076267.html editing/deleting/whitespace-pre-1.html editing/deleting/delete-selection-001.html
Created attachment 214996 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 214981 [details] Patch Attachment 214981 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/10408010 New failing tests: editing/deleting/non-smart-delete.html editing/deleting/delete-ws-fixup-004.html editing/pasteboard/cut-text-001.html editing/pasteboard/4076267-2.html editing/deleting/delete-at-paragraph-boundaries-008.html editing/deleting/delete-ws-fixup-003.html editing/deleting/delete-contiguous-ws-001.html editing/deleting/delete-and-undo.html editing/deleting/paragraph-in-preserveNewline.html editing/deleting/delete-trailing-ws-001.html editing/pasteboard/4076267.html editing/deleting/whitespace-pre-1.html editing/deleting/delete-selection-001.html
Created attachment 215000 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
(In reply to comment #28) > (From update of attachment 214981 [details]) > Attachment 214981 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/10408010 > > New failing tests: > editing/deleting/non-smart-delete.html > editing/deleting/delete-ws-fixup-004.html > editing/pasteboard/cut-text-001.html > editing/pasteboard/4076267-2.html > editing/deleting/delete-at-paragraph-boundaries-008.html > editing/deleting/delete-ws-fixup-003.html > editing/deleting/delete-contiguous-ws-001.html > editing/deleting/delete-and-undo.html > editing/deleting/paragraph-in-preserveNewline.html > editing/deleting/delete-trailing-ws-001.html > editing/pasteboard/4076267.html > editing/deleting/whitespace-pre-1.html > editing/deleting/delete-selection-001.html All failed test cases has text diff in editing callbacks calls. But final behavior is not changed(no diff in renderTree dump). It appears to me extra redundant editing callback been has been removed from original test expected output. is it ok to update these expectation? or any concerns are there.
(In reply to comment #30) > (In reply to comment #28) > > (From update of attachment 214981 [details] [details]) > > Attachment 214981 [details] [details] did not pass mac-ews (mac): > > Output: http://webkit-queues.appspot.com/results/10408010 > > All failed test cases has text diff in editing callbacks calls. > But final behavior is not changed(no diff in renderTree dump). It appears to me extra redundant editing callback been has been removed from original test expected output. > > is it ok to update these expectation? or any concerns are there. That's the diff? We certainly shouldn't be landing the patch as is without understanding what kind of failures we're seeing.
(In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #28) > > > (From update of attachment 214981 [details] [details] [details]) > > > Attachment 214981 [details] [details] [details] did not pass mac-ews (mac): > > > Output: http://webkit-queues.appspot.com/results/10408010 > > > > All failed test cases has text diff in editing callbacks calls. > > But final behavior is not changed(no diff in renderTree dump). It appears to me extra redundant editing callback been has been removed from original test expected output. > > > > is it ok to update these expectation? or any concerns are there. > > That's the diff? We certainly shouldn't be landing the patch as is without understanding what kind of failures we're seeing. Ok I will debug for this failure. One second thought i have, tests are failing after i added newSelection.setIsDirectional(m_selection.isDirectional()); --> as your suggestion. This is extra update that we are pushing with this patch. I mean this line will not effect current bug fix. Could I remove this line for sake of this bug to be fixed without any failing tests. Definitely we can update directionality later with another bug name and proper test case for that.(something like missing directionlity after DOM updation) What you say??
Created attachment 215209 [details] Patch
(In reply to comment #33) > Created an attachment (id=215209) [details] > Patch Hi rniwa, I have updated accorodingly, Could you look further,
Comment on attachment 215209 [details] Patch Clearing flags on attachment: 215209 Committed r158186: <http://trac.webkit.org/changeset/158186>
All reviewed patches have been landed. Closing bug.