Bug 15057 - EditorClientGtk is missing some important keypress handling, patch attached
Summary: EditorClientGtk is missing some important keypress handling, patch attached
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-08-22 17:57 PDT by Jasper Bryant-Greene
Modified: 2007-12-05 07:40 PST (History)
2 users (show)

See Also:


Attachments
adds missing keypress cases to EditorClientGtk::handleKeypress (4.15 KB, patch)
2007-08-22 17:59 PDT, Jasper Bryant-Greene
aroben: review-
Details | Formatted Diff | Diff
updated patch with more keypress cases and a ChangeLog entry (6.92 KB, patch)
2007-08-22 20:43 PDT, Jasper Bryant-Greene
oliver: review-
Details | Formatted Diff | Diff
updated patch to implement back/forward word/paragraph and home/end while selecting, and removed duplication (7.99 KB, patch)
2007-08-22 21:25 PDT, Jasper Bryant-Greene
oliver: review-
Details | Formatted Diff | Diff
boolean was not named according to code style guidelines (8.00 KB, patch)
2007-08-22 21:31 PDT, Jasper Bryant-Greene
no flags Details | Formatted Diff | Diff
consolidates windows, qt and gtk keypress handling (26.50 KB, patch)
2007-08-23 01:21 PDT, Jasper Bryant-Greene
oliver: review-
Details | Formatted Diff | Diff
fixed changelog and copyright info (26.44 KB, patch)
2007-08-23 01:57 PDT, Jasper Bryant-Greene
aroben: review-
Details | Formatted Diff | Diff
updated patch based on suggestions, also changed characters to keycodes (27.27 KB, patch)
2007-08-23 16:02 PDT, Jasper Bryant-Greene
oliver: review-
Details | Formatted Diff | Diff
(should have) fixed build failures on windows and mac (29.64 KB, patch)
2007-08-23 18:06 PDT, Jasper Bryant-Greene
no flags Details | Formatted Diff | Diff
#include <windows.h> when building on windows, for VK_* (29.74 KB, patch)
2007-08-23 18:45 PDT, Jasper Bryant-Greene
m: review-
Details | Formatted Diff | Diff
events for home and end keys (2.98 KB, patch)
2007-12-05 01:35 PST, Luca Bruno
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jasper Bryant-Greene 2007-08-22 17:57:07 PDT
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.
Comment 1 Jasper Bryant-Greene 2007-08-22 17:59:15 PDT
Created attachment 16083 [details]
adds missing keypress cases to EditorClientGtk::handleKeypress
Comment 2 Adam Roben (:aroben) 2007-08-22 18:41:49 PDT
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.
Comment 3 Oliver Hunt 2007-08-22 19:10:52 PDT
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
Comment 4 Oliver Hunt 2007-08-22 19:12:02 PDT
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.
Comment 5 Jasper Bryant-Greene 2007-08-22 20:43:15 PDT
Created attachment 16086 [details]
updated patch with more keypress cases and a ChangeLog entry
Comment 6 Jasper Bryant-Greene 2007-08-22 20:45:19 PDT
Patching this has exposed another issue; please see bug #15058 also.
Comment 7 Oliver Hunt 2007-08-22 20:54:27 PDT
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?
Comment 8 Oliver Hunt 2007-08-22 21:01:03 PDT
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
Comment 9 Adam Roben (:aroben) 2007-08-22 21:16:19 PDT
(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).
Comment 10 Jasper Bryant-Greene 2007-08-22 21:25:17 PDT
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.
Comment 11 Jasper Bryant-Greene 2007-08-22 21:27:59 PDT
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.
Comment 12 Jasper Bryant-Greene 2007-08-22 21:31:30 PDT
Created attachment 16088 [details]
boolean was not named according to code style guidelines
Comment 13 Oliver Hunt 2007-08-22 21:39:39 PDT
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
Comment 14 Adam Roben (:aroben) 2007-08-22 21:46:58 PDT
(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?
Comment 15 Oliver Hunt 2007-08-22 21:51:15 PDT
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?
Comment 16 Adam Roben (:aroben) 2007-08-22 21:52:58 PDT
(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.
Comment 17 Oliver Hunt 2007-08-22 21:58:31 PDT
(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
Comment 18 Adam Roben (:aroben) 2007-08-22 22:32:05 PDT
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
Comment 19 Jasper Bryant-Greene 2007-08-23 01:21:59 PDT
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.
Comment 20 Oliver Hunt 2007-08-23 01:30:18 PDT
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.
Comment 21 Jasper Bryant-Greene 2007-08-23 01:57:58 PDT
Created attachment 16093 [details]
fixed changelog and copyright info

Fixed up the ChangeLog and added copyright notices.
Comment 22 Adam Roben (:aroben) 2007-08-23 09:27:39 PDT
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.
Comment 23 Adam Roben (:aroben) 2007-08-23 09:28:17 PDT
Also, you should set the review flag to ? when uploading a patch so that people know it needs review.
Comment 24 Jasper Bryant-Greene 2007-08-23 16:02:41 PDT
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.
Comment 25 Oliver Hunt 2007-08-23 16:08:03 PDT
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?
Comment 26 Oliver Hunt 2007-08-23 16:31:54 PDT
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? :-/
Comment 27 Jasper Bryant-Greene 2007-08-23 18:06:23 PDT
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.
Comment 28 Oliver Hunt 2007-08-23 18:09:55 PDT
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.
Comment 29 Jasper Bryant-Greene 2007-08-23 18:45:51 PDT
Created attachment 16104 [details]
#include <windows.h> when building on windows, for VK_*
Comment 30 Oliver Hunt 2007-08-23 20:56:30 PDT
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.
Comment 31 Luca Bruno 2007-12-05 01:35:48 PST
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.
Comment 32 Alp Toker 2007-12-05 07:15:42 PST
Comment on attachment 17715 [details]
events for home and end keys

r=me

Spaces, not tabs in the ChangeLog next time please.
Comment 33 Alp Toker 2007-12-05 07:40:03 PST
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.