Bug 16462 - REGRESSION: access keys broken on Windows
: REGRESSION: access keys broken on Windows
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: PC Windows XP
: P1 Normal
Assigned To:
:
: Regression
:
:
  Show dependency treegraph
 
Reported: 2007-12-16 03:43 PST by
Modified: 2007-12-16 08:18 PST (History)


Attachments
proposed fix (6.58 KB, patch)
2007-12-16 05:21 PST, Alexey Proskuryakov
no flags Review Patch | Details | Formatted Diff | Diff
proposed fix (8.25 KB, patch)
2007-12-16 05:35 PST, Alexey Proskuryakov
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2007-12-16 03:43:17 PST
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.
------- Comment #1 From 2007-12-16 05:21:10 PST -------
Created an attachment (id=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.
------- Comment #2 From 2007-12-16 05:24:45 PST -------
(From update of attachment 17930 [details])
This needs some more work...
------- Comment #3 From 2007-12-16 05:35:14 PST -------
Created an attachment (id=17931) [details]
proposed fix

Fixed DRT to send system key events as appropriate.
------- Comment #4 From 2007-12-16 07:21:20 PST -------
(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.
------- Comment #5 From 2007-12-16 07:23:55 PST -------
(From update of attachment 17931 [details])
I don't entirely understand why the code to call handleAccessKey couldn't just go into keyEvent.

r=me, though
------- Comment #6 From 2007-12-16 08:18:55 PST -------
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.