Bug 186792 – Restrict Selection in contenteditable to the extent of that contenteditable

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Restrict Selection in contenteditable the extent of that contenteditable

[Megan Gardner]

Created attachment 342996 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Megan Gardner]

Created attachment 343065 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

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.

[Megan Gardner]

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.

[Megan Gardner]

Created attachment 343107 [details]
Patch

[Megan Gardner]

<rdar://problem/40277487>

[WebKit Commit Bot]

Comment on attachment 343107 [details]
Patch

Clearing flags on attachment: 343107

Committed r233016: <https://trac.webkit.org/changeset/233016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.