Bug 48271 – Support Appkit key bindings and custom key bindings in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

Ensure the keybindings work correctly in WebKit2.

[Enrica Casucci]

Created attachment 71808 [details]
patch

[Early Warning System Bot]

Attachment 71808 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4779010

[Alexey Proskuryakov]

Comment on attachment 71808 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71808&action=review

One thing not mentioned in the bug or ChangeLog is that there is a race condition here - the application may be in an entirely different state by the time it gets to re-process the Command-key combo. We should be on lookout for user-visible problems caused by that.

Looks good to me. Please address the comments, and Qt build problems.

> WebKit2/UIProcess/WebPageProxy.h:289
> +    // Keyboard handling
> +    void interpretKeyEvent(uint32_t, String&);

These arguments should have names. It's not at all clear what they are.

> WebKit2/UIProcess/API/mac/PageClientImpl.mm:237
> +        [m_wkView _resentEvent:nativeEvent];

The pattern is that selector name is a verb:

resent (v) feel bitter or indignant about

> WebKit2/UIProcess/API/mac/WKView.mm:90
> +    NSEvent *_resentKeyDownEvent; // We keep here the event when resending it to the application.

Maybe _keyDownEventBeingResent? With that name, the comment seems superfluous, but perhaps you could explain why it needs to be stored for future readers of the code.

> WebKit2/UIProcess/API/mac/WKView.mm:91
> +    NSString* _currentEditingCommand;

Ditto, an explanation of why we need to store this could be helpful. I'm not 100% sure if I understand why this is one command, and not an array.

> WebKit2/UIProcess/API/mac/WKView.mm:314
> +- (BOOL)_handleStyleKeyEquivalent:(NSEvent *)event

You could add a comment about why we handle italic and bold, but not underline here. See <https://bugs.webkit.org/show_bug.cgi?id=24943#c8> and later comments for some discussion, but I think that boils down to "historic reasons".

[Alexey Proskuryakov]

Comment on attachment 71808 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71808&action=review

> WebKit2/UIProcess/API/mac/WKView.mm:348
> +    // Pass command-key combos through WebCore if there is a key binding available for
> +    // this event. This lets web pages have a crack at intercepting command-modified keypresses.

Another thing that bothers me is reliance on the Command key - in comments if not in code. Not all menu commands have it - even Safari has Ctrl+Tab, for example.

[Enrica Casucci]

> One thing not mentioned in the bug or ChangeLog is that there is a race condition here - the application may be in an entirely different state by the time it gets to re-process the Command-key combo. We should be on lookout for user-visible problems caused by that.

I will add a comment in the ChangeLog and address all the other comments as well as fixing the qt build.

> Ditto, an explanation of why we need to store this could be helpful. I'm not 100% sure if I understand why this is one command, and not an array.
You are correct, this should be an array. I will change this.
> 
> > WebKit2/UIProcess/API/mac/WKView.mm:314
> > +- (BOOL)_handleStyleKeyEquivalent:(NSEvent *)event
> 
> You could add a comment about why we handle italic and bold, but not underline here. See <https://bugs.webkit.org/show_bug.cgi?id=24943#c8> and later comments for some discussion, but I think that boils down to "historic reasons".
I did not think we needed a comment since we were simply reimplementing the existing WebKit behavior, but I can definitely add one.

[Enrica Casucci]

