Skip to content
Permalink
Browse files
Picking an emoji via the emoji dialog (Ctrl+Cmd+Space) fires inconsis…
…tent beforeinput events.

https://bugs.webkit.org/show_bug.cgi?id=170955
<rdar://problem/31697653>

Reviewed by Ryosuke Niwa.

Source/WebKit:

Currently, we insert text with TextEventInputAutocompletion as the text event input type if any text range to
replace was specified by the platform. Instead, limit this only to when the text replacement range is not empty.
This more closely matches the intention of the spec, which states that the "insertReplacementText" inputType
should be used when "[replacing] existing text by means of a spell checker, auto-correct or similar".

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::insertTextAsync):

Source/WebKitLegacy/mac:

Tweak -insertText: to pass TextEventInputAutocompletion to Editor::insertText when inserting text, if existing
text is being replaced.

* WebView/WebHTMLView.mm:
(-[WebHTMLView insertText:]):

Tools:

Replace UIScriptController.insertText with UIScriptController.replaceTextAtRange, and implement
replaceTextAtRange in WebKit1. See corresponding layout tests (input-event-insert-replacement.html and
before-input-prevent-insert-replacement.html) for more detail.

* DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:
* DumpRenderTree/mac/AppKitTestSPI.h: Added.

Introduce an SPI header for private AppKit headers needed to support DumpRenderTree.

* DumpRenderTree/mac/UIScriptControllerMac.mm:
(WTR::UIScriptController::replaceTextAtRange):
(WTR::UIScriptController::insertText): Deleted.
* TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
* TestRunnerShared/UIScriptContext/UIScriptController.cpp:
(WTR::UIScriptController::replaceTextAtRange):
(WTR::UIScriptController::insertText): Deleted.
* TestRunnerShared/UIScriptContext/UIScriptController.h:
* WebKitTestRunner/mac/UIScriptControllerMac.mm:
(WTR::UIScriptController::replaceTextAtRange):
(WTR::UIScriptController::insertText): Deleted.

Replace UIScriptController.insertText with UIScriptController.replaceTextAtRange, which better describes the
behavior of this function.

LayoutTests:

Augments two existing layout tests to check for additional cases of inserting text with replacement ranges.
Also enables this test for WebKit1 on Mac. Both these tests are currently enabled only for WebKit2, and also only
check the case where we're replacing an existing non-empty range of text.

* fast/events/before-input-prevent-insert-replacement-expected.txt:
* fast/events/before-input-prevent-insert-replacement.html:
* fast/events/input-event-insert-replacement-expected.txt:
* fast/events/input-event-insert-replacement.html:

Tests for cases of replacing existing text ranges, and inserting text at a position.

* platform/mac-wk1/TestExpectations:
* resources/ui-helper.js:

Add a new UIHelper function to insert text at a given replacement range. This codepath is taken when selecting
an emoji using the emoji picker menu on Mac, and also when selecting a dead key option after holding down on a
vowel key.

(window.UIHelper.replaceTextAtRange):


Canonical link: https://commits.webkit.org/192681@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@221234 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
whsieh committed Aug 28, 2017
1 parent 1e123bb commit 97605ecfc6903dd81033b2cab86ae6efcc01e70b
Showing 19 changed files with 289 additions and 132 deletions.
@@ -1,3 +1,31 @@
2017-08-27 Wenson Hsieh <wenson_hsieh@apple.com>

Picking an emoji via the emoji dialog (Ctrl+Cmd+Space) fires inconsistent beforeinput events.
https://bugs.webkit.org/show_bug.cgi?id=170955
<rdar://problem/31697653>

Reviewed by Ryosuke Niwa.

Augments two existing layout tests to check for additional cases of inserting text with replacement ranges.
Also enables this test for WebKit1 on Mac. Both these tests are currently enabled only for WebKit2, and also only
check the case where we're replacing an existing non-empty range of text.

* fast/events/before-input-prevent-insert-replacement-expected.txt:
* fast/events/before-input-prevent-insert-replacement.html:
* fast/events/input-event-insert-replacement-expected.txt:
* fast/events/input-event-insert-replacement.html:

