Bug 119605

Summary: AX: Not able to use arrow keys to read text in a WK2 app
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: AccessibilityAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: OS X 10.8   
Attachments:
Description Flags
Patch darin: review+

Description Beth Dakin 2013-08-08 16:23:07 PDT
Arrow keys should be able to read text line-by-line in a WebView when running with VoiceOver. This feature is broken in WK2.

<rdar://problem/14281275>
Comment 1 Beth Dakin 2013-08-08 16:28:37 PDT
Created attachment 208380 [details]
Patch
Comment 2 Darin Adler 2013-08-08 18:38:03 PDT
Comment on attachment 208380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review

> Source/WebCore/page/EventHandler.cpp:3349
> +        // provides KB navigation and selection for enhanced accessibility users

Given the title of this function and the code below, I am not sure we need this comment any more. Or if we ever did!

> Source/WebKit2/ChangeLog:22
> +        This function now gives accessibility a chance to handle the event too. And it 
> +        also actually tracks whether or not the event was handled by scrolling instead of 
> +        assuming that it was and universally returning true.

Why do we need to fix the boolean result here as part of this patch? Nice fix, but how is it relevant?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:633
> +    // First give accessibility a chance to handle the event.
> +    Frame* frame = frameForEvent(event);
> +    frame->eventHandler()->handleKeyboardSelectionMovementForAccessibility(event);
> +    if (event->defaultHandled())
> +        return true;

Can the event ever be zero?
Comment 3 Jon Lee 2013-08-08 22:12:49 PDT
Comment on attachment 208380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review

> Tools/TestWebKitAPI/Tests/WebKit2/DidNotHandleKeyDown.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2010?
Comment 4 Beth Dakin 2013-08-09 11:39:35 PDT
(In reply to comment #2)
> (From update of attachment 208380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:3349
> > +        // provides KB navigation and selection for enhanced accessibility users
> 
> Given the title of this function and the code below, I am not sure we need this comment any more. Or if we ever did!
> 

Good point. I removed it.

> > Source/WebKit2/ChangeLog:22
> > +        This function now gives accessibility a chance to handle the event too. And it 
> > +        also actually tracks whether or not the event was handled by scrolling instead of 
> > +        assuming that it was and universally returning true.
> 
> Why do we need to fix the boolean result here as part of this patch? Nice fix, but how is it relevant?
> 

This could be done in a separate patch, and maybe should be. This change is required to get this feature working in some WebKit2 apps that were previously swallowing some events that they can not longer swallow. Those events need to be sent to WK2 in order to support this accessibility feature. But in the non-accessibility case, WK2 needs to accurately report whether it handled the event or not in order to avoid behavioral regressions.

> > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:633
> > +    // First give accessibility a chance to handle the event.
> > +    Frame* frame = frameForEvent(event);
> > +    frame->eventHandler()->handleKeyboardSelectionMovementForAccessibility(event);
> > +    if (event->defaultHandled())
> > +        return true;
> 
> Can the event ever be zero?

By the time we reach this function, no it cannot.
Comment 5 Beth Dakin 2013-08-09 11:39:51 PDT
(In reply to comment #3)
> (From update of attachment 208380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit2/DidNotHandleKeyDown.cpp:2
> > + * Copyright (C) 2010 Apple Inc. All rights reserved.
> 
> 2010?

Oops, good catch.

Thanks Darin!
Comment 6 Darin Adler 2013-08-09 12:56:21 PDT
Comment on attachment 208380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review

>>> Tools/TestWebKitAPI/Tests/WebKit2/DidNotHandleKeyDown.cpp:2
>>> + * Copyright (C) 2010 Apple Inc. All rights reserved.
>> 
>> 2010?
> 
> Oops, good catch.
> 
> Thanks Darin!

No fair, you thanked me but Jon noticed the wrong date ;-)
Comment 7 Beth Dakin 2013-08-09 12:57:01 PDT
(In reply to comment #6)
> (From update of attachment 208380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review
> 
> >>> Tools/TestWebKitAPI/Tests/WebKit2/DidNotHandleKeyDown.cpp:2
> >>> + * Copyright (C) 2010 Apple Inc. All rights reserved.
> >> 
> >> 2010?
> > 
> > Oops, good catch.
> > 
> > Thanks Darin!
> 
> No fair, you thanked me but Jon noticed the wrong date ;-)

Haha, oops! Sorry Jon! Thanks to you too!
Comment 8 Darin Adler 2013-08-09 12:57:18 PDT
Comment on attachment 208380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review

>>> Source/WebKit2/ChangeLog:22
>>> +        assuming that it was and universally returning true.
>> 
>> Why do we need to fix the boolean result here as part of this patch? Nice fix, but how is it relevant?
> 
> This could be done in a separate patch, and maybe should be. This change is required to get this feature working in some WebKit2 apps that were previously swallowing some events that they can not longer swallow. Those events need to be sent to WK2 in order to support this accessibility feature. But in the non-accessibility case, WK2 needs to accurately report whether it handled the event or not in order to avoid behavioral regressions.

I think it would be neat to land this in two steps: Add the bool first and then add handleKeyboardSelectionMovementForAccessibility. But not absolutely required.
Comment 9 Beth Dakin 2013-08-09 12:58:25 PDT
(In reply to comment #8)
> (From update of attachment 208380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208380&action=review
> 
> >>> Source/WebKit2/ChangeLog:22
> >>> +        assuming that it was and universally returning true.
> >> 
> >> Why do we need to fix the boolean result here as part of this patch? Nice fix, but how is it relevant?
> > 
> > This could be done in a separate patch, and maybe should be. This change is required to get this feature working in some WebKit2 apps that were previously swallowing some events that they can not longer swallow. Those events need to be sent to WK2 in order to support this accessibility feature. But in the non-accessibility case, WK2 needs to accurately report whether it handled the event or not in order to avoid behavioral regressions.
> 
> I think it would be neat to land this in two steps: Add the bool first and then add handleKeyboardSelectionMovementForAccessibility. But not absolutely required.

Oops, I just committed right before I read this. Sorry about that.

http://trac.webkit.org/changeset/153907