Bug 24943 – Command-B and Command-I do not generate keydown events in contentEditable regions

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

[Robby Walker]

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.

[Robby Walker]

Created attachment 29085 [details]
Demonstration page showing missing events

[Eric Seidel (no email)]

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.

[Julie Parent]

We do actually want to do the execCommand('bold') ourselves, but we need other properties of contentEditable, like enter behavior, rich text paste, etc.

[Mark Tsui]

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.

[Tony Chang]

I see the problem.  Investing a fix (which I think is to move some code out of WebKit and into WebCore).

[Tony Chang]

Created attachment 50472 [details]
test with optional preventDefault

[Tony Chang]

Created attachment 50473 [details]
Patch

[Tony Chang]

(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 .

[Darin Adler]

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.

[Alexey Proskuryakov]

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?

[Tony Chang]

(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).

[Mark Tsui]

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.

[Tony Chang]

Created attachment 50573 [details]
Patch

[Tony Chang]

(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.

[Alexey Proskuryakov]

We may be breaking other Mac applications that use WebKit if changes are required for Safari.

[Tony Chang]

(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.

[Tony Chang]

Created attachment 50765 [details]
Patch

[Tony Chang]

(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).

[Adam Barth]

@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?

[Alexey Proskuryakov]

I'll review this.

[Darin Adler]

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?

[Darin Adler]

Created attachment 56006 [details]
fragment of a patch demonstrating my suggested fix

[Alexey Proskuryakov]

<rdar://problem/7121644> is the same issue, but for <input type=text>. This patch should fix both.

[Tony Chang]

Created attachment 56216 [details]
Patch

[Tony Chang]

(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.

[Darin Adler]

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.

[Darin Adler]

(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.

[Tony Chang]

Created attachment 56333 [details]
Patch

[Alexey Proskuryakov]

+        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?

[Darin Adler]

(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.

[Tony Chang]

Committed r59722: <http://trac.webkit.org/changeset/59722>

[Alexey Proskuryakov]

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.