Tests for cases of replacing existing text ranges, and inserting text at a position.

* platform/mac-wk1/TestExpectations:
* resources/ui-helper.js:

Add a new UIHelper function to insert text at a given replacement range. This codepath is taken when selecting
an emoji using the emoji picker menu on Mac, and also when selecting a dead key option after holding down on a
vowel key.

(window.UIHelper.replaceTextAtRange):

2017-08-27 Devin Rousso <webkit@devinrousso.com>

Web Inspector: Record actions performed on WebGLRenderingContext
@@ -1,7 +1,20 @@
a
Typing 'a'...
(editable): type=beforeinput, inputType=insertText, data=`a`, dataTransfer=`null`
(editable): type=input, inputType=insertText, data=`a`, dataTransfer=`null`
Attempting to replace 'a' with 'b'...
(editable): type=beforeinput, inputType=insertReplacementText, data=`null`, dataTransfer=`plain:"b", html:"b"`
To manually test, press and hold down 'a' and select one of the accented characters.
You should observe that the replacement accented character does not replace 'a'.
Then insert an emoji using the emoji picker. This should insert something into the editor.
Finally, select all the content and attempt to replace it with an emoji. This should do nothing.
ac
(1) Typing 'a'...
(editable): type=beforeinput, inputType=insertText, data=a, dataTransfer=(null)
(editable): type=input, inputType=insertText, data=a, dataTransfer=(null)
The editor now has text content: a
(2) Preventing default when replacing 'a' with 'b'...
(editable): type=beforeinput, inputType=insertReplacementText, data=null, dataTransfer=plain:"b", html:"b"
The editor now has text content: a
(3) Inserting 'c' after 'a'...
(editable): type=beforeinput, inputType=insertText, data=c, dataTransfer=(null)
(editable): type=input, inputType=insertText, data=c, dataTransfer=(null)
The editor now has text content: ac
(4) Selecting all and preventing replacement with 'd'...
(editable): type=beforeinput, inputType=insertReplacementText, data=null, dataTransfer=plain:"d", html:"d"
The editor now has text content: ac

@@ -1,60 +1,52 @@
<!DOCTYPE html>
<html>
<body>
<div id="editable" onbeforeinput=handleInputEvent(event) oninput=handleInputEvent(event) contenteditable></div>
<div id="output"></div>
<script type="text/javascript">
let write = s => output.innerHTML += s + "<br>";
var progress = 0;
editable.focus();
<head>
<script src="../../resources/ui-helper.js"></script>
</head>

if (window.internals && window.testRunner) {
internals.settings.setInputEventsEnabled(true);
testRunner.dumpAsText();
testRunner.waitUntilDone();
if (window.eventSender && testRunner.runUIScript) {
write("Typing 'a'...");
eventSender.keyDown("a");
write("Attempting to replace 'a' with 'b'...");
testRunner.runUIScript(getUIScript(), (result) => incrementProgress());
}
} else {
write("To manually test, press and hold down 'a' and select one of the accented characters.");
write("You should observe that the replacement accented character does not replace 'a'.");
}
<div>To manually test, press and hold down 'a' and select one of the accented characters.</div>
<div>You should observe that the replacement accented character does not replace 'a'.</div>
<div>Then insert an emoji using the emoji picker. This should insert something into the editor.</div>
<div>Finally, select all the content and attempt to replace it with an emoji. This should do nothing.</div>
<div id="editable" onbeforeinput=handleInputEvent(event) oninput=handleInputEvent(event) contenteditable></div>
<div id="output"></div>

function incrementProgress()
{
progress++;
if (progress == 2)
testRunner.notifyDone();
}
<script type="text/javascript">
let write = s => output.innerHTML += s + "<br>";
editable.focus();

