REGRESSION: access keys broken on Windows

• Component: Forms
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

I realized that I broke access keys with my fix to bug 16436. Layout tests didn't catch that, because we send WM_KEYDOWN/WM_CHAR events with Alt key pressed, and not WM_SYSKEYDOWN.

Patch forthcoming.

[Alexey Proskuryakov]

Created attachment 17930 [details]
proposed fix

I hate to submit this without an automated test - but it looks like while our behavior matches IE well, the underlying mechanics are quite different. So, while it's possible to catch the changes with a test, such a test would fail for certain legitimate changes, too.

[Alexey Proskuryakov]

Comment on attachment 17930 [details]
proposed fix

This needs some more work...

[Alexey Proskuryakov]

Created attachment 17931 [details]
proposed fix

Fixed DRT to send system key events as appropriate.

[Darin Adler]

(In reply to comment #1)
> I hate to submit this without an automated test - but it looks like while our
> behavior matches IE well, the underlying mechanics are quite different. So,
> while it's possible to catch the changes with a test, such a test would fail
> for certain legitimate changes, too.

It's OK to have an automated test that captures our current behavior, even if it's too specific, and requires behavior that is not necessarily "correct". Our automated tests help us notice behavior changes. Failures in them don't have to reflect bugs.

[Darin Adler]

Comment on attachment 17931 [details]
proposed fix

I don't entirely understand why the code to call handleAccessKey couldn't just go into keyEvent.

r=me, though

[Alexey Proskuryakov]

Committed revision 28770.

(In reply to comment #4)
> It's OK to have an automated test that captures our current behavior, even if
> it's too specific, and requires behavior that is not necessarily "correct". Our
> automated tests help us notice behavior changes. Failures in them don't have to
> reflect bugs.

I looked into it a bit more, and it seems that a test cannot be written without changing code - there is no way to dispatch a WM_SYSCHAR with alt key pressed.

I agree that it is possible to write some kind of test for this, possibly by adding functionality to DRT, but somehow, it feels wrong to me.

> I don't entirely understand why the code to call handleAccessKey couldn't just
> go into keyEvent.

Checking for system key is entirely platform-specific, not even wxWindows does that. I still have a feeling that a perfect implementation of this feature on Windows should use some mechanism that's separate form event dispatch.