Bug 191608

Summary:

[iOS] Do not handle key events that are key commands

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

HTML Editing

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, megan_gardner, rniwa, thorton, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar, PlatformOnly

Version:

WebKit Local Build

Hardware:

iPhone / iPad

OS:

iOS 12

Bug Depends on:

Bug Blocks:

190571

Attachments:

Description	Flags
Patch and layout tests	none
Patch and layout tests	none
Patch and layout tests	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
To Land	none

Daniel Bates

Reported 2018-11-13 16:11:03 PST

Similar to the behavior on Mac, iOS WebKit needs to take to care to recognize editing key commands that may insert text from ones that do not (e.g. Command + i). For instance, in richly editing elements pressing Command + i or Command + b is expected to italicize or make bold, respectively, any newly typed text or any selected text. Performing such commands should not cause a text insertion. You can create a rich-edit element by making it contenteditable and setting the CSS property -webkit-user-modify: read-write. For example: <div contenteditable="true" style="width: 500px; height: 500px; border: 1px solid black; user-modify: read-write; -webkit-user-modify: read-write;"></div>

Attachments
Patch and layout tests (15.91 KB, patch) 2018-11-15 12:47 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Patch and layout tests (16.85 KB, patch) 2018-11-15 15:24 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Patch and layout tests (16.81 KB, patch) 2018-11-30 15:51 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.58 MB, application/zip) 2018-12-01 16:45 PST, EWS Watchlist	no flags	Details
To Land (16.67 KB, patch) 2018-12-03 12:43 PST, Daniel Bates	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-11-13 16:11:27 PST

<rdar://problem/46046013>

Daniel Bates

Comment 2 2018-11-15 12:06:44 PST

It seems sufficient to track whether or not a WebEvent is a key command and have WebKit not handle such events.

Daniel Bates

Comment 3 2018-11-15 12:47:10 PST

Created attachment 354971 [details] Patch and layout tests

EWS Watchlist

Comment 4 2018-11-15 12:48:51 PST Comment hidden (obsolete)

Attachment 354971 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/WebEvent.h:186: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:188: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:189: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:190: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:191: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:192: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:193: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:194: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:195: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:216: Should have space between @property and attributes. [whitespace/property] [4] ERROR: Source/WebCore/platform/ios/WebEvent.h:216: Should not have spaces around = in property attributes. [whitespace/property] [4] ERROR: Source/WebCore/platform/ios/WebEvent.mm:56: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/ios/WebEvent.mm:238: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:240: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:241: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:242: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:243: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:244: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:245: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:246: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:247: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:248: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 22 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Daniel Bates

Comment 5 2018-11-15 15:24:45 PST

Created attachment 354992 [details] Patch and layout tests

EWS Watchlist

Comment 6 2018-11-15 15:27:19 PST

Attachment 354992 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/WebEvent.h:186: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:188: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:189: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:190: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:191: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:192: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:193: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:194: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:195: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.h:216: Should have space between @property and attributes. [whitespace/property] [4] ERROR: Source/WebCore/platform/ios/WebEvent.h:216: Should not have spaces around = in property attributes. [whitespace/property] [4] ERROR: Source/WebCore/platform/ios/WebEvent.mm:56: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WebCore/platform/ios/WebEvent.mm:238: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:240: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:241: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:242: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:243: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:244: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:245: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:246: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:247: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/ios/WebEvent.mm:248: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 22 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 7 2018-11-15 15:37:34 PST

Comment on attachment 354992 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=354992&action=review > Source/WebCore/platform/ios/WebEvent.mm:238 > +- (WebEvent *)initWithKeyEventType:(WebEventType)type > + timeStamp:(CFTimeInterval)timeStamp Nit: I know other functions in this file does that but I don't think we're supposed to align : like this in WebKit. > LayoutTests/fast/events/ios/rich-edit-command+i-dispatches-keydown.html:30 > +var testElement = document.getElementById("test"); > +var event; > +var ignoredFirstKeyDownEvent = false; Use let? > LayoutTests/fast/events/ios/rich-edit-command+i-dispatches-keydown.html:32 > +function shouldIgnoreKeyDownEvent(e) Nit: Spell out event. > LayoutTests/fast/events/ios/rich-edit-command+i-dispatches-keydown.html:47 > + event = e; > + Why don't we just rely on window.event?? > LayoutTests/fast/events/ios/rich-edit-command+i.html:54 > + await UIHelper.keyDown("i", ["meta"]); > +} Why can't we just dump the markup here? keydown is handled synchronously by WebCore.

Daniel Bates

Comment 8 2018-11-29 10:56:19 PST

