Selection changes that come as a result of focusing an element need to be coordinated by AT to handle both a selection change notification and a focus change notification. To simplify this, a flag should be set in intent to allow platforms to add appropriate context to their notifications. This will make it easier to avoid things like double speak and/or needing to duplicate logic.
<rdar://problem/21642883>
Created attachment 255967 [details] patch
Created attachment 255970 [details] patch
Comment on attachment 255970 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=255970&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1686 > + if (AXObjectCache* cache = axObjectCache()) i don't think you have to worry about axObjectCache() being nullptr inside of an AX object > Source/WebCore/dom/Element.h:325 > + static AXTextStateChangeIntent defaultFocusTextStateChangeIntent() { return AXTextStateChangeIntent(AXTextStateChangeTypeSelectionMove, AXTextSelection { AXTextSelectionDirectionDiscontiguous, AXTextSelectionGranularityUnknown, true }); } is this one different from the default AXTextStateChangeIntent(); for cases, like select(Element::defaultFocusTextStateChangeIntent()); it would be nice to not have to pass the argument > Source/WebCore/editing/FrameSelection.cpp:174 > + AXTextStateChangeIntent newIntent; can we remove the else and just initialize with newIntent = intent; > Source/WebCore/html/HTMLTextFormControlElement.h:113 > + void restoreCachedSelection(const AXTextStateChangeIntent& = AXTextStateChangeIntent()); AXTextStateChangeIntent is looking a bit wordy. Since this is being used all over non accessibility code, maybe the name should be changed to something like TextIntention
(In reply to comment #4) > Comment on attachment 255970 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255970&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1686 > > + if (AXObjectCache* cache = axObjectCache()) > > i don't think you have to worry about axObjectCache() being nullptr inside > of an AX object ok > > > Source/WebCore/dom/Element.h:325 > > + static AXTextStateChangeIntent defaultFocusTextStateChangeIntent() { return AXTextStateChangeIntent(AXTextStateChangeTypeSelectionMove, AXTextSelection { AXTextSelectionDirectionDiscontiguous, AXTextSelectionGranularityUnknown, true }); } > > is this one different from the default AXTextStateChangeIntent(); > for cases, like > select(Element::defaultFocusTextStateChangeIntent()); > it would be nice to not have to pass the argument Yes. The default AXTextStateChangeIntent() is all unknown types and focusChange is false. I could have the default value be Element::defaultFocusTextStateChangeIntent() but that feels dangerous. > > > Source/WebCore/editing/FrameSelection.cpp:174 > > + AXTextStateChangeIntent newIntent; > > can we remove the else and just initialize with > newIntent = intent; ok > > > Source/WebCore/html/HTMLTextFormControlElement.h:113 > > + void restoreCachedSelection(const AXTextStateChangeIntent& = AXTextStateChangeIntent()); > > AXTextStateChangeIntent is looking a bit wordy. Since this is being used > all over non accessibility code, maybe the name should be changed to > something like > > TextIntention Would it be ok to do that in a future patch? I'd like to keep this one small.
Created attachment 256044 [details] patch
Created attachment 256045 [details] patch
(In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 255970 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=255970&action=review > > > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1686 > > > + if (AXObjectCache* cache = axObjectCache()) > > > > i don't think you have to worry about axObjectCache() being nullptr inside > > of an AX object > > ok > > > > > > Source/WebCore/dom/Element.h:325 > > > + static AXTextStateChangeIntent defaultFocusTextStateChangeIntent() { return AXTextStateChangeIntent(AXTextStateChangeTypeSelectionMove, AXTextSelection { AXTextSelectionDirectionDiscontiguous, AXTextSelectionGranularityUnknown, true }); } > > > > is this one different from the default AXTextStateChangeIntent(); > > for cases, like > > select(Element::defaultFocusTextStateChangeIntent()); > > it would be nice to not have to pass the argument > > Yes. The default AXTextStateChangeIntent() is all unknown types and > focusChange is false. I could have the default value be > Element::defaultFocusTextStateChangeIntent() but that feels dangerous. > > > > > > Source/WebCore/editing/FrameSelection.cpp:174 > > > + AXTextStateChangeIntent newIntent; > > > > can we remove the else and just initialize with > > newIntent = intent; > > ok > > > > > > Source/WebCore/html/HTMLTextFormControlElement.h:113 > > > + void restoreCachedSelection(const AXTextStateChangeIntent& = AXTextStateChangeIntent()); > > > > AXTextStateChangeIntent is looking a bit wordy. Since this is being used > > all over non accessibility code, maybe the name should be changed to > > something like > > > > TextIntention > > Would it be ok to do that in a future patch? I'd like to keep this one small. probably ok
Comment on attachment 256045 [details] patch Clearing flags on attachment: 256045 Committed r186256: <http://trac.webkit.org/changeset/186256>
All reviewed patches have been landed. Closing bug.
The new test is failing on some bots: https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK1%20(Tests)/r186256%20(5807)/platform/mac/accessibility/selection-notification-focus-change-pretty-diff.html
Comment on attachment 256045 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=256045&action=review > LayoutTests/platform/mac/accessibility/selection-notification-focus-change-expected.txt:16 > +PASS axTextFocusChangeThree is true Having those lines after the TEST COMPLETE is a good sign something is wrong with the test.
(In reply to comment #11) > The new test is failing on some bots: > https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK1%20(Tests)/ > r186256%20(5807)/platform/mac/accessibility/selection-notification-focus- > change-pretty-diff.html Tentative layout test fix landed in <http://trac.webkit.org/changeset/186264>.