Bug 171585 – [WK2] Add support for keeping the selection in a focused editable element when dragging begins

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/31544320>

[Wenson Hsieh]

Created attachment 308888 [details]
First pass

[Wenson Hsieh]

Created attachment 308889 [details]
Fix OpenSource iOS build

[Wenson Hsieh]

Created attachment 308926 [details]
Handle multiple dragged content ranges in RenderText

[Beth Dakin]

Comment on attachment 308926 [details]
Handle multiple dragged content ranges in RenderText

View in context: https://bugs.webkit.org/attachment.cgi?id=308926&action=review

r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a look at some point.

> Source/WebCore/dom/DocumentMarkerController.cpp:142
> +        DocumentMarker::DraggedContentData draggedContentData { markedText.node() };

Could textPiece ever be null? If not, this is okay. If so, you should add a null check.

[alan]

(In reply to Beth Dakin from comment #4)
> Comment on attachment 308926 [details]
> Handle multiple dragged content ranges in RenderText
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308926&action=review
> 
> r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a
> look at some point.
Wenson and I had a quick chat about textEffectsModifier logic.

[Beth Dakin]

(In reply to zalan from comment #5)
> (In reply to Beth Dakin from comment #4)
> > Comment on attachment 308926 [details]
> > Handle multiple dragged content ranges in RenderText
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=308926&action=review
> > 
> > r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a
> > look at some point.
> Wenson and I had a quick chat about textEffectsModifier logic.

👍

[Wenson Hsieh]

Created attachment 309010 [details]
Remove TextEffectsRange, move relevant logic to InlineTextBox.

[Wenson Hsieh]

Created attachment 309011 [details]
Remove TextEffectsRange, move relevant logic to InlineTextBox.

[Wenson Hsieh]

Created attachment 309012 [details]
Rebased on master

[Wenson Hsieh]

(In reply to Beth Dakin from comment #4)
> Comment on attachment 308926 [details]
> Handle multiple dragged content ranges in RenderText
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308926&action=review
> 
> r=me, though it would be good to get Simon, Myles, Dean, or Alan to take a
> look at some point.
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:142
> > +        DocumentMarker::DraggedContentData draggedContentData { markedText.node() };
> 
> Could textPiece ever be null? If not, this is okay. If so, you should add a
> null check.

I think this will be ok. Even if the target node is null, this will just initialize the  DraggedContentData to have a null targetNode (a RefPtr). In the two places where we use this, (shouldInsertAsSeparateMarker and draggedContentContainsReplacedElement), we handle the !targetNode case gracefully, and won't crash trying to dereference null.

[Wenson Hsieh]

Created attachment 309014 [details]
Rebased on master (again)

[Build Bot]

Comment on attachment 309014 [details]
Rebased on master (again)

Attachment 309014 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3670184

New failing tests:
media/modern-media-controls/media-documents/background-color-and-centering.html

[Build Bot]

Created attachment 309024 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Wenson Hsieh]

Created attachment 309095 [details]
Uploading for EWS pass

[WebKit Commit Bot]

Comment on attachment 309095 [details]
Uploading for EWS pass

Clearing flags on attachment: 309095

Committed r216212: <http://trac.webkit.org/changeset/216212>

[Simon Fraser (smfr)]

Comment on attachment 308926 [details]
Handle multiple dragged content ranges in RenderText

View in context: https://bugs.webkit.org/attachment.cgi?id=308926&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:555
> +    auto draggedContentRanges = renderer().draggedContentRangesBetweenOffsets(m_start, m_start + m_len);

It's a shame that we're slowing down the paint code for something that almost never happens. draggedContentRangesBetweenOffsets() shows up on profiles. It's small, but still, is there a better way to avoid this work when the user isn't dragging?

[Tim Horton]

File a new bug! There’s surely a way.