Bug 194923 - AX: Treat AXChildrenInNavigationOrder as AXChildren before adding support for aria-flowto
Summary: AX: Treat AXChildrenInNavigationOrder as AXChildren before adding support for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-21 15:52 PST by Eric Liang
Modified: 2019-02-22 20:31 PST (History)
13 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2019-02-21 16:21 PST, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (5.99 KB, patch)
2019-02-22 15:24 PST, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (6.01 KB, patch)
2019-02-22 15:35 PST, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (6.17 KB, patch)
2019-02-22 17:05 PST, Eric Liang
no flags Details | Formatted Diff | Diff
Patch (5.97 KB, patch)
2019-02-22 18:33 PST, Eric Liang
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.82 MB, application/zip)
2019-02-22 19:26 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Liang 2019-02-21 15:52:29 PST
Should treat them as the same for now, before we add support to AXChildrenInNavigationOrder.
Comment 1 Eric Liang 2019-02-21 15:52:47 PST
<rdar://problem/39540088> macOS - regression - VoiceOver reads hidden text in wrong order in Safari
Comment 2 Eric Liang 2019-02-21 16:21:23 PST
Created attachment 362662 [details]
Patch
Comment 3 chris fleizach 2019-02-21 16:47:48 PST
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
Comment 4 Eric Liang 2019-02-21 16:54:44 PST
Yea I saw NSAccessibilitySPI.h doesn't have any definitions so I hold off adding it. Will add now.
Comment 5 chris fleizach 2019-02-21 16:57:19 PST
(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
Comment 6 Lucas Forschler 2019-02-22 07:42:48 PST
rdar://problem/39540088
Comment 7 Eric Liang 2019-02-22 15:24:02 PST
Created attachment 362770 [details]
Patch
Comment 8 Build Bot 2019-02-22 15:26:13 PST
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.
Comment 9 Eric Liang 2019-02-22 15:35:47 PST
Created attachment 362772 [details]
Patch
Comment 10 chris fleizach 2019-02-22 16:24:19 PST
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
Comment 11 Eric Liang 2019-02-22 16:45:27 PST
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.
Comment 12 chris fleizach 2019-02-22 17:04:21 PST
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
Comment 13 Eric Liang 2019-02-22 17:05:18 PST
Created attachment 362797 [details]
Patch
Comment 14 Eric Liang 2019-02-22 17:51:25 PST
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
Comment 15 chris fleizach 2019-02-22 17:57:58 PST
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?
Comment 16 Eric Liang 2019-02-22 18:33:28 PST
Created attachment 362810 [details]
Patch
Comment 17 Eric Liang 2019-02-22 18:37:17 PST
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?
Comment 18 chris fleizach 2019-02-22 19:16:32 PST
(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?
Comment 19 Build Bot 2019-02-22 19:26:42 PST
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
Comment 20 Build Bot 2019-02-22 19:26:44 PST
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
Comment 21 WebKit Commit Bot 2019-02-22 20:31:39 PST
Comment on attachment 362810 [details]
Patch

Clearing flags on attachment: 362810

Committed r241983: <https://trac.webkit.org/changeset/241983>
Comment 22 WebKit Commit Bot 2019-02-22 20:31:41 PST
All reviewed patches have been landed.  Closing bug.