function handleInputEvent(event)
{
write(`(${event.target.id}): type=${event.type}, inputType=${event.inputType}, data=\`${event.data}\`, dataTransfer=\`${dataTransferAsString(event.dataTransfer)}\``);
if (event.inputType === "insertReplacementText") {
event.preventDefault();
incrementProgress();
}
}
if (window.testRunner && window.eventSender && testRunner.runUIScript) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
runTest();
}

function dataTransferAsString(dataTransfer)
{
if (!dataTransfer)
return "null";
async function runTest() {
write("(1) Typing 'a'...");
eventSender.keyDown("a");
write(`The editor now has text content: ${editable.textContent}`);
write("(2) Preventing default when replacing 'a' with 'b'...");
await UIHelper.replaceTextAtRange("b", 0, 1)
write(`The editor now has text content: ${editable.textContent}`);
write("(3) Inserting 'c' after 'a'...");
await UIHelper.replaceTextAtRange("c", 1, 0);
write(`The editor now has text content: ${editable.textContent}`);
write("(4) Selecting all and preventing replacement with 'd'...");
document.execCommand("SelectAll")
await UIHelper.replaceTextAtRange("d", 0, 2);
write(`The editor now has text content: ${editable.textContent}`);

return `plain:"${dataTransfer.getData('text/plain')}", html:"${dataTransfer.getData('text/html')}"`;
}
testRunner.notifyDone();
}

function getUIScript()
{
return `
(function() {
uiController.insertText("b", 0, 1);
uiController.uiScriptComplete();
})();`
}
</script>
function handleInputEvent(event)
{
let dataTransferString = event.dataTransfer ? `plain:"${event.dataTransfer.getData('text/plain')}", html:"${event.dataTransfer.getData('text/html')}"` : "(null)";
write(`(${event.target.id}): type=${event.type}, inputType=${event.inputType}, data=${event.data}, dataTransfer=${dataTransferString}`);

if (event.type === "beforeinput" && event.inputType === "insertReplacementText")
event.preventDefault();
}
</script>
</body>
</html>
</html>
@@ -1,11 +1,23 @@
To manually test, press and hold down 'a' and select one of the accented characters."
You should observe a pair of beforeinput/input events for both 'a' and the replacement accented character."
Importantly, the inputType of these four events should be 'insertReplacementText'."
Then insert a single emoji character. You should observe beforeinput/input events for the inserted emoji."
Importantly, the inputType of these two events should be 'insertText'."

Typing 'a'...
(editable): type=beforeinput, inputType=insertText, data=`a`, dataTransfer=`null`
(editable): type=input, inputType=insertText, data=`a`, dataTransfer=`null`
(1) Typing 'a'...
(editable): type=beforeinput, inputType=insertText, data=a, dataTransfer=null
(editable): type=input, inputType=insertText, data=a, dataTransfer=null
The value of the input is now: a

Replacing 'a' with 'b'...
(editable): type=beforeinput, inputType=insertReplacementText, data=`b`, dataTransfer=`null`
(editable): type=input, inputType=insertReplacementText, data=`b`, dataTransfer=`null`
(2) Replacing 'a' with 'b'...
(editable): type=beforeinput, inputType=insertReplacementText, data=b, dataTransfer=null
(editable): type=input, inputType=insertReplacementText, data=b, dataTransfer=null
The value of the input is now: b
(3) Inserting 'c' after 'b'...
(editable): type=beforeinput, inputType=insertText, data=c, dataTransfer=null
(editable): type=input, inputType=insertText, data=c, dataTransfer=null
The value of the input is now: bc
(4) Selecting all and replacing with 'd'...
(editable): type=beforeinput, inputType=insertReplacementText, data=d, dataTransfer=null
(editable): type=input, inputType=insertReplacementText, data=d, dataTransfer=null
The value of the input is now: d

@@ -1,55 +1,47 @@
<!DOCTYPE html>
<html>
<body>
<input id="editable" onbeforeinput=logInputEvent(event) oninput=logInputEvent(event)></input>
<div id="output"></div>
<script type="text/javascript">
let write = s => output.innerHTML += s + "<br>";
var progress = 0;
editable.focus();
<head>
<script src="../../resources/ui-helper.js"></script>
</head>

