While resolving Bug 143958 I introduced a problem with using the context menu to paste content into an editable fields. With the new changes, if you right-clicked in a whitespace area with the intention of pasting content, the new selection logic will hop over the whitespace and grab the preceding word. To fix this, we should use the original behavior with editable fields so that we can leave our insertion point where it is when pasting.
<rdar://problem/21441466>
Created attachment 256324 [details] Patch
Committed r186480: <http://trac.webkit.org/changeset/186480>
No test?
Comment on attachment 256324 [details] Patch Can we get a layout test for both modes?
Reopening to attach new patch.
Created attachment 256389 [details] Patch
Comment on attachment 256389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256389&action=review > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:27 > + var x = input.offsetParent.offsetLeft + input.offsetLeft + 200; > + var y = input.offsetParent.offsetTop + input.offsetTop + input.offsetHeight / 2; IIRC in previous tests, I used <spans> to give JS something to target so that you could get rid of the 200 here and just read offsets/clientRects/whatever (and use the center). Might be less fragile.
Comment on attachment 256389 [details] Patch Attachment 256389 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5844986906017792 New failing tests: platform/mac/editing/selection/context-menu-select-editability.html
Created attachment 256392 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 256389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256389&action=review >> LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:27 >> + var y = input.offsetParent.offsetTop + input.offsetTop + input.offsetHeight / 2; > > IIRC in previous tests, I used <spans> to give JS something to target so that you could get rid of the 200 here and just read offsets/clientRects/whatever (and use the center). > > Might be less fragile. EWS shows that my test *is* fragile, so I will try this!
Created attachment 256483 [details] Patch
Comment on attachment 256483 [details] Patch Attachment 256483 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4514907475148800 New failing tests: platform/mac/editing/selection/context-menu-select-editability.html
Created attachment 256488 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 256500 [details] Patch v3 (Should not timeout even in failing case)
Comment on attachment 256500 [details] Patch v3 (Should not timeout even in failing case) View in context: https://bugs.webkit.org/attachment.cgi?id=256500&action=review > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:40 > + { This { is not where it belongs.
Created attachment 256501 [details] Patch v4 (Guard against undefined selections)
Created attachment 256502 [details] Patch
Comment on attachment 256502 [details] Patch Attachment 256502 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5623346192449536 New failing tests: platform/mac/editing/selection/context-menu-select-editability.html
Created attachment 256508 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 256502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256502&action=review Looks like the test is failing on the bot? > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:32 > + var target = document.getElementById("readOnlyWhitespace"); > + > + var x = target.offsetParent.offsetLeft + target.offsetLeft + target.clientWidth / 2; > + var y = target.offsetParent.offsetTop + target.offsetTop + target.clientHeight / 2; > + > + if (!window.eventSender) { > + testRunner.notifyDone(); > + return; > + } > + Can we extract a helper function to do this? It's not great to repeat it here and in editableTest > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:43 > + var selection = window.getSelection(); > + if (typeof selection != 'undefined') { > + readOnlySelectionContents = selection.toString(); > + } else { > + readOnlySelectionContents = ''; > + } window.getSelection() is never undefined. > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:56 > + readOnlySelectionContents = window.getSelection().toString(); > + if (readOnlySelectionContents.length) { > + readOnlySelectionContents.trim(); > + > + // We expect it to select 'New York, New York' based on dictionary lookup > + if (readOnlySelectionContents == "New York, New York") > + testPassed('ReadOnly: SUCCESS'); > + else > + testFailed('Did not get "New York, New York". Got: ' + readOnlySelectionContents); > + } else { > + testFailed("Read-only selection had no range."); > + } Why don't we just do: shouldBe("getSelection().toString().trim()", '"New York, New York"') instead? > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:71 > + if (!window.eventSender) { > + testRunner.notifyDone(); > + return; > + } We should check this condition in onLoad() function instead.
Comment on attachment 256502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256502&action=review > LayoutTests/platform/mac/editing/selection/context-menu-select-editability-expected.txt:15 > +TEST COMPLETE > + > + > + > +PASS Editable: SUCCESS > +PASS ReadOnly: SUCCESS It's wrong to have PASS output after TEST COMPLETE. Please use jsTestIsAsync and finishJSTest() instead of testRunner methods. > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:101 > + testRunner.dumpAsText(); This is also unneeded, js-test-pre does that automatically. > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:105 > + setTimeout(editableTest, 0); This looks suspect. What is the operation that we need to wait for after loading?
The timeouts in my test case are caused by Bug 146836.
Created attachment 256619 [details] Patch
Created attachment 256622 [details] Patch (Reuploaded to run EWS now that Bug 146695 is fixed)
Comment on attachment 256622 [details] Patch (Reuploaded to run EWS now that Bug 146695 is fixed) View in context: https://bugs.webkit.org/attachment.cgi?id=256622&action=review > LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:30 > + // esc key to kill the context menu > + eventSender.keyDown(String.fromCharCode(0x001B), null); Do you still need this now that contextClick doesn't actually pop a context menu?
Comment on attachment 256622 [details] Patch (Reuploaded to run EWS now that Bug 146695 is fixed) View in context: https://bugs.webkit.org/attachment.cgi?id=256622&action=review >> LayoutTests/platform/mac/editing/selection/context-menu-select-editability.html:30 >> + eventSender.keyDown(String.fromCharCode(0x001B), null); > > Do you still need this now that contextClick doesn't actually pop a context menu? You're right. I'll take this out.
Committed r186696: <http://trac.webkit.org/changeset/186696>