Bug 130250

Summary: AX: Not able to use arrow keys to read text with VoiceOver before selection is set someplace (anyplace).
Product: WebKit Reporter: Samuel White <samuel_white>
Component: AccessibilityAssignee: Samuel White <samuel_white>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, ap, bfulgham, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, mario, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.9   
Attachments:
Description Flags
Patch.
none
Updated patch.
none
Updated patch.
none
Updated patch. none

Samuel White
Reported 2014-03-14 11:00:07 PDT
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).
Attachments
Patch. (25.93 KB, patch)
2014-03-17 10:40 PDT, Samuel White
no flags
Updated patch. (24.71 KB, patch)
2014-03-17 14:06 PDT, Samuel White
no flags
Updated patch. (26.75 KB, patch)
2014-03-17 14:54 PDT, Samuel White
no flags
Updated patch. (26.28 KB, patch)
2014-03-18 10:39 PDT, Samuel White
no flags
Samuel White
Comment 1 2014-03-17 10:40:07 PDT
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
Comment 2 2014-03-17 11:20:40 PDT
(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 3 2014-03-17 11:32:56 PDT
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
Comment 4 2014-03-17 11:48:57 PDT
(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
Comment 5 2014-03-17 11:52:34 PDT
(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
Comment 6 2014-03-17 14:06:53 PDT
Created attachment 226961 [details] Updated patch. Updates per feedback.
Samuel White
Comment 7 2014-03-17 14:08:46 PDT
(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
Comment 8 2014-03-17 14:54:44 PDT
Created attachment 226969 [details] Updated patch. Speculative fix to get win building.
chris fleizach
Comment 9 2014-03-17 16:16:02 PDT
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
Comment 10 2014-03-17 16:31:54 PDT
(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 11 2014-03-17 16:43:49 PDT
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
Comment 12 2014-03-18 09:49:58 PDT
Samuel White
Comment 13 2014-03-18 10:39:30 PDT
Created attachment 227072 [details] Updated patch. Should fix remaining other platform build issues.
Samuel White
Comment 14 2014-03-18 13:04:18 PDT
(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 15 2014-03-18 23:18:09 PDT
Comment on attachment 227072 [details] Updated patch. Clearing flags on attachment: 227072 Committed r165870: <http://trac.webkit.org/changeset/165870>
WebKit Commit Bot
Comment 16 2014-03-18 23:19:02 PDT
All reviewed patches have been landed. Closing bug.
Brent Fulgham
Comment 17 2014-03-26 12:26:22 PDT
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>.
Note You need to log in before you can comment on or make changes to this bug.