Bug 48271 - Support Appkit key bindings and custom key bindings in WebKit2
Summary: Support Appkit key bindings and custom key bindings in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-10-25 15:31 PDT by Enrica Casucci
Modified: 2010-10-27 17:10 PDT (History)
7 users (show)

See Also:


Attachments
patch (25.89 KB, patch)
2010-10-25 15:51 PDT, Enrica Casucci
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
New patch (27.42 KB, patch)
2010-10-26 14:32 PDT, Enrica Casucci
ap: review+
Details | Formatted Diff | Diff
Patch 3 (28.66 KB, patch)
2010-10-26 15:09 PDT, Enrica Casucci
no flags Details | Formatted Diff | Diff
Patch4 (31.24 KB, patch)
2010-10-27 16:29 PDT, Enrica Casucci
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2010-10-25 15:31:19 PDT
Ensure the keybindings work correctly in WebKit2.
Comment 1 Enrica Casucci 2010-10-25 15:51:52 PDT
Created attachment 71808 [details]
patch
Comment 2 Early Warning System Bot 2010-10-25 17:36:45 PDT
Attachment 71808 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4779010
Comment 3 Alexey Proskuryakov 2010-10-26 10:42:36 PDT
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".
Comment 4 Alexey Proskuryakov 2010-10-26 10:46:14 PDT
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.
Comment 5 Enrica Casucci 2010-10-26 11:29:45 PDT
> 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.
Comment 6 Enrica Casucci 2010-10-26 11:30:56 PDT
(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.
Comment 7 Darin Adler 2010-10-26 11:44:06 PDT
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.
Comment 8 Csaba Osztrogonác 2010-10-26 12:04:04 PDT
(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?
Comment 9 Darin Adler 2010-10-26 12:05:43 PDT
I see now that Alexey already requested that you fix the Qt build problem, and I trust you will do so.
Comment 10 Enrica Casucci 2010-10-26 14:32:25 PDT
Created attachment 71939 [details]
New patch

addressed the comments.
Comment 11 Alexey Proskuryakov 2010-10-26 14:41:19 PDT
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.
Comment 12 Early Warning System Bot 2010-10-26 14:42:00 PDT
Attachment 71939 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4751028
Comment 13 Enrica Casucci 2010-10-26 14:43:46 PDT
Will do. Thanks!
Comment 14 Enrica Casucci 2010-10-26 14:45:05 PDT
I noticed that the qt build is still broken. I'll fix it before landing the patch.
Comment 15 Enrica Casucci 2010-10-26 15:09:59 PDT
Created attachment 71945 [details]
Patch 3

Fixing the qt build (hopefully).
Comment 16 Csaba Osztrogonác 2010-10-26 23:43:25 PDT
Comment on attachment 71945 [details]
Patch 3

Set r? for EWS bots
Comment 17 Csaba Osztrogonác 2010-10-26 23:55:15 PDT
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?
Comment 18 Andras Becsi 2010-10-27 04:56:36 PDT
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.
Comment 19 Enrica Casucci 2010-10-27 16:29:38 PDT
Created attachment 72109 [details]
Patch4

I found a bug in my previous patch.
Comment 20 Alexey Proskuryakov 2010-10-27 16:52:33 PDT
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.
Comment 21 Alexey Proskuryakov 2010-10-27 16:53:38 PDT
> Could you add a comment?

In ChangeLog, of course.
Comment 22 Enrica Casucci 2010-10-27 16:59:21 PDT
> 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.
Comment 23 Enrica Casucci 2010-10-27 17:05:37 PDT
Committed revision 70730.
Comment 24 Alexey Proskuryakov 2010-10-27 17:10:15 PDT
> 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.