Bug 114227 – Text selected with double-click gets unselected after DOM modification

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Nathan Vander Wilt]

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.

[Nathan Vander Wilt]

Created attachment 196978 [details]
Fixed version of previous attachment (removed temporary test stuff interfering with intended demo)

[Santosh Mahto]

Created attachment 214493 [details]
work in progress

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Santosh Mahto]

Created attachment 214653 [details]
Patch

[Ryosuke Niwa]

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?

[Santosh Mahto]

Created attachment 214663 [details]
Patch

[Santosh Mahto]

> > 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.

[Build Bot]

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

[Build Bot]

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

[Santosh Mahto]

Created attachment 214671 [details]
Patch

[Ryosuke Niwa]

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.

[Santosh Mahto]

Created attachment 214701 [details]
Patch

[Santosh Mahto]

> 
> > 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..

[Ryosuke Niwa]

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());

[Santosh Mahto]

(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.

[Ryosuke Niwa]

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?

[Santosh Mahto]

(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()

[Santosh Mahto]

Created attachment 214981 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Santosh Mahto]

(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.

[Ryosuke Niwa]

(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.

[Santosh Mahto]

(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??

[Santosh Mahto]

Created attachment 215209 [details]
Patch

[Santosh Mahto]

(In reply to comment #33)
> Created an attachment (id=215209) [details]
> Patch

Hi rniwa,
I have updated accorodingly, Could you look further,

[WebKit Commit Bot]

Comment on attachment 215209 [details]
Patch

Clearing flags on attachment: 215209

Committed r158186: <http://trac.webkit.org/changeset/158186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.