(In reply to comment #4)
> (From update of attachment 71808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71808&action=review
> 
> > WebKit2/UIProcess/API/mac/WKView.mm:348
> > +    // Pass command-key combos through WebCore if there is a key binding available for
> > +    // this event. This lets web pages have a crack at intercepting command-modified keypresses.
> 
> Another thing that bothers me is reliance on the Command key - in comments if not in code. Not all menu commands have it - even Safari has Ctrl+Tab, for example.

I copied the comment from WebKit, but you are right, and I'll change it.

[Darin Adler]

Please don’t break the Qt build.

../../../WebKit2/UIProcess/API/qt/qwkpage.cpp:271: error: cannot allocate an object of abstract type ‘QWKPagePrivate’
../../../WebKit2/UIProcess/API/qt/qwkpage_p.h:37: note:   because the following virtual functions are pure within ‘QWKPagePrivate’:
../../../WebKit2/UIProcess/PageClient.h:62: note: 	virtual void WebKit::PageClient::interceptKeyEvent(const WebKit::NativeWebKeyboardEvent&, WTF::String&)

I guess we’ll need to implement interceptKeyEvent for Qt or figure out why the ifdefs aren’t doing the right thing.

[Csaba Osztrogonác]

(In reply to comment #7)
> I guess we’ll need to implement interceptKeyEvent for Qt or figure out why the ifdefs aren’t doing the right thing.

I cc-ed our WebKit2 guys. Could you check this Qt-WebKit2 break?

[Darin Adler]

I see now that Alexey already requested that you fix the Qt build problem, and I trust you will do so.

[Enrica Casucci]

Created attachment 71939 [details]
New patch

addressed the comments.

[Alexey Proskuryakov]

Comment on attachment 71939 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71939&action=review

r=me. We'll likely learn even more about all this while reimplementing input method support!

> WebKit2/UIProcess/API/mac/PageClientImpl.mm:237
> +        [m_wkView _setEventToResend:nativeEvent];

Can we say "_setEventBeingResent"?

> WebKit2/UIProcess/API/mac/WKView.mm:92
> +    NSEvent *_keyDownEventBeingResent; // We keep here the event when resending it to
> +                                       // the application to distinguish the case of a new event from one 
> +                                       // that has been already sent to WebCore.

We usually prefer to have a long comment before code, rather than to have to align it.

[Early Warning System Bot]

Attachment 71939 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4751028

[Enrica Casucci]

Will do. Thanks!

[Enrica Casucci]

I noticed that the qt build is still broken. I'll fix it before landing the patch.

[Enrica Casucci]

Created attachment 71945 [details]
Patch 3

Fixing the qt build (hopefully).

[Csaba Osztrogonác]

Comment on attachment 71945 [details]
Patch 3

Set r? for EWS bots

[Csaba Osztrogonác]

Comment on attachment 71945 [details]
Patch 3

Remove r?, because it can't be applied now. (WebCore/WebCore.xcodeproj/project.pbxproj)

Could you update to ToT, please?

[Andras Becsi]

Comment on attachment 71945 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=71945&action=review

> WebKit2/UIProcess/API/qt/qwkpage_p.h:59
> +    virtual void interceptKeyEvent(const WebKit::NativeWebKeyboardEvent& event, WTF:Vector<WTF:String>& commandName);

Typo. You probably wanted to use scope operators (::) for WTF.
After fixing these the patch also builds on Qt.

Thank you very much for looking after the Qt build issue.

[Enrica Casucci]

Created attachment 72109 [details]
Patch4

I found a bug in my previous patch.

[Alexey Proskuryakov]

Comment on attachment 72109 [details]
Patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=72109&action=review

> WebCore/dom/KeyboardEvent.h:36
> +        KeypressCommand() { }

I don't immediately see why a default constructor is now needed. Could you add a comment?

> WebCore/dom/KeyboardEvent.h:38
> +        KeypressCommand(const String& commandName, const String& text) : commandName(commandName), text(text) { ASSERT(commandName == "insertText:" || commandName == "insertText"); }

This is surprising and fragile. How hard would it be to ensure that only one form is seen by KeypressCommand? Unless I'm mistaken, that would be a one byte change.

> WebKit2/UIProcess/WebPageProxy.h:304
> +#if PLATFORM(MAC)
> +    // Keyboard handling
> +    void interpretKeyEvent(uint32_t eventType, Vector<WebCore::KeypressCommand>&);
> +#endif

How do we need to send the event back to application on other platforms for menu shortcut processing?

> WebKit2/UIProcess/API/mac/WKView.mm:324
> +    // This should not be changed, since it could break some Mac applications that
> +    // rely on this inherent behavior.
> +    // See https://bugs.webkit.org/show_bug.cgi?id=24943

That's a stronger comment than the one made by Darin in bug 24943. I'm not sure which is more accurate.

[Alexey Proskuryakov]

> Could you add a comment?

In ChangeLog, of course.

[Enrica Casucci]

> I don't immediately see why a default constructor is now needed. Could you add a comment?

It is needed for the generated encoder/decoder class. I'll add a comment in ChangeLog.
> > WebCore/dom/KeyboardEvent.h:38
> > +        KeypressCommand(const String& commandName, const String& text) : commandName(commandName), text(text) { ASSERT(commandName == "insertText:" || commandName == "insertText"); }
> 
> This is surprising and fragile. How hard would it be to ensure that only one form is seen by KeypressCommand? Unless I'm mistaken, that would be a one byte change.

I did it this way to avoid changing WebKit code and I don't want to pass selector strings to the webprocess. I prefer to keep it this way.

> > WebKit2/UIProcess/WebPageProxy.h:304
> > +#if PLATFORM(MAC)
> > +    // Keyboard handling
> > +    void interpretKeyEvent(uint32_t eventType, Vector<WebCore::KeypressCommand>&);
> > +#endif
> 
> How do we need to send the event back to application on other platforms for menu shortcut processing?

This patch is only about supporting Mac. Other platforms will be supported later.

> > WebKit2/UIProcess/API/mac/WKView.mm:324
> > +    // This should not be changed, since it could break some Mac applications that
> > +    // rely on this inherent behavior.
> > +    // See https://bugs.webkit.org/show_bug.cgi?id=24943
> 
> That's a stronger comment than the one made by Darin in bug 24943. I'm not sure which is more accurate.
Me neither. I'll stick with this.

[Enrica Casucci]

Committed revision 70730.

[Alexey Proskuryakov]

> I did it this way to avoid changing WebKit code and I don't want to pass selector strings to the webprocess. I prefer to keep it this way.

Another way to reduce the variance would be changing WebCore code to use command name without the semicolon.

Editor command names start with an upper case letter. This patch introduces a third way to store and pass commands ("insertText:", "InsertText", and "insertText") - I don't think we had that many before.