Bug 130250 – AX: Not able to use arrow keys to read text with VoiceOver before selection is set someplace (anyplace).

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

[Samuel White]

If selection hasn't yet been set, accessibility clients (VoiceOver) can't move selection around using arrow keys (works correctly AFTER initial selection has been set).

[Samuel White]

Created attachment 226938 [details]
Patch.

Possible we shouldn't expose the ability to toggle enhanced accessibility to every platform. Looks like iOS doesn't key off it for anything. Thoughts?

[chris fleizach]

(In reply to comment #1)
> Created an attachment (id=226938) [details]
> Patch.
> 
> Possible we shouldn't expose the ability to toggle enhanced accessibility to every platform. Looks like iOS doesn't key off it for anything. Thoughts?

Seems like iOS could key off of this in a future world

[chris fleizach]

Comment on attachment 226938 [details]
Patch.

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

> LayoutTests/platform/mac/accessibility/selection-initial.html:33
> +        // Enable enhanced accessibility (necessary for accessibility specific seleciton handling).

spelling
seleciton

> LayoutTests/platform/mac/accessibility/selection-initial.html:67
> +        accessibilityController.enableEnhancedAccessibility(false);

this should be disabled in the TestRunner code itself, much like we disable AX after each test

> Source/WebCore/page/EventHandler.cpp:3063
> +    

unnecessary whitespace change

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:83
> +WK_EXPORT void WKAccessibilityEnableEnhancedAccessibility(bool enable);

"enable" not necessary

[Samuel White]

(In reply to comment #3)
> (From update of attachment 226938 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226938&action=review
> 
> > LayoutTests/platform/mac/accessibility/selection-initial.html:33
> > +        // Enable enhanced accessibility (necessary for accessibility specific seleciton handling).
> 
> spelling
> seleciton
> 
> > LayoutTests/platform/mac/accessibility/selection-initial.html:67
> > +        accessibilityController.enableEnhancedAccessibility(false);
> 
> this should be disabled in the TestRunner code itself, much like we disable AX after each test

Good point. I'm only seeing 'resetToConsistentState' on the AX controller. Can you point me toward the area you're mentioning. Thanks.

> 
> > Source/WebCore/page/EventHandler.cpp:3063
> > +    
> 
> unnecessary whitespace change
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:83
> > +WK_EXPORT void WKAccessibilityEnableEnhancedAccessibility(bool enable);
> 
> "enable" not necessary

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 226938 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=226938&action=review
> > 
> > > LayoutTests/platform/mac/accessibility/selection-initial.html:33
> > > +        // Enable enhanced accessibility (necessary for accessibility specific seleciton handling).
> > 
> > spelling
> > seleciton
> > 
> > > LayoutTests/platform/mac/accessibility/selection-initial.html:67
> > > +        accessibilityController.enableEnhancedAccessibility(false);
> > 
> > this should be disabled in the TestRunner code itself, much like we disable AX after each test
> 
> Good point. I'm only seeing 'resetToConsistentState' on the AX controller. Can you point me toward the area you're mentioning. Thanks.

https://bugs.webkit.org/attachment.cgi?id=222286&action=review

> 
> > 
> > > Source/WebCore/page/EventHandler.cpp:3063
> > > +    
> > 
> > unnecessary whitespace change
> > 
> > > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:83
> > > +WK_EXPORT void WKAccessibilityEnableEnhancedAccessibility(bool enable);
> > 
> > "enable" not necessary

[Samuel White]

Created attachment 226961 [details]
Updated patch.

Updates per feedback.

[Samuel White]

(In reply to comment #3)
> (From update of attachment 226938 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226938&action=review
> 
> > LayoutTests/platform/mac/accessibility/selection-initial.html:33
> > +        // Enable enhanced accessibility (necessary for accessibility specific seleciton handling).
> 
> spelling
> seleciton

Oops, fixed.

> 
> > LayoutTests/platform/mac/accessibility/selection-initial.html:67
> > +        accessibilityController.enableEnhancedAccessibility(false);
> 
> this should be disabled in the TestRunner code itself, much like we disable AX after each test

Fixed.

> 
> > Source/WebCore/page/EventHandler.cpp:3063
> > +    
> 
> unnecessary whitespace change

This was me fixing the only non indented newline.

> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:83
> > +WK_EXPORT void WKAccessibilityEnableEnhancedAccessibility(bool enable);
> 
> "enable" not necessary

Removed.

[Samuel White]

Created attachment 226969 [details]
Updated patch.

Speculative fix to get win building.

[chris fleizach]

Comment on attachment 226969 [details]
Updated patch.

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

> Source/WebCore/page/EventHandler.cpp:3063
> +    

superfluous line change should be removed

> Source/WebCore/ChangeLog:8
> +        If initial selection isn't set when we handle selection movement for accessibility we need to set it.

why do we need to set it?

[Samuel White]

(In reply to comment #9)
> (From update of attachment 226969 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226969&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:3063
> > +    
> 
> superfluous line change should be removed

Can you elaborate on why this change should be removed? It was the only incorrectly indented line, fixing it seems helpful and related as this function was being reworked.

> 
> > Source/WebCore/ChangeLog:8
> > +        If initial selection isn't set when we handle selection movement for accessibility we need to set it.
> 
> why do we need to set it?

Are you insinuating that the comment should be more detailed?

[chris fleizach]

Comment on attachment 226969 [details]
Updated patch.

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

>>> Source/WebCore/page/EventHandler.cpp:3063
>>> +    
>> 
>> superfluous line change should be removed
> 
> Can you elaborate on why this change should be removed? It was the only incorrectly indented line, fixing it seems helpful and related as this function was being reworked.

it looks like (i might be incorrect), that you're adding a line with 4 empty spaces in it.
I think the preference in WebKit is to have lines with no empty spaces if possible

>>> Source/WebCore/ChangeLog:8
>>> +        If initial selection isn't set when we handle selection movement for accessibility we need to set it.
>> 
>> why do we need to set it?
> 
> Are you insinuating that the comment should be more detailed?

Yes please

[Samuel White]

<rdar://problem/16327812>

[Samuel White]

Created attachment 227072 [details]
Updated patch.

Should fix remaining other platform build issues.

[Samuel White]

(In reply to comment #11)
> (From update of attachment 226969 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226969&action=review
> 
> >>> Source/WebCore/page/EventHandler.cpp:3063
> >>> +    
> >> 
> >> superfluous line change should be removed
> > 
> > Can you elaborate on why this change should be removed? It was the only incorrectly indented line, fixing it seems helpful and related as this function was being reworked.
> 
> it looks like (i might be incorrect), that you're adding a line with 4 empty spaces in it.
> I think the preference in WebKit is to have lines with no empty spaces if possible
> 

We should prob. consider adding this to the style guide.
https://bugs.webkit.org/show_bug.cgi?id=130415
Deleted the indents to make spacing uniform. Thanks.

> >>> Source/WebCore/ChangeLog:8
> >>> +        If initial selection isn't set when we handle selection movement for accessibility we need to set it.
> >> 
> >> why do we need to set it?
> > 
> > Are you insinuating that the comment should be more detailed?
> 
> Yes please

Done.

[WebKit Commit Bot]

Comment on attachment 227072 [details]
Updated patch.

Clearing flags on attachment: 227072

Committed r165870: <http://trac.webkit.org/changeset/165870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This change caused tests on Windows to start crashing. This is because of the weird way DRT on Windows links to the WebCore test stuff. We can wind up with multiple definitions for the symbols.

I checked in a correction in r166307 <https://trac.webkit.org/r166307>.