Bug 15057 – EditorClientGtk is missing some important keypress handling, patch attached

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

[Jasper Bryant-Greene]

EditorClientGtk is missing some important keypress handling. A patch is attached to resolve this.

Problems caused by these missing keypress cases included forms not submitting when <enter> is pressed in a text field, selection not being altered with shift-left, shift-right etc in text fields, and end/home not working properly.

This is probably not every case, but it's at least the ones that the QT code handles.

The QT code is basically identical (neither the GTK nor QT code is specific to that platform) and a way should be found to share the GTK and QT code for this, but I'm leaving that as a separate issue.

[Jasper Bryant-Greene]

Created attachment 16083 [details]
adds missing keypress cases to EditorClientGtk::handleKeypress

[Adam Roben (:aroben)]

Comment on attachment 16083 [details]
adds missing keypress cases to EditorClientGtk::handleKeypress

This patch looks fine (except for the lack of a ChangeLog entry (see <http://webkit.org/coding/contributing.html>)), but I'd much rather see us reduce code duplication rather than increase it.

(In reply to comment #0)

> The QT code is basically identical (neither the GTK nor QT code is specific to
> that platform) and a way should be found to share the GTK and QT code for this,
> but I'm leaving that as a separate issue.

I think Windows currently has the most complete code for this. We should work on moving WebView::interpretKeyEvent and WebView::handleEditingKeyboardEvent down into WebCore.

[Oliver Hunt]

Adam, that seems fairly risky to undertake, and i also don't believe it would be overly compatible with mac given the excitingly different IM model it uses. My complaint would be the dramatic formatting change :D

[Oliver Hunt]

I think in fact, the general theme of this patch is correct, but we should have another bug along the lines of "move interpretkeyevent logic into webcore" although that is basically not possible for mac, at least for now.

[Jasper Bryant-Greene]

Created attachment 16086 [details]
updated patch with more keypress cases and a ChangeLog entry

[Jasper Bryant-Greene]

Patching this has exposed another issue; please see bug #15058 also.

[Oliver Hunt]

Comment on attachment 16086 [details]
updated patch with more keypress cases and a ChangeLog entry


+                    case VK_C:
+                    case VK_X:
+                        frame->editor()->execCommand("Copy");
+                        break;

isn't ctrl-x meant to be cut?

And what about forward/back word/paragraph?

[Oliver Hunt]

Comment on attachment 16086 [details]
updated patch with more keypress cases and a ChangeLog entry

Looks fine, you might as well do the word/paragraph-fu though, and the changelog should include the files/methods that are touched.

You should be able to just run the prepare-Changelog script and it should just magically work

[Adam Roben (:aroben)]

(In reply to comment #3)
> Adam, that seems fairly risky to undertake, and i also don't believe it would
> be overly compatible with mac given the excitingly different IM model it uses.

I'm not advocating that the Mac code be altered. The Mac code is very different from the Windows, Qt, and (now) Gtk+ code, and even if it weren't this isn't a good time to be mucking around with it.

What I am saying is that, as far as I can tell, Windows, Qt, and Gtk+ all want the same implementation of EditorClient::handleKeypress. Windows has the most complete implementation (including handling many key combinations not handled currently by Qt). I think it makes far more sense for us to spend the time to make the Windows implementation sharable rather than for Gtk+ to duplicate all the Qt code (which, as I said, isn't even a complete implementation).

[Jasper Bryant-Greene]

Created attachment 16087 [details]
updated patch to implement back/forward word/paragraph and home/end while selecting, and removed duplication

As discussed on IRC, Ctrl-X is replaced with copy when the content is not editable. QT does this too.

I've updated the patch to support:
 * back/forward word/paragraph
 * shift-home/end to select to start/end of document

Also, I've removed a lot of duplication from the switch statements and made the method have just one exit point. Hope the refactoring is OK.

[Jasper Bryant-Greene]

I'm happy to take a look at the Windows code and make it shareable. I could submit a patch as an alternative to this.

[Jasper Bryant-Greene]

Created attachment 16088 [details]
boolean was not named according to code style guidelines

[Oliver Hunt]

Comment on attachment 16087 [details]
updated patch to implement back/forward word/paragraph and home/end while selecting, and removed duplication

Okay, i think i agree with Adam's subsequent comment, I think we just need to move WebView::interpretKeyEvent and WebView::handleEditingKeyboardEvent into shared code... it might even be possible to move it into WebCore...
possibly in EventHandler surrounded by a #if !PLATFORM(MAC) block

So 
EventHandler::handleEditingKeyboardEvent(KeyboardEvent*) would do the current WebView::handleEditingKeyboardEvent behaviour on all non-mac platforms.  and on mac it would call the editing client

[Adam Roben (:aroben)]

(In reply to comment #13)
> EventHandler::handleEditingKeyboardEvent(KeyboardEvent*) would do the current
> WebView::handleEditingKeyboardEvent behaviour on all non-mac platforms.  and on
> mac it would call the editing client

I think it might make more sense to put the shared code in Editor. Right now the control flow is:

EventTargetNode::defaultEventHandler
EventHandler::defaultKeyboardEventHandler
Editor::handleKeypress
EditorClient::handleKeypress
WebView::handleEditingKeyboardEvent
Editor::{insertText,execCommand}

I don't think it makes sense to bounce back to EventHandler when the EventHandler has already asked the Editor to handle the event. I think that Editor::handleKeypress should do the work that WebView::handleKeyboardEditingEvent does right now (though on Mac I suppose it should continue to just call up to the EditorClient). Perhaps we can make EditorClient::handleKeypress be a Mac-only method?

[Oliver Hunt]

Ah whoops, i thought it did come from EventHandler -- yes it should be part of Editor, not EventHandler.

I did consider saying that EditorClient::handleKeyPress should be mac only but decided that was too obvious :p

One concern is what should happen if different platforms allow different keys to be bound to those actions?

[Adam Roben (:aroben)]

(In reply to comment #15)
> I did consider saying that EditorClient::handleKeyPress should be mac only but
> decided that was too obvious :p

I love stating the obvious.

> One concern is what should happen if different platforms allow different keys
> to be bound to those actions?

If that is needed we can make a the current WebView::interpretKeyEvent be a platform-specific Editor method. For now it seems like Qt and Gtk+ want to follow the Windows conventions.

[Oliver Hunt]

(In reply to comment #16)
> (In reply to comment #15)
> > One concern is what should happen if different platforms allow different keys
> > to be bound to those actions?
> 
> If that is needed we can make a the current WebView::interpretKeyEvent be a
> platform-specific Editor method. For now it seems like Qt and Gtk+ want to
> follow the Windows conventions.
> 
True, sounds good then

[Adam Roben (:aroben)]

I think we can move the existing implementation of Editor::handleKeypress into EditorMac.mm, and then put the new implementation in Editor.cpp surrounded by #if !PLATFORM(MAC)/#endif

[Jasper Bryant-Greene]

Created attachment 16090 [details]
consolidates windows, qt and gtk keypress handling

OK, here's a patch implementing the refactoring described above. I've tested it on GTK, and it works well in my tests. Appreciate comments.

[Oliver Hunt]

Comment on attachment 16090 [details]
consolidates windows, qt and gtk keypress handling

This needs to have a correct changelog for gtk

You also need to have correct copyright info in Editor{Mac}.cpp

Add that, and i would say it's an r+

However it will at least need to be tested on Mac and Webkit/Win prior to landing.

I can do this tomorrow.

[Jasper Bryant-Greene]

Created attachment 16093 [details]
fixed changelog and copyright info

Fixed up the ChangeLog and added copyright notices.

[Adam Roben (:aroben)]

Comment on attachment 16093 [details]
fixed changelog and copyright info

+static const KeyEntry keyEntries[] = {

I think we might as well move this whole array inside interpretKeyEvent, since it's only used in there.

+const char* Editor::interpretKeyEvent(const KeyboardEvent* evt)

Since we're touching this code anyway, I think it would be good to make this a static helper function instead of a method on Editor, and perhaps give it a better name like "commandForEvent".

+#if !PLATFORM(MAC)
+    const char* interpretKeyEvent(const KeyboardEvent* evt);
     void handleKeypress(KeyboardEvent*);
+#endif

handleKeypress should not be within the #if !PLATFORM(MAC) block. handleKeypress exists on all platforms, but it has two implementations, one in Editor.cpp, and one in EditorMac.mm.

You should remove the declarations of WebView::interpretKeyEvent and WebView::handleEditingKeyboardEvent from WebView.h.

It would also be good to be slightly more explicit in your ChangeLogs by saying that the code was moved from WebView to Editor.

[Adam Roben (:aroben)]

Also, you should set the review flag to ? when uploading a patch so that people know it needs review.

[Jasper Bryant-Greene]

Created attachment 16102 [details]
updated patch based on suggestions, also changed characters to keycodes

Thanks for your suggestions. I've updated the patch based on them.

Also, I've changed the entries in keyEntries that were specified using a character to keycodes, as the character doesn't work on GTK and I don't think it would have worked on Qt either. For example 'X' has changed to VK_X.

I need verification that this specific change hasn't broken Windows keypress handling, though.

[Oliver Hunt]

Comment on attachment 16102 [details]
updated patch based on suggestions, also changed characters to keycodes

Looks good to me, just testing mac/windows builds.

Adam, you have any comments?

[Oliver Hunt]

Comment on attachment 16102 [details]
updated patch based on suggestions, also changed characters to keycodes

This causes a build failure on mac as KeyboardCodes.h does not exist on mac.  This can be fixed by an ifdef around the include

Mac also fails due due selectionForEvent not being available in EditorMac -- i suspect moving the Mac impl of Editor::handleKeypress into an ifdef'd block in Editor.cpp, or remove the static modifier in Editor.cpp

On windows KeyboardCodes.h does not exit (ifdef fix again), and WebEditorClient requires interpretKeyEvent -- which might be difficult to work around... perhaps make it accesible from outside webcore? :-/

[Jasper Bryant-Greene]

Created attachment 16103 [details]
(should have) fixed build failures on windows and mac

I've made selectionForEvent non-static and put a prototype in EditorMac.mm.

commandForKeyEvent is now a static member of Editor.

KeyboardCodes.h has been surrounded with #if !PLATFORM(MAC) && !PLATFORM(WIN) ... #endif, however this file is identical on GTK/Qt/WX and so I will submit a subsequent patch to resolve this duplication.

[Oliver Hunt]

Comment on attachment 16103 [details]
(should have) fixed build failures on windows and mac

Okay looks good, once more with the mac/windows testing though.

[Jasper Bryant-Greene]

Created attachment 16104 [details]
#include <windows.h> when building on windows, for VK_*

[Oliver Hunt]

You should set the review flag to '?'

VK_{I,B,C,X,V,...} don't exist in windows, however VK_{letter} should be equal to letter, eg VK_C == 'C', so you should be able to just use 'I', 'B', 'C', etc which should get rid of the need for KeyboardCodes.h and windows.h (which gains nothing).

You also need to modify WebEditorClient.cpp to include <WebCore/Editor.h> for windows.

[Luca Bruno]

Created attachment 17715 [details]
events for home and end keys

This patch adds more functionality to home and end keys. For example, you can select the whole contents of the page using shift+end or shift+home.

Same way works when in editable elements such as INPUT.

[Alp Toker]

Comment on attachment 17715 [details]
events for home and end keys

r=me

Spaces, not tabs in the ChangeLog next time please.

[Alp Toker]

We're probably going to end up using the GTK+ keybindings system for better integration rather than copying the Win code.

The necessary patches in this bug have been landed. Closing.