Bug 171585 - [WK2] Add support for keeping the selection in a focused editable element when dragging begins
Summary: [WK2] Add support for keeping the selection in a focused editable element whe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-02 17:57 PDT by Wenson Hsieh
Modified: 2017-08-06 22:27 PDT (History)
10 users (show)

See Also:


Attachments
First pass (51.45 KB, patch)
2017-05-02 20:55 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix OpenSource iOS build (51.38 KB, patch)
2017-05-02 21:58 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Handle multiple dragged content ranges in RenderText (51.83 KB, patch)
2017-05-03 11:00 PDT, Wenson Hsieh
bdakin: review+
Details | Formatted Diff | Diff
Remove TextEffectsRange, move relevant logic to InlineTextBox. (48.48 KB, patch)
2017-05-03 21:20 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Remove TextEffectsRange, move relevant logic to InlineTextBox. (48.52 KB, patch)
2017-05-03 21:23 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebased on master (48.87 KB, patch)
2017-05-03 21:30 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebased on master (again) (48.89 KB, patch)
2017-05-03 21:55 PDT, Wenson Hsieh
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.86 MB, application/zip)
2017-05-04 00:45 PDT, Build Bot
no flags Details
Uploading for EWS pass (48.83 KB, patch)
2017-05-04 13:12 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2017-05-02 17:57:20 PDT
<rdar://problem/31544320>
Comment 1 Wenson Hsieh 2017-05-02 20:55:22 PDT
Created attachment 308888 [details]
First pass
Comment 2 Wenson Hsieh 2017-05-02 21:58:58 PDT
Created attachment 308889 [details]
Fix OpenSource iOS build
Comment 3 Wenson Hsieh 2017-05-03 11:00:47 PDT
Created attachment 308926 [details]
Handle multiple dragged content ranges in RenderText
Comment 4 Beth Dakin 2017-05-03 14:19:48 PDT
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.
Comment 5 zalan 2017-05-03 14:34:29 PDT
(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.
Comment 6 Beth Dakin 2017-05-03 14:40:49 PDT
(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.

๐Ÿ‘
Comment 7 Wenson Hsieh 2017-05-03 21:20:08 PDT
Created attachment 309010 [details]
Remove TextEffectsRange, move relevant logic to InlineTextBox.
Comment 8 Wenson Hsieh 2017-05-03 21:23:39 PDT
Created attachment 309011 [details]
Remove TextEffectsRange, move relevant logic to InlineTextBox.
Comment 9 Wenson Hsieh 2017-05-03 21:30:09 PDT
Created attachment 309012 [details]
Rebased on master
Comment 10 Wenson Hsieh 2017-05-03 21:35:37 PDT
(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.
Comment 11 Wenson Hsieh 2017-05-03 21:55:02 PDT
Created attachment 309014 [details]
Rebased on master (again)
Comment 12 Build Bot 2017-05-04 00:45:38 PDT
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
Comment 13 Build Bot 2017-05-04 00:45:39 PDT
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
Comment 14 Wenson Hsieh 2017-05-04 13:12:50 PDT
Created attachment 309095 [details]
Uploading for EWS pass
Comment 15 WebKit Commit Bot 2017-05-04 15:28:26 PDT
Comment on attachment 309095 [details]
Uploading for EWS pass

Clearing flags on attachment: 309095

Committed r216212: <http://trac.webkit.org/changeset/216212>
Comment 16 Simon Fraser (smfr) 2017-08-06 21:44:38 PDT
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?
Comment 17 Tim Horton 2017-08-06 22:27:28 PDT
File a new bug! Thereโ€™s surely a way.