Should treat them as the same for now, before we add support to AXChildrenInNavigationOrder.
<rdar://problem/39540088> macOS - regression - VoiceOver reads hidden text in wrong order in Safari
Created attachment 362662 [details] Patch
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
Yea I saw NSAccessibilitySPI.h doesn't have any definitions so I hold off adding it. Will add now.
(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
rdar://problem/39540088
Created attachment 362770 [details] Patch
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.
Created attachment 362772 [details] Patch
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 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 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
Created attachment 362797 [details] Patch
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 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?
Created attachment 362810 [details] Patch
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?
(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 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
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 on attachment 362810 [details] Patch Clearing flags on attachment: 362810 Committed r241983: <https://trac.webkit.org/changeset/241983>
All reviewed patches have been landed. Closing bug.