if (window.internals && window.testRunner) {
internals.settings.setInputEventsEnabled(true);
testRunner.dumpAsText();
testRunner.waitUntilDone();
if (window.eventSender && testRunner.runUIScript) {
write("Typing 'a'...");
eventSender.keyDown("a");
write(`The value of the input is now: ${editable.value}`);
write("");
write("Replacing 'a' with 'b'...");
testRunner.runUIScript(getUIScript(), (result) => incrementProgress());
}
} else {
write("To manually test, press and hold down 'a' and select one of the accented characters.");
write("You should observe a pair of beforeinput/input events for both 'a' and the replacement accented character.");
write("Importantly, the inputType of the last two events should be 'insertReplacementText'.");
}
<div>To manually test, press and hold down 'a' and select one of the accented characters."</div>
<div>You should observe a pair of beforeinput/input events for both 'a' and the replacement accented character."</div>
<div>Importantly, the inputType of these four events should be 'insertReplacementText'."</div>
<div>Then insert a single emoji character. You should observe beforeinput/input events for the inserted emoji."</div>
<div>Importantly, the inputType of these two events should be 'insertText'."</div>

function incrementProgress()
{
progress++;
if (progress != 5)
return;
<input id="editable" onbeforeinput=logInputEvent(event) oninput=logInputEvent(event)></input>
<div id="output"></div>
<script type="text/javascript">
let write = s => output.innerHTML += s + "<br>";
editable.focus();
if (window.testRunner && window.eventSender && testRunner.runUIScript) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
runTest();
}

write(`The value of the input is now: ${editable.value}`);
testRunner.notifyDone();
}
function logInputEvent(event)
{
write(`(${event.target.id}): type=${event.type}, inputType=${event.inputType}, data=${event.data}, dataTransfer=${event.dataTransfer}`);
}

function logInputEvent(event)
{
write(`(${event.target.id}): type=${event.type}, inputType=${event.inputType}, data=\`${event.data}\`, dataTransfer=\`${event.dataTransfer}\``);
incrementProgress();
}
async function runTest() {
write("(1) Typing 'a'...");
eventSender.keyDown("a");
write(`The value of the input is now: ${editable.value}`);
write("(2) Replacing 'a' with 'b'...");
await UIHelper.replaceTextAtRange("b", 0, 1);
write(`The value of the input is now: ${editable.value}`);
write("(3) Inserting 'c' after 'b'...");
await UIHelper.replaceTextAtRange("c", 1, 0);
write(`The value of the input is now: ${editable.value}`);
write("(4) Selecting all and replacing with 'd'...");
document.execCommand("SelectAll")
await UIHelper.replaceTextAtRange("d", 0, 2);
write(`The value of the input is now: ${editable.value}`);

function getUIScript()
{
return `
(function() {
uiController.insertText("b", 0, 1);
uiController.uiScriptComplete();
})();`
}
</script>
</body>
</html>
testRunner.notifyDone();
}
</script>
</html>
@@ -85,10 +85,6 @@ fast/forms/file/open-file-panel.html [ Skip ]
# WK1 and WK2 mousemove events are subtly different in ways that break this test on WK1.
fast/events/ghostly-mousemoves-in-subframe.html [ Skip ]

# Test support for inserting special characters is not yet implemented on WK1.
fast/events/before-input-prevent-insert-replacement.html [ Skip ]
fast/events/input-event-insert-replacement.html [ Skip ]

# Media Stream API testing is not supported for WK1 yet.
fast/mediastream
http/tests/media/media-stream
@@ -110,6 +110,15 @@ window.UIHelper = class UIHelper {
});
}

static replaceTextAtRange(text, location, length) {
return new Promise(resolve => {
testRunner.runUIScript(`(() => {
uiController.replaceTextAtRange("${text}", ${location}, ${length});
uiController.uiScriptComplete('Done');
})()`, resolve);
});
}