After discussing with UIKit keyboard engineers I have decided to go with a different approach for this patch to make the corresponding UIKit change simpler. The current proposed patch (attachment #354992 [details]) make UIKit responsible for telling WebKit whether a key event is a key command. Instead, it seems sufficient to use an approach and makes the UIKit logic simpler if WebKit does not handle a key event that looks like a key command and let UIKit handle the event. If it turns out that it is not a key command then UIKit will call WebKit back to insert text.

Daniel Bates

Comment 9 2018-11-30 15:22:25 PST

(In reply to Ryosuke Niwa from comment #7) > > LayoutTests/fast/events/ios/rich-edit-command+i-dispatches-keydown.html:30 > > +var testElement = document.getElementById("test"); > > +var event; > > +var ignoredFirstKeyDownEvent = false; > > Use let? > Will switch to let. > > LayoutTests/fast/events/ios/rich-edit-command+i-dispatches-keydown.html:32 > > +function shouldIgnoreKeyDownEvent(e) > > Nit: Spell out event. > > > LayoutTests/fast/events/ios/rich-edit-command+i-dispatches-keydown.html:47 > > + event = e; > > + > > Why don't we just rely on window.event?? window.event is a non-standard IE extension. Writing tests against this non-standard extensions adds another task to the person that one day looks to remove window.event support. Even if we (WebKit) never remove support for window.event it does not seem like good programming practice to use until it is standardized. Therefore, I will rename the argument to this function event and change this line to read: window.event = event; > > > LayoutTests/fast/events/ios/rich-edit-command+i.html:54 > > + await UIHelper.keyDown("i", ["meta"]); > > +} > > Why can't we just dump the markup here? keydown is handled synchronously by > WebCore. We do not update the value of the text field before the keydown listener is dispatched. You can see this by visiting <data:text/html,<input%20type="text"%20value=""%20onkeydown="document.getElementById('console').textContent=this.value"><pre%20id="console"></pre>> and typing a few keys.

Daniel Bates

Comment 10 2018-11-30 15:51:43 PST

Created attachment 356254 [details] Patch and layout tests

Ryosuke Niwa

Comment 11 2018-11-30 16:54:50 PST

Comment on attachment 356254 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=356254&action=review > LayoutTests/fast/events/ios/key-command-italic-dispatches-keydown.html:13 > + user-modify: read-write; > + -webkit-user-modify: read-write; Remove these as we discussed in person. > LayoutTests/fast/events/ios/key-command-italic-dispatches-keydown.html:46 > + window.event = event; We shouldn't have this useless code since window.event is defined here. > LayoutTests/fast/events/ios/key-command-italic.html:8 > +Markup.waitUntilDone(); It's unnecessary to have a separate script element to call waitUntilDone. Just do it in the other script element. > LayoutTests/fast/events/ios/key-command-italic.html:16 > + user-modify: read-write; > + -webkit-user-modify: read-write; Ditto. > LayoutTests/fast/events/ios/type-digits-holding-control-key.html:7 > +let charactersToType = "1234567890".split(""); Use const? > LayoutTests/fast/events/ios/type-digits-holding-control-key.html:8 > +for (let c of charactersToType) Use const?

Daniel Bates

Comment 12 2018-11-30 17:00:36 PST

(In reply to Ryosuke Niwa from comment #11) > Comment on attachment 356254 [details] > Patch and layout tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356254&action=review > > > LayoutTests/fast/events/ios/key-command-italic-dispatches-keydown.html:13 > > + user-modify: read-write; > > + -webkit-user-modify: read-write; > > Remove these as we discussed in person. > Will do. > > LayoutTests/fast/events/ios/key-command-italic-dispatches-keydown.html:46 > > + window.event = event; > > We shouldn't have this useless code since window.event is defined here. > Will remove. > > LayoutTests/fast/events/ios/key-command-italic.html:8 > > +Markup.waitUntilDone(); > > It's unnecessary to have a separate script element to call waitUntilDone. > Just do it in the other script element. > > > LayoutTests/fast/events/ios/key-command-italic.html:16 > > + user-modify: read-write; > > + -webkit-user-modify: read-write; > > Ditto. > Will remove. > > LayoutTests/fast/events/ios/type-digits-holding-control-key.html:7 > > +let charactersToType = "1234567890".split(""); > > Use const? Will change to const. > > > LayoutTests/fast/events/ios/type-digits-holding-control-key.html:8 > > +for (let c of charactersToType) > > Use const? Will change to const.

EWS Watchlist

Comment 13 2018-12-01 16:45:54 PST

Comment on attachment 356254 [details] Patch and layout tests Attachment 356254 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10233982 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall.https.html

EWS Watchlist

Comment 14 2018-12-01 16:45:56 PST

Created attachment 356326 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Daniel Bates

Comment 15 2018-12-03 12:43:04 PST

Created attachment 356394 [details] To Land

Daniel Bates

Comment 16 2018-12-03 12:44:54 PST

(In reply to Build Bot from comment #13) > Comment on attachment 356254 [details] > Patch and layout tests > > Attachment 356254 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: https://webkit-queues.webkit.org/results/10233982 > > New failing tests: > imported/w3c/web-platform-tests/webrtc/simplecall.https.html I am unclear how the proposed change could cause this failure.

Daniel Bates

Comment 17 2018-12-03 12:45:44 PST

Comment on attachment 356394 [details] To Land Clearing flags on attachment: 356394 Committed r238814: <https://trac.webkit.org/changeset/238814>

Daniel Bates

Comment 18 2018-12-03 12:45:46 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.