Bug 194923 – AX: Treat AXChildrenInNavigationOrder as AXChildren before adding support for aria-flowto

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

[Eric Liang]

Should treat them as the same for now, before we add support to AXChildrenInNavigationOrder.

[Eric Liang]

<rdar://problem/39540088> macOS - regression - VoiceOver reads hidden text in wrong order in Safari

[Eric Liang]

Created attachment 362662 [details]
Patch

[chris fleizach]

Comment on attachment 362662 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        * accessibility/mac/WebAccessibilityObjectWrapperMac.mm:

can you add a description for why you're doing this

also can you add a test that verifies that children is the same as children in navigation order

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:156
> +#ifndef NSAccessibilityChildrenInNavigationOrderAttribute

this is defined as a private attribute so we should make this available through a SPI.h file

See 
/Volumes/data/web/OpenSource/Source/WebCore/PAL/pal/spi/cocoa/NSColorSPI.h

for example

and it should go in
/Volumes/data/web/OpenSource/Source/WebCore/PAL/pal/spi/mac/NSAccessibilitySPI.h

[Eric Liang]

Yea I saw NSAccessibilitySPI.h doesn't have any definitions so I hold off adding it. Will add now.

[chris fleizach]

(In reply to Eric Liang from comment #4)
> Yea I saw NSAccessibilitySPI.h doesn't have any definitions so I hold off
> adding it. Will add now.

I don't think you need to add the #defines. it's already defined. you just need to export the symbols correctly like in

/WebCore/PAL/pal/spi/cocoa/NSTouchBarSPI.h

[Lucas Forschler]

rdar://problem/39540088

[Eric Liang]

Created attachment 362770 [details]
Patch

[EWS Watchlist]

Attachment 362770 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Liang]

Created attachment 362772 [details]
Patch

[chris fleizach]

Comment on attachment 362772 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        AXChildrenInNavigationOrder returns different array of elements than that from AXChildren for AppKit. This is because currently WebKit doesn't support AXChildrenInNavigationOrder, and though AppKit's fallback, AppKit reorder them based on horizontal positions. Added this temporary support for WebKit so that AppKit doesn't reorder them.

can you break this up on smaller lines?

> LayoutTests/accessibility/mac/children-in-navigation-order-returns-children.html:37
> +        shouldBe("children1.length", "2");

I think the 2 children in this div might be the same role already. what if you added a button in here to ensure there were different objects

[Eric Liang]

Comment on attachment 362772 [details]
Patch

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

>> LayoutTests/accessibility/mac/children-in-navigation-order-returns-children.html:37
>> +        shouldBe("children1.length", "2");
> 
> I think the 2 children in this div might be the same role already. what if you added a button in here to ensure there were different objects

I picked the example where AppKit reorders. And the 2 children inside this link is 1) AXStaticText 2)Group that contains another AXStaticText
However in DRT, we don't have AppKit that reorders it. So instead, what we are really testing, in this test case, is that asking AXChildrenInNavigationOrder gives us a non-empty array.
I can also add another button just to make sure.

[chris fleizach]

Comment on attachment 362772 [details]
Patch

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

>>> LayoutTests/accessibility/mac/children-in-navigation-order-returns-children.html:37
>>> +        shouldBe("children1.length", "2");
>> 
>> I think the 2 children in this div might be the same role already. what if you added a button in here to ensure there were different objects
> 
> I picked the example where AppKit reorders. And the 2 children inside this link is 1) AXStaticText 2)Group that contains another AXStaticText
> However in DRT, we don't have AppKit that reorders it. So instead, what we are really testing, in this test case, is that asking AXChildrenInNavigationOrder gives us a non-empty array.
> I can also add another button just to make sure.

we also want to test that the result is the same for AXChildren and InNavOrder

[Eric Liang]

Created attachment 362797 [details]
Patch

[Eric Liang]

shouldBeTrue("children0[i].role == children1[i].role"); I think should be enough to test that results from AXChildren and InNavOrder are the same?
(In reply to chris fleizach from comment #12)
> Comment on attachment 362772 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362772&action=review
> 
> >>> LayoutTests/accessibility/mac/children-in-navigation-order-returns-children.html:37
> >>> +        shouldBe("children1.length", "2");
> >> 
> >> I think the 2 children in this div might be the same role already. what if you added a button in here to ensure there were different objects
> > 
> > I picked the example where AppKit reorders. And the 2 children inside this link is 1) AXStaticText 2)Group that contains another AXStaticText
> > However in DRT, we don't have AppKit that reorders it. So instead, what we are really testing, in this test case, is that asking AXChildrenInNavigationOrder gives us a non-empty array.
> > I can also add another button just to make sure.
> 
> we also want to test that the result is the same for AXChildren and
> InNavOrder

[chris fleizach]

Comment on attachment 362797 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This is needed because on the Mac, if AXChildrenInNavigationOrder returns empty,  AppKit will ask for AXChildren and then reorder them based on horizontal positions. Added this temporary support for WebKit so that AppKit doesn't reorder them.

this line is now too long!

too many spaces
"mpty, AppKit"

why is this temporary?

[Eric Liang]

Created attachment 362810 [details]
Patch

[Eric Liang]

Guess I was trying to say until aria-flow-to was added, this implementation of InNavOrder is a temporary support.(In reply to chris fleizach from comment #15)
> Comment on attachment 362797 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362797&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This is needed because on the Mac, if AXChildrenInNavigationOrder returns empty,  AppKit will ask for AXChildren and then reorder them based on horizontal positions. Added this temporary support for WebKit so that AppKit doesn't reorder them.
> 
> this line is now too long!
> 
> too many spaces
> "mpty, AppKit"
> 
> why is this temporary?

[chris fleizach]

(In reply to Eric Liang from comment #17)
> Guess I was trying to say until aria-flow-to was added, this implementation
> of InNavOrder is a temporary support.(In reply to chris fleizach from

I was thinking more and not sure that aria-flowto will be able to be implemented here, because we need aria-flowto to jump around the whole page... but that's another story

> comment #15)
> > Comment on attachment 362797 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=362797&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        This is needed because on the Mac, if AXChildrenInNavigationOrder returns empty,  AppKit will ask for AXChildren and then reorder them based on horizontal positions. Added this temporary support for WebKit so that AppKit doesn't reorder them.
> > 
> > this line is now too long!
> > 
> > too many spaces
> > "mpty, AppKit"
> > 
> > why is this temporary?

[EWS Watchlist]

Comment on attachment 362810 [details]
Patch

Attachment 362810 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11255992

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html

[EWS Watchlist]

Created attachment 362813 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[WebKit Commit Bot]

Comment on attachment 362810 [details]
Patch

Clearing flags on attachment: 362810

Committed r241983: <https://trac.webkit.org/changeset/241983>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.