On MacOS it is possible to interact with a disabled <select> element with VoiceOver. We tested on High Sierra 10.13.6 (17G65) and the lates public Mojave Beta. This happens with disabled=“disabled” and/or aria-disabled=“true” applied to the select. <select id="location1" name="location1" disabled="disabled"> <select id="location2" name="location2" aria-disabled="true"> <select id="location3" name="location3" aria-disabled="true" disabled="disabled"> In all three cases Safari shows in the node inspector that the element is disabled (under Accessibility). Please see attached test case. Steps To Reproduce: 1. With VoiceOver running open the test case 2. Reach any of the drop-downs and press space bar to activate. 3. Press VO-spacebar to expand the drop-down 4. Select any of the items in the drop-down and press VO-spacebar Results: The drop-down updates to the selected item. Regression: We were unable to test on versions older than High Sierra.
<rdar://problem/47578083>
<rdar://problem/42622018>
Created attachment 360261 [details] Patch
Comment on attachment 360261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360261&action=review > Source/WebCore/accessibility/AccessibilityMenuList.cpp:48 > + Element* element = this->element(); auto element = this->element(); if (!element || element->isDsiabled()) return false; > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3160 > + // Used by DRT to find an accessible node by its element id. comment is wrong > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3161 > + if ([attributeName isEqualToString:@"AXDRTCollaspedAttribute"]) spelled wrong AXDRTCollaspedAttribute > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:9 > + <select id="test1" disabled> no indent on these controls > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:18 > + <select id="test2" disabled aria-disabled="true"> can you add a test for disabled and aria-disabled='false" > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:34 > + //Dont have tests for 'enabled' controls because when menu list opens a popup, it hangs on LayoutTests. // We don't test for enabled controls because opening menu lists waits on the UI process to display the menu. > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:39 > + jsTestIsAsync = true; make sure indentation is 4 spaces every where > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:44 > + pressNext(); can you verify that collapsed is false before you start > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:48 > + }, 10); do we need to wait 10 or can we just do it after 0 > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:52 > + { make sure { on previous line > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:65 > + }, 20); do we need to wait 20? > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:81 > + }, 20); do we need to wait 20?
Comment on attachment 360261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360261&action=review >> Source/WebCore/accessibility/AccessibilityMenuList.cpp:48 >> + Element* element = this->element(); > > auto element = this->element(); > if (!element || element->isDsiabled()) > return false; Just to make sure is isDisabled() equivalent to isDisabledFormControl()? Because I saw other places in Accessibility we use isDisabledFormControl()
Comment on attachment 360261 [details] Patch Attachment 360261 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10906952 New failing tests: accessibility/press-not-work-for-disabled-menu-list.html
Created attachment 360266 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 360277 [details] Patch
Comment on attachment 360277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360277&action=review > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:65 > + shouldBeTrue("pressedElement.boolAttributeValue('AXDRTCollapsedAttribute')"); This checks isCollapsed is true before starting the AXPress.
Created attachment 360278 [details] Patch
Created attachment 360279 [details] Patch
Comment on attachment 360279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360279&action=review > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:2 > +<html> we should move this test just inside the Mac/ platform, since it's using a specialized Mac-only attribute for checking the value. then you can remove the win test expectation
Comment on attachment 360279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360279&action=review > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:56 > + }, 9); 9 or 0? > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:71 > + }, 10); this needs to be 10? > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:81 > + shouldBeTrue("checkedElement.boolAttributeValue('AXDRTCollapsedAttribute')"); for these different shouldBeTrue's -- can you add something before, like "After AXPress: Collapsed: " + checkedElement.boolAttributeValue('AXDRTCollapsedAttribute') so that when we see the list of results they're parseable for what they mean? > LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:85 > + }, 10); does this need to be10?
Comment on attachment 360279 [details] Patch Attachment 360279 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10909423 New failing tests: accessibility/press-not-work-for-disabled-menu-list.html
Created attachment 360284 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 360279 [details] Patch Attachment 360279 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10909409 New failing tests: accessibility/press-not-work-for-disabled-menu-list.html
Created attachment 360285 [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 360279 [details] Patch Attachment 360279 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10909479 New failing tests: accessibility/press-not-work-for-disabled-menu-list.html
Created attachment 360286 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 360279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360279&action=review >> LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:56 >> + }, 9); > > 9 or 0? The idea here is that because a successful AXPress waits on the UI process to display, it would wait for a short time to check the attribute. Although our expectations with disabled form controls don't need this wait, I still "pretend" that we need this wait because otherwise the check is meaningless. >> LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:71 >> + }, 10); > > this needs to be 10? This is an arbitrary wait time in which we assume a successful initiation of UI process needs. I don't really know how long I should set this number though.
(In reply to Eric Liang from comment #20) > Comment on attachment 360279 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360279&action=review > > >> LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:56 > >> + }, 9); > > > > 9 or 0? > > The idea here is that because a successful AXPress waits on the UI process > to display, it would wait for a short time to check the attribute. Although > our expectations with disabled form controls don't need this wait, I still > "pretend" that we need this wait because otherwise the check is meaningless. We shouldn't be using a random timeout like this to wait for that. What exactly are you waiting for? There is a lot of milestones / events that we can wait for in UIScriptController. > >> LayoutTests/accessibility/press-not-work-for-disabled-menu-list.html:71 > >> + }, 10); > > > > this needs to be 10? > > This is an arbitrary wait time in which we assume a successful initiation of > UI process needs. I don't really know how long I should set this number > though. Again, we shouldn't be doing that. The way to enforce UI process getting an IPC is to execute a script via UIScriptController
Comment on attachment 360279 [details] Patch r- because of the bad use of timeouts in the tests that could lead to this test being flaky on bots.
Created attachment 360439 [details] Patch
Comment on attachment 360439 [details] Patch The test change looks good. I'll let Chris review the actual code change.
Comment on attachment 360439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360439&action=review > Source/WebCore/accessibility/AccessibilityMenuList.cpp:60 > + if (cache) I don't think we want Document here. You can use "this" since its in reference to this menu list > LayoutTests/ChangeLog:8 > + * accessibility/press-not-work-for-disabled-menu-list.html: Added. this should be moved to Mac platform folder. it will also fail on all the other platforms
Comment on attachment 360439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360439&action=review > Source/WebCore/accessibility/AccessibilityMenuList.cpp:51 > + RenderMenuList* menuList = static_cast<RenderMenuList*>(renderer()); Don't use static_cast<>. We have casting macros, like is<> and downcast<> which assert that the cast is correct.
Created attachment 360886 [details] Patch
Created attachment 360903 [details] Patch
Comment on attachment 360903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360903&action=review > Source/WebCore/accessibility/AccessibilityMenuList.cpp:52 > + RenderMenuList* menuList = downcast<RenderMenuList>(renderer()); > + if (menuList->popupIsVisible()) You should use is<RenderMenuList>(renderer()) here; is<> does the type check and the null pointer check for you.
Created attachment 360930 [details] Patch
Comment on attachment 360930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360930&action=review > Source/WebCore/accessibility/AccessibilityMenuList.cpp:56 > + result = true; it looks like you can replace the usage of bool result with just a AXNotification directly > Source/WebCore/accessibility/AccessibilityMenuList.cpp:59 > + Ref<Document> document(m_renderer->document()); document appears to be unused > Source/WebCore/accessibility/AccessibilityMenuList.cpp:60 > + if (cache) you can do if (auto cache = axObjectCache()) cache->
Created attachment 360936 [details] Patch
Comment on attachment 360936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360936&action=review > Source/WebCore/accessibility/AccessibilityMenuList.cpp:56 > + cache->postNotification(element, result ? AXObjectCache::AXPressDidSucceed : AXObjectCache::AXPressDidFail); I meant you can declare AXNotification notification = PressDidFail and then inside the if block set it to PressDidSucc then just pass that directly
Created attachment 360939 [details] Patch
Created attachment 360942 [details] Patch
Comment on attachment 360942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360942&action=review > Source/WebCore/accessibility/AccessibilityMenuList.cpp:58 > + result = true; you don't need result at all now
Created attachment 361067 [details] Patch
Comment on attachment 361067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361067&action=review > LayoutTests/accessibility/mac/press-not-work-for-disabled-menu-list.html:57 > + menulist.press(); we're missing the expected results file in this patch
Created attachment 361070 [details] Patch
Comment on attachment 361070 [details] Patch Clearing flags on attachment: 361070 Committed r241190: <https://trac.webkit.org/changeset/241190>
All reviewed patches have been landed. Closing bug.
Looks like the test accessibility/mac/press-not-work-for-disabled-menu-list.html added in https://trac.webkit.org/changeset/241190/webkit is a flakey failure. History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmac%2Fpress-not-work-for-disabled-menu-list.html Diff: --- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/accessibility/mac/press-not-work-for-disabled-menu-list-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/accessibility/mac/press-not-work-for-disabled-menu-list-actual.txt @@ -1,3 +1,4 @@ +CONSOLE MESSAGE: line 56: TypeError: undefined is not an object (evaluating 'menulist.addNotificationListener') This tests that menu lists that are disabled will not be triggered by AXPress actions. @@ -7,7 +8,7 @@ Got notification: AXPressDidFail Got notification: AXPressDidFail Got notification: AXPressDidFail -PASS successfullyParsed is true +FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined). TEST COMPLETE
@EricLiang - are you able to look into flakiness? (In reply to Truitt Savell from comment #42) > Looks like the test > accessibility/mac/press-not-work-for-disabled-menu-list.html > > added in https://trac.webkit.org/changeset/241190/webkit > > is a flakey failure. > > History: > http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=accessibility%2Fmac%2Fpress-not-work-for- > disabled-menu-list.html > > Diff: > --- > /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/ > accessibility/mac/press-not-work-for-disabled-menu-list-expected.txt > +++ > /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/ > accessibility/mac/press-not-work-for-disabled-menu-list-actual.txt > @@ -1,3 +1,4 @@ > +CONSOLE MESSAGE: line 56: TypeError: undefined is not an object (evaluating > 'menulist.addNotificationListener') > > This tests that menu lists that are disabled will not be triggered by > AXPress actions. > > @@ -7,7 +8,7 @@ > Got notification: AXPressDidFail > Got notification: AXPressDidFail > Got notification: AXPressDidFail > -PASS successfullyParsed is true > +FAIL successfullyParsed should be true (of type boolean). Was undefined (of > type undefined). > > TEST COMPLETE
Looking into it.