Bug 14140 - REGRESSION: Complex system KeyBindings don't work properly
Summary: REGRESSION: Complex system KeyBindings don't work properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac (Intel) OS X 10.4
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-06-14 08:48 PDT by Michail Pishchagin
Modified: 2007-12-16 09:40 PST (History)
3 users (show)

See Also:


Attachments
~/Library/KeyBindings/DefaultKeyBinding.dict (2.93 KB, text/plain)
2007-06-14 12:49 PDT, Michail Pishchagin
no flags Details
proposed fix (9.37 KB, patch)
2007-12-16 07:48 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michail Pishchagin 2007-06-14 08:48:52 PDT
$ cat ~/Library/KeyBindings/DefaultKeyBinding.dict
{
        "^m" = {
                "^s"    = ("insertText:", "\U21E7");  /* C-s     shift */
        };

        "~+" = ( "cut:", "insertText:", "«", "paste:", "insertText:", "»", "moveBackward:" );

        /* HTML/XML opening and closing tag pair. */
        "^<" = ( "setMark:",
                 "moveWordBackward:",
                 "deleteToMark:",
                 "insertText:", "<",
                 "yank:",
                 "insertText:", ">",
                 "setMark:",
                 "insertText:", "</",
                 "yank:",
                 "insertText:", ">",
                 "swapWithMark:" );
}

When entering text in text areas, these commands don't work properly in Safari 3 anymore. In Safari 2 I used to type "a", then press 

&#8963;&#8679;<, and "a" got automatically converted to "<a></a>" with cursor flashing in the middle. Now it's no longer the case. Also "&#8997;+" doesn't result in "«»" appearing.




Also "&#8963;M &#8963;S" doesn't insert "&#8679;" anymore, seems that nested bindings are no longer handled at all.
Comment 1 Michail Pishchagin 2007-06-14 08:51:36 PDT
Shortcuts haven't displayed as intended, I've used mnemonics to describe them instead:

These commands don't work properly in Safari 3 anymore. In Safari 2 I used to type "a", then press "Ctrl-Shift-<", and "a" got automatically converted to "<a></a>" with cursor flashing in the middle. Now it's no longer the case. Also Opt-+ doesn't result in "«»" appearing.

Also "Ctrl-M Ctrl-S" doesn't insert shift key glyph anymore, seems that nested bindings are no longer handled at all.

Comment 2 Mark Rowe (bdash) 2007-06-14 12:04:52 PDT
Can you attach your entire DefaultKeyBinding.dict file to this bug to make testing easier?  This is almost certainly fallout from the move to engine-rendered form controls from NSViews. I think you'll find that your custom key bindings are similarly broken in Safari 2.0 when editing an "editable region" rather than a form control.
Comment 3 Michail Pishchagin 2007-06-14 12:49:10 PDT
Created attachment 15028 [details]
~/Library/KeyBindings/DefaultKeyBinding.dict
Comment 4 Mark Rowe (bdash) 2007-06-14 14:15:58 PDT
I can confirm that with your key bindings file C-m C-s does not insert the Shift glyph.  When I type "a", then press C-S-<, I see the following in my text field:
a><</>

This is definitely less than desirable.  I was testing with a recent WebKit build on Leopard.
Comment 5 Mark Rowe (bdash) 2007-06-14 14:19:38 PDT
<rdar://problem/5270958>
Comment 6 Alexey Proskuryakov 2007-06-18 22:06:02 PDT
Regression -> P1
Comment 7 Alexey Proskuryakov 2007-12-16 07:48:41 PST
Created attachment 17932 [details]
proposed fix

That's a nice feature, I didn't even know about it!

Unfortunately, there doesn't seem to be a way to write a layout test - I couldn't find any API to install a per-application key binding dictionary. Xcode seems to do that somehow, presumably with an SPI.
Comment 8 Darin Adler 2007-12-16 09:18:28 PST
Comment on attachment 17932 [details]
proposed fix

+        Vector<KeypressCommand>& keypressCommands() { return m_keypressCommands; }

Even in a case like this, I usually prefer to provide a get/set pair rather than a single accessor. This allows us to change the set operation to do additional work without changing all callers, for example.

Change looks good, r=me
Comment 9 Alexey Proskuryakov 2007-12-16 09:40:29 PST
Committed revision 28772.

I haven't taken the suggestion to use a get/set pair: there aren't a lot of callers in this case, so it would be easy to modify this when needed. Also, I'm not sure if KeyboardEvent is even the right object to store this information.