static wait(promise)
{
testRunner.waitUntilDone();
@@ -1,3 +1,19 @@
2017-08-27 Wenson Hsieh <wenson_hsieh@apple.com>

Picking an emoji via the emoji dialog (Ctrl+Cmd+Space) fires inconsistent beforeinput events.
https://bugs.webkit.org/show_bug.cgi?id=170955
<rdar://problem/31697653>

Reviewed by Ryosuke Niwa.

Currently, we insert text with TextEventInputAutocompletion as the text event input type if any text range to
replace was specified by the platform. Instead, limit this only to when the text replacement range is not empty.
This more closely matches the intention of the spec, which states that the "insertReplacementText" inputType
should be used when "[replacing] existing text by means of a spell checker, auto-correct or similar".

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::insertTextAsync):

2017-08-27 Wenson Hsieh <wenson_hsieh@apple.com>

[iOS WK2] Web process crashes after changing selection to the end of the document when speaking a selection
@@ -4843,11 +4843,10 @@ void WebPage::insertTextAsync(const String& text, const EditingRange& replacemen

bool replacesText = false;
if (replacementEditingRange.location != notFound) {
RefPtr<Range> replacementRange = rangeFromEditingRange(frame, replacementEditingRange, static_cast<EditingRangeIsRelativeTo>(editingRangeIsRelativeTo));
if (replacementRange) {
if (auto replacementRange = rangeFromEditingRange(frame, replacementEditingRange, static_cast<EditingRangeIsRelativeTo>(editingRangeIsRelativeTo))) {
SetForScope<bool> isSelectingTextWhileInsertingAsynchronously(m_isSelectingTextWhileInsertingAsynchronously, suppressSelectionUpdate);
frame.selection().setSelection(VisibleSelection(*replacementRange, SEL_DEFAULT_AFFINITY));
replacesText = true;
replacesText = replacementEditingRange.length;
}
}

@@ -1,3 +1,17 @@
2017-08-27 Wenson Hsieh <wenson_hsieh@apple.com>

Picking an emoji via the emoji dialog (Ctrl+Cmd+Space) fires inconsistent beforeinput events.
https://bugs.webkit.org/show_bug.cgi?id=170955
<rdar://problem/31697653>

Reviewed by Ryosuke Niwa.

Tweak -insertText: to pass TextEventInputAutocompletion to Editor::insertText when inserting text, if existing
text is being replaced.

* WebView/WebHTMLView.mm:
(-[WebHTMLView insertText:]):

2017-08-24 Chris Dumez <cdumez@apple.com>

[Directory Upload] Add basic support for input.webkitdirectory
@@ -7154,11 +7154,13 @@ - (void)insertText:(id)string
_private->softSpaceRange = NSMakeRange(NSNotFound, 0);
#endif

bool replacesText = false;
if (replacementRange.location != NSNotFound) {
WebRangeIsRelativeTo rangeIsRelativeTo = needToRemoveSoftSpace ? WebRangeIsRelativeTo::Paragraph : WebRangeIsRelativeTo::EditableRoot;
RefPtr<Range> domRange = [[self _frame] _convertToDOMRange:replacementRange rangeIsRelativeTo:rangeIsRelativeTo];
if (domRange)
if (auto domRange = [[self _frame] _convertToDOMRange:replacementRange rangeIsRelativeTo:rangeIsRelativeTo]) {
coreFrame->selection().setSelection(VisibleSelection(*domRange, SEL_DEFAULT_AFFINITY));
replacesText = replacementRange.length;
}
}

bool eventHandled = false;
@@ -7171,7 +7173,7 @@ - (void)insertText:(id)string
if (!dictationAlternativeLocations.isEmpty())
eventHandled = coreFrame->editor().insertDictatedText(eventText, dictationAlternativeLocations, event);
else
eventHandled = coreFrame->editor().insertText(eventText, event);
eventHandled = coreFrame->editor().insertText(eventText, event, replacesText ? TextEventInputAutocompletion : TextEventInputKeyboard);

#if USE(INSERTION_UNDO_GROUPING)
if (registerUndoGroup)

0 comments on commit 97605ec

Please sign in to comment.