Summary: | AX: VoiceOver allows to interact with a disabled <select> element | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Liang <ericliang> | ||||||||||||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||
Severity: | Critical | CC: | aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, ericliang, ews-watchlist, jcraig, jdiggs, rniwa, samuel_white, simon.fraser, tsavell, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=196505 | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Eric Liang
2019-01-26 19:39:18 PST
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. |