Bug 24943

Summary: Command-B and Command-I do not generate keydown events in contentEditable regions
Product: WebKit Reporter: Robby Walker <robbyw>
Component: HTML EditingAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, darin, enrica, eric, jparent, justin.garcia, mark.tsui.01, ojan, rniwa, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Demonstration page showing missing events
none
test with optional preventDefault
none
Patch
none
Patch
none
Patch
none
fragment of a patch demonstrating my suggested fix
none
Patch
none
Patch darin: review+

Description Robby Walker 2009-03-30 14:24:08 PDT
Mac only.  See the attached file.  Pressing Command-B or Command-I while in a rich text area does not generate a keydown, keypress, or keyup event.  Command-U gives keydown and keypress, but not keyup.
Comment 1 Robby Walker 2009-03-30 14:24:40 PDT
Created attachment 29085 [details]
Demonstration page showing missing events
Comment 2 Eric Seidel (no email) 2009-03-30 15:09:06 PDT
I've been told a workaround can be to set your region to read-write-plaintext-only to catch these events.  But then you'd have to do the execCommand("bold") yourself, or do your own bold tag wrapping.
Comment 3 Julie Parent 2009-03-30 15:13:19 PDT
We do actually want to do the execCommand('bold') ourselves, but we need other properties of contentEditable, like enter behavior, rich text paste, etc.
Comment 4 Mark Tsui 2009-12-10 20:13:34 PST
What is the status of this bug? Key events for Command-B/I are necessary for rich text editors that would like to implement their own formatting behaviour.
Comment 5 Tony Chang 2010-03-10 21:20:37 PST
I see the problem.  Investing a fix (which I think is to move some code out of WebKit and into WebCore).
Comment 6 Tony Chang 2010-03-10 22:42:59 PST
Created attachment 50472 [details]
test with optional preventDefault
Comment 7 Tony Chang 2010-03-10 22:48:46 PST
Created attachment 50473 [details]
Patch
Comment 8 Tony Chang 2010-03-10 22:57:40 PST
(In reply to comment #7)
> Created an attachment (id=50473) [details]
> Patch

Leaving out cmd+u/ctrl+u on mac/win seems like an oversight, so I added it.

I also moved when the handling happens in mac, which fixes the bug where a keypress event isn't fired.  This also allows preventDefault in the keypress event from blocking the bold/italic/underline.

This is a Safari mac specific fix.  As far as I can tell, Chromium Mac doesn't fire keypress events for any cmd key combinations.  That's a separate bug.  Safari win also doesn't fire keypress for ctrl+b or ctrl+u, but that also seems like a separate fix.

Having said all that, I think this change is a hack.  It would be better to get rid of _handleStyleKeyEquivalent and add WEBCORE_COMMANDs for bold and italic and have Safari handle this directly (kind of like how it handles cmd+a and other keyboard events).  I tried to make this change, but I can't because the code is in Safari.  If we did this, we should get rid of WebKitRespectStandardStyleKeyEquivalentsPreferenceKey since it won't be needed.

All this was originally added in http://trac.webkit.org/changeset/7697 .
Comment 9 Darin Adler 2010-03-11 11:24:07 PST
Comment on attachment 50473 [details]
Patch

Thanks so much for tackling this.

> +        No test because eventSender.keyDown sends key events directly to
> +        webcore so we can't test key handling in WebKit/mac.

That is not true. EventSendingController sends a keyDown like this:

    [[[[mainFrame webView] window] firstResponder] keyDown:event];

Did you try making a test? What happened?

Maybe the real issue is that DumpRenderTree doesn't call performKeyEquivalent. We could easily fix DumpRenderTree so it does.
Comment 10 Alexey Proskuryakov 2010-03-11 16:44:04 PST
I don't think it's correct to expect keypress for Cmd+B. Keydown should work, but not keypress, because these come with a character code.

Does IE dispatch keypress for Ctrl+I?
Comment 11 Tony Chang 2010-03-11 17:46:10 PST
(In reply to comment #10)
> I don't think it's correct to expect keypress for Cmd+B. Keydown should work,
> but not keypress, because these come with a character code.
> 
> Does IE dispatch keypress for Ctrl+I?

You're right, there should be no keypress event (IE doens't dispatch a keypress for ctrl+anything).  I think this means that this is the wrong place for WebKit mac to be handling cmd+b and cmd+i (there's no way to allow keydown and not allow keypress at this point).

As I mentioned before, the right fix is to probably just remove this code and add WEBCORE_COMMAND(bold) and WEBCORE_COMMAND(italic).  I think Safari can then handle the cmd+b and cmd+i key presses directly (probably wherever the code is for handling cmd+a).

(In reply to comment #9)
> Did you try making a test? What happened?

Yes, I made a test and none of the formatting got applied when using
  eventSender.keyDown("b", ["metaKey"]);

cmd+a didn't seem to work either.

> Maybe the real issue is that DumpRenderTree doesn't call performKeyEquivalent.
> We could easily fix DumpRenderTree so it does.

Sorry, my Objective C fu is weak.  I'm not exactly sure how to call this or what the expected code path is (performKeyEquivalent appears to be called directly from Safari).
Comment 12 Mark Tsui 2010-03-11 19:11:38 PST
Thanks Tony for looking at this.

I agree that the existing behaviour for cmd+u/a feels correct and that cmd+b/i should be handled with the same code.
Comment 13 Tony Chang 2010-03-11 19:15:49 PST
Created attachment 50573 [details]
Patch
Comment 14 Tony Chang 2010-03-11 19:17:07 PST
(In reply to comment #13)
> Created an attachment (id=50573) [details]
> Patch

I think this is the right direction, but the patch as is will just cause cmd+b and cmd+i to do nothing.  I think the rest of the fix would be in Safari code.
Comment 15 Alexey Proskuryakov 2010-03-11 20:01:58 PST
We may be breaking other Mac applications that use WebKit if changes are required for Safari.
Comment 16 Tony Chang 2010-03-11 20:28:21 PST
(In reply to comment #15)
> We may be breaking other Mac applications that use WebKit if changes are
> required for Safari.

That's true, you would lose cmd+b and cmd+i formatting in rich text areas.

It seems like it would be a good thing to consolidate editor key handling in one place like it is in WebKit/win.  To do that, we either need to move cmd+b and cmd+i handling into Safari or we need to move cmd+a handling out (it looks like when I press cmd+a, Safari calls WebHTMLView::selectAll directly).

Either of these options would break other Mac apps that use WebKit (do they all really have to implement cmd+a?), but it seems like the best way forward.  Perhaps moving cmd+a handling into WebKit would be a safer change because it would just be adding an extra fallback after the application passed the key event to WebKit.
Comment 17 Tony Chang 2010-03-15 23:33:35 PDT
Created attachment 50765 [details]
Patch
Comment 18 Tony Chang 2010-03-15 23:38:26 PDT
(In reply to comment #17)
> Created an attachment (id=50765) [details]
> Patch

Ok, I think I found a place in the code that will allow keydown to fire and if it's not handled, then we can apply the formatting.  I also found out why eventSender wasn't working and added a layout test.

This patch fires keypress for cmd+anything, but that doesn't seem to be a regression.  In ToT webkit, I see keypress fired for cmd+anything except b and i (try the one of the test case attachments on this bug).
Comment 19 Adam Barth 2010-05-13 00:14:38 PDT
@enrica or @ojan: This editing bug has been up for review for two months.  The patch is tiny and has a test.  Would either of you be willing to review it?
Comment 20 Alexey Proskuryakov 2010-05-13 00:38:15 PDT
I'll review this.
Comment 21 Darin Adler 2010-05-13 12:04:28 PDT
Comment on attachment 50765 [details]
Patch

Seems fine to change behavior.

But I don't think the specifics of the code change here is right. All we need to do is unify the style keys with other key equivalents such as ones in the menus. I have a patch that shows how to do this that I will attach now.

The Command-U change should not be bundled in with the part of this change that is mentioned in the bug title. That change needs to be judged on its own merits, and could change the behavior of existing Mac OS X applications.

WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm:430
 +      [preferences setRespectStandardStyleKeyEquivalents:YES];
Why is this change needed? Isn't this preference on by default?
Comment 22 Darin Adler 2010-05-13 12:04:55 PDT
Created attachment 56006 [details]
fragment of a patch demonstrating my suggested fix
Comment 23 Alexey Proskuryakov 2010-05-13 12:08:15 PDT
<rdar://problem/7121644> is the same issue, but for <input type=text>. This patch should fix both.
Comment 24 Tony Chang 2010-05-17 00:42:03 PDT
Created attachment 56216 [details]
Patch
Comment 25 Tony Chang 2010-05-17 00:45:57 PDT
(In reply to comment #21)
> (From update of attachment 50765 [details])
> But I don't think the specifics of the code change here is right. All we need to do is unify the style keys with other key equivalents such as ones in the menus. I have a patch that shows how to do this that I will attach now.

Huh, I thought I had tried this before, but clearly I'm mistaken.  This seems to work fine.

> The Command-U change should not be bundled in with the part of this change that is mentioned in the bug title. That change needs to be judged on its own merits, and could change the behavior of existing Mac OS X applications.

Removed.

> WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm:430
>  +      [preferences setRespectStandardStyleKeyEquivalents:YES];
> Why is this change needed? Isn't this preference on by default?

It's NO by default:
http://trac.webkit.org/browser/trunk/WebKit/mac/WebView/WebPreferences.mm#L331

The newest patch doesn't have a test because DRT never calls performKeyEquivalent.  This method appears to be called directly by Safari.  I tried putting a printf in the function and none of the tests failed.
Comment 26 Darin Adler 2010-05-17 09:48:55 PDT
Comment on attachment 56216 [details]
Patch

Good to see my code change in a patch for review.

> +        No tests because performKeyEquivalent does not seem to be called by DRT.

That's a reason we can't make an automated test, but a manual test should be possible. See check-ins like <http://trac.webkit.org/changeset/58829> for examples of how to add a manual test.
Comment 27 Darin Adler 2010-05-17 09:50:52 PDT
(In reply to comment #25)
> The newest patch doesn't have a test because DRT never calls performKeyEquivalent.  This method appears to be called directly by Safari.

The method is not called directly by Safari. It's called by -[NSApplication sendEvent:] if you send that method an appropriate event.
Comment 28 Tony Chang 2010-05-18 01:12:41 PDT
Created attachment 56333 [details]
Patch
Comment 29 Alexey Proskuryakov 2010-05-18 09:35:00 PDT
+        ret = [self _handleStyleKeyEquivalent:event] || [super performKeyEquivalent:event];

Would we want to give the application a chance to handle key equivalents before trying hardcoded ones in WebKit?
Comment 30 Darin Adler 2010-05-18 09:39:34 PDT
(In reply to comment #29)
> +        ret = [self _handleStyleKeyEquivalent:event] || [super performKeyEquivalent:event];
> 
> Would we want to give the application a chance to handle key equivalents before trying hardcoded ones in WebKit?

Maybe. But that would be another behavior change, aside from the "give the web page first crack at it", so I think that should be considered separately on its own merits.
Comment 31 Tony Chang 2010-05-18 17:37:04 PDT
Committed r59722: <http://trac.webkit.org/changeset/59722>
Comment 32 Alexey Proskuryakov 2011-02-25 21:13:24 PST
The bug title only mentioned keydown, while description mentioned both keydown and keyup.

Unfortunately, keyup still doesn't work. I've filed bug 55291 for that.