Bug 191608 - [iOS] Do not handle key events that are key commands
Summary: [iOS] Do not handle key events that are key commands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks: 190571
  Show dependency tree / graph
 
Reported: 2018-11-13 16:11 PST by Daniel Bates
Modified: 2018-12-03 12:45 PST (History)
6 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 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>
Comment 1 Radar WebKit Bug Importer 2018-11-13 16:11:27 PST 
<rdar://problem/46046013>
Comment 2 Daniel Bates 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.
Comment 3 Daniel Bates 2018-11-15 12:47:10 PST 
Created attachment 354971 [details]
Patch and layout tests
Comment 4 EWS Watchlist 2018-11-15 12:48:51 PST Comment hidden (obsolete)
Comment 5 Daniel Bates 2018-11-15 15:24:45 PST 
Created attachment 354992 [details]
Patch and layout tests
Comment 6 EWS Watchlist 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.
Comment 7 Ryosuke Niwa 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.
Comment 8 Daniel Bates 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.
Comment 9 Daniel Bates 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.
Comment 10 Daniel Bates 2018-11-30 15:51:43 PST 
Created attachment 356254 [details]
Patch and layout tests
Comment 11 Ryosuke Niwa 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?
Comment 12 Daniel Bates 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.
Comment 13 EWS Watchlist 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
Comment 14 EWS Watchlist 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
Comment 15 Daniel Bates 2018-12-03 12:43:04 PST 
Created attachment 356394 [details]
To Land
Comment 16 Daniel Bates 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.
Comment 17 Daniel Bates 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>
Comment 18 Daniel Bates 2018-12-03 12:45:46 PST 
All reviewed patches have been landed.  Closing bug.