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.
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
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?
> > 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.
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
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.
>
> > 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 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
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
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??
2013-04-08 17:52 PDT, Nathan Vander Wilt
2013-04-08 18:01 PDT, Nathan Vander Wilt
2013-10-17 13:09 PDT, Santosh Mahto
2013-10-17 13:59 PDT, Build Bot
2013-10-17 14:19 PDT, Build Bot
2013-10-17 15:21 PDT, Build Bot
2013-10-19 08:37 PDT, Santosh Mahto
2013-10-19 13:14 PDT, Santosh Mahto
2013-10-19 13:57 PDT, Build Bot
2013-10-19 14:24 PDT, Santosh Mahto
2013-10-20 07:56 PDT, Santosh Mahto
2013-10-23 12:30 PDT, Santosh Mahto
2013-10-23 13:40 PDT, Build Bot
2013-10-23 14:12 PDT, Build Bot
2013-10-23 14:47 PDT, Build Bot
2013-10-25 14:08 PDT, Santosh Mahto