Bug 191608 – [iOS] Do not handle key events that are key commands

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Daniel Bates]

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>

[Radar WebKit Bug Importer]

<rdar://problem/46046013>

[Daniel Bates]

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

[Daniel Bates]

Created attachment 354971 [details]
Patch and layout tests

[EWS Watchlist]

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]

Created attachment 354992 [details]
Patch and layout tests

[EWS Watchlist]

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 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]

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]

(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]

Created attachment 356254 [details]
Patch and layout tests

[Ryosuke Niwa]

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]

(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 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]

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]

Created attachment 356394 [details]
To Land

[Daniel Bates]

(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 on attachment 356394 [details]
To Land

Clearing flags on attachment: 356394

Committed r238814: <https://trac.webkit.org/changeset/238814>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.