Bug 186792

Summary: Restrict Selection in contenteditable to the extent of that contenteditable
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, ews-watchlist, rniwa, simon.fraser, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Patch none

Description Megan Gardner 2018-06-18 17:40:42 PDT
Restrict Selection in contenteditable the extent of that contenteditable
Comment 1 Megan Gardner 2018-06-18 17:43:57 PDT
Created attachment 342996 [details]
Patch
Comment 2 EWS Watchlist 2018-06-19 02:59:55 PDT
Comment on attachment 342996 [details]
Patch

Attachment 342996 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8244791

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
http/tests/preload/onload_event.html
Comment 3 EWS Watchlist 2018-06-19 03:00:06 PDT
Created attachment 343038 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 4 Megan Gardner 2018-06-19 10:40:10 PDT
Created attachment 343065 [details]
Patch
Comment 5 EWS Watchlist 2018-06-19 11:53:11 PDT
Comment on attachment 343065 [details]
Patch

Attachment 343065 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8249991

New failing tests:
accessibility/mac/selection-notification-focus-change.html
Comment 6 EWS Watchlist 2018-06-19 11:53:12 PDT
Created attachment 343071 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 7 Wenson Hsieh 2018-06-19 14:21:44 PDT
Comment on attachment 343065 [details]
Patch

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

This looks reasonable to me. Can we write a test for this?

> Source/WebKit/ChangeLog:9
> +        a single content editable on iOS. There is functionality to esure this on mac, so it has been

Nit - esure

> Source/WebKit/ChangeLog:10
> +        exposed and utalized for restricting the extent of a selection.

Nit - utalized

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1339
> +        VisiblePosition targetPosition = m_page->mainFrame().eventHandler().selectionExtentRespectingEditingBoundary(frame.selection().selection(), result.localPoint(), result.targetNode());

Nit - I don't think this local variable adds much.

Also, I don't think we're guaranteed to find a target node when hit-testing, yet selectionExtentRespectingEditingBoundary requires a non-null targetNode. We should probably just make selectionExtentRespectingEditingBoundary take a Node& and add a null check here.
Comment 8 Megan Gardner 2018-06-19 15:05:23 PDT
Comment on attachment 343065 [details]
Patch

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

As I said in the WebCore log, my system currently won't allow me to run any tests, but I will write one as soon as I can update.

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1339
>> +        VisiblePosition targetPosition = m_page->mainFrame().eventHandler().selectionExtentRespectingEditingBoundary(frame.selection().selection(), result.localPoint(), result.targetNode());
> 
> Nit - I don't think this local variable adds much.
> 
> Also, I don't think we're guaranteed to find a target node when hit-testing, yet selectionExtentRespectingEditingBoundary requires a non-null targetNode. We should probably just make selectionExtentRespectingEditingBoundary take a Node& and add a null check here.

There's too much in selectionExtentRespectingEditingBoundary that relies on taking a Node*, but I will add null checks both here and in the function.
Comment 9 Megan Gardner 2018-06-19 15:07:33 PDT
Created attachment 343107 [details]
Patch
Comment 10 Megan Gardner 2018-06-20 09:56:01 PDT
<rdar://problem/40277487>
Comment 11 WebKit Commit Bot 2018-06-20 10:23:03 PDT
Comment on attachment 343107 [details]
Patch

Clearing flags on attachment: 343107

Committed r233016: <https://trac.webkit.org/changeset/233016>
Comment 12 WebKit Commit Bot 2018-06-20 10:23:05 PDT
All reviewed patches have been landed.  Closing bug.