Bug 40529 – eventSender.keyDown("delete") incorrectly sends a backspace on some platforms

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Robert Hogan]

[Qt] Use backspace for eventSender.keyDown("delete")

[Robert Hogan]

Created attachment 58561 [details]
Patch

[Robert Hogan]

Created attachment 58562 [details]
Patch

[Robert Hogan]

To tell the truth I think using "delete" to refer to backspace in eventsender is a bit confusing. I was going to add a "backspace" event but noticed that Mac actually attaches the delete unicode value to "delete" while everyone else uses backspace.

[Eric Seidel (no email)]

Comment on attachment 58562 [details]
Patch

Yay for fixing layout tests!  Are we certain this is the right fix and that it doesn't needs some different fix in WebCore?

[Eric Seidel (no email)]

CCing editing peeps just so they see this go by.

[Alexey Proskuryakov]

Comment on attachment 58562 [details]
Patch

This seems wrong - Mac EventSender sends backspace for "\x8" and forward delete for "delete":

    } else if ([character isEqualToString:@"delete"]) {
        const unichar ch = 0x7f;

[Alexey Proskuryakov]

I just noticed that you said the same in comment 3, but then I'm not sure why you poposed this fix.

[Robert Hogan]

(In reply to comment #7)
> I just noticed that you said the same in comment 3, but then I'm not sure why you poposed this fix.

For the fairly blinkered reason that it makes keyDown("delete") work for all tests that use it on Qt, and otherwise everyone but Mac translates keyDown("delete") as backspace.

win:

    else if (JSStringIsEqualToUTF8CString(character, "delete"))
        virtualKeyCode = VK_BACK;

chromium:

    else if ("delete" == codeStr)
        code = base::VKEY_BACK;

and so on.

The Mac keycodes listed at: http://forums.macrumors.com/showthread.php?t=780577 don't include backspace, but it has forwardDelete: 'kVK_ForwardDelete             = 0x75'.

This link ( http://www.davidalison.com/2008/03/mac-where-did-my-backspace-key-go.html ) suggests that Mac does something special with backspace/delete so maybe that's the source of the difference?

[Alexey Proskuryakov]

Mac virtual key code for backspace in 0x33. This key is marked "delete" on U.S. Mac keyboards, and the other one is marked with an icon, and called "forward delete", which is a source of some confusion. There is also a "clear" key, which is located where num lock usually is on PC keyboards.

See e.g. <http://boredzo.org/blog/wp-content/uploads/2007/05/IMTx-virtual-keycodes.pdf> for a layout of an (old) Mac keyboard.

The immediate reason for the difference in layout test behavior is that on Mac, character code 0x7f is later turned into 0x08 in PlatformKeyboardEvent:

    // Turn 0x7F into 8, because backspace needs to always be 8.
    if (m_text == "\x7F")
        m_text = "\x8";
    if (m_unmodifiedText == "\x7F")
        m_unmodifiedText = "\x8";

Mac character code for forward delete is 0xF728 (NSDeleteFunctionKey), not 0x7f.

I'm not yet sure what's going on, will need to dig up the history of this code. But we probably don't want eventSender.keyDown("delete") and eventSender.keyDown("\x8") to do the same thing.

[Alexey Proskuryakov]

Created attachment 58682 [details]
proposed fix

[Robert Hogan]

(In reply to comment #10)
> Created an attachment (id=58682) [details]
> proposed fix

why not add keyDown("backspace")? I think you'd have to know your keycodes to realize \x8 is backspace without a bit of digging.

[Darin Adler]

Comment on attachment 58682 [details]
proposed fix

> +        * platform/mac/KeyEventMac.mm: (WebCore::PlatformKeyboardEvent::PlatformKeyboardEvent):
> +        Use virtual key code to force correct character code for clarity. Also, reworded comment,
> +        since saying that "backspace needs to always be 8" misleadingly implied that it could
> +        "sometimes" be such without this code.

I don’t think that the use of 0x33 here improves clarity. Where does that 0x33 value come from? AppKit has constants named NSDeleteCharacter and NSDeleteFunctionKey, but I don’t see any constant for the 0x33 value. At least within this source file, I’d like to see a named constant used for the 0x33 value. Or maybe the comment can say what the number means.

Do we have a guarantee that this new code behaves the same in all cases as the old code? Even in applications that construct their own NSEvent objects?

>      // The adjustments below are only needed in backward compatibility mode, but we cannot tell what mode we are in from here.

Is this comment still correct? What is "backward compatibility mode"?

> +        * DumpRenderTree/mac/EventSendingController.mm:
> +        (-[EventSendingController keyDown:withModifiers:withLocation:]): We were sending a broken
> +        event for "delete" - it had virtual key code from forward delete, and text from backspace.
> +        Fixed "delete" to mean forward delete.

I don't think that was our intention when making the word "delete" be one of the values taken by the keyDown function in eventSender.

Both keys are labeled "delete" on my keyboard, and the Unicode character for U+007F is named DELETE. Because of all these overlapping uses of the word, I think it’s confusing to use "delete" to mean the forward delete button. Maybe we should change eventSender.keyDown to work with the names "backspace" and "forward delete" and eliminate the "delete" name entirely?

r=me on this change as is, since it at least makes things consistent, but please consider my comments anyway

[Alexey Proskuryakov]

> I don’t think that the use of 0x33 here improves clarity. 
> Where does that 0x33 value come from? 
> AppKit has constants named NSDeleteCharacter and NSDeleteFunctionKey, 
> but I don’t see any constant for the 0x33 value.

Virtual key codes are only documented in Inside Mac, and a few have constants in HIToolBox/Events.h header.

I've changed this to check Windows virtual key code, as other code in this function does, and used constants from WindowsKeyboardCodes.h.

> Do we have a guarantee that this new code behaves the same in all cases as the old code? Even in applications that construct their own NSEvent objects?

I've checked that combinations like Option+Backspace are unaffected. Applications that construct inconsistent NSEvents (like DRT did) can be affected, of course.

> >      // The adjustments below are only needed in backward compatibility mode, but we cannot tell what mode we are in from here.
> 
> Is this comment still correct? What is "backward compatibility mode"?

It's Dashboard compatibility mode that we extended to apply to applications linked against old WebKit, see <http://trac.webkit.org/changeset/28693>.

This comment refers to the fact that we don't generate keypress event, and thus don't expose character code for backspace and tab keys to JS (other than in backward compatibility mode). But I think that having inconsistent data internally is also bad, so I removed this comment now.

> Both keys are labeled "delete" on my keyboard, and the Unicode character for U+007F is named DELETE. Because of all these overlapping uses of the word, I think it’s confusing to use "delete" to mean the forward delete button. Maybe we should change eventSender.keyDown to work with the names "backspace" and "forward delete" and eliminate the "delete" name entirely?

AFAIK, it's only Apple's U.S. keyboards that have this trait - even international Apple keyboards have an icon for backspace. But I definitely agree that backspace/forward delete are the least ambiguous technical terms for this in WebKit code. I chose to not make this change in this patch to avoid making it larger and more confusing.

At least, the new test will make sure that DRT ports won't get this wrong.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/61153>.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61153 might have broken Chromium Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/61153
http://trac.webkit.org/changeset/61154
http://trac.webkit.org/changeset/61155