REGRESSION (r183133-r183138): Secondary clicking in whitespace selects preceding word

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/21441466>

[Brent Fulgham]

Created attachment 256324 [details]
Patch

[Brent Fulgham]

Committed r186480: <http://trac.webkit.org/changeset/186480>

[Simon Fraser (smfr)]

No test?

[Tim Horton]

Comment on attachment 256324 [details]
Patch

Can we get a layout test for both modes?

[Brent Fulgham]

Reopening to attach new patch.

[Brent Fulgham]

Created attachment 256389 [details]
Patch

[Tim Horton]

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.

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

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!

[Brent Fulgham]

Created attachment 256483 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 256500 [details]
Patch v3 (Should not timeout even in failing case)

[Tim Horton]

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.

[Brent Fulgham]

Created attachment 256501 [details]
Patch v4 (Guard against undefined selections)

[Brent Fulgham]

Created attachment 256502 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

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.

[Alexey Proskuryakov]

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?

[Brent Fulgham]

The timeouts in my test case are caused by Bug 146836.

[Brent Fulgham]

Created attachment 256619 [details]
Patch

[Brent Fulgham]

Created attachment 256622 [details]
Patch (Reuploaded to run EWS now that Bug 146695 is fixed)

[Tim Horton]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

Committed r186696: <http://trac.webkit.org/changeset/186696>