Bug 119605 – AX: Not able to use arrow keys to read text in a WK2 app

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Beth Dakin]

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>

[Beth Dakin]

Created attachment 208380 [details]
Patch

[Darin Adler]

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?

[Jon Lee]

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?

[Beth Dakin]

(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.

[Beth Dakin]

(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!

[Darin Adler]

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 ;-)

[Beth Dakin]

(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!

[Darin Adler]

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.

[Beth Dakin]

(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