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).
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?
(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
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
(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
(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
Created attachment 226961 [details] Updated patch. Updates per feedback.
(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.
Created attachment 226969 [details] Updated patch. Speculative fix to get win building.
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?
(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?
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
<rdar://problem/16327812>
Created attachment 227072 [details] Updated patch. Should fix remaining other platform build issues.
(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.
Comment on attachment 227072 [details] Updated patch. Clearing flags on attachment: 227072 Committed r165870: <http://trac.webkit.org/changeset/165870>
All reviewed patches have been landed. Closing bug.
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>.