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

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 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 Alexey Proskuryakov 2007-12-16 05:21:10 PST
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.
Comment 2 Alexey Proskuryakov 2007-12-16 05:24:45 PST
Comment on attachment 17930 [details]
proposed fix

This needs some more work...
Comment 3 Alexey Proskuryakov 2007-12-16 05:35:14 PST
Created attachment 17931 [details]
proposed fix

Fixed DRT to send system key events as appropriate.
Comment 4 Darin Adler 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 Darin Adler 2007-12-16 07:23:55 PST
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
Comment 6 Alexey Proskuryakov 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.