Bug 193878 – AX: VoiceOver allows to interact with a disabled <select> element

• Component: Accessibility
• Status: RESOLVED FIXED

[Eric Liang]

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.

[Radar WebKit Bug Importer]

<rdar://problem/47578083>

[Eric Liang]

<rdar://problem/42622018>

[Eric Liang]

Created attachment 360261 [details]
Patch

[chris fleizach]

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?

[Eric Liang]

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()

[EWS Watchlist]

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

[EWS Watchlist]

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

[Eric Liang]

Created attachment 360277 [details]
Patch

[Eric Liang]

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.

[Eric Liang]

Created attachment 360278 [details]
Patch

[Eric Liang]

Created attachment 360279 [details]
Patch

[chris fleizach]

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

[chris fleizach]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Eric Liang]

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.

[Ryosuke Niwa]

(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

[Ryosuke Niwa]

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.

[Eric Liang]

Created attachment 360439 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 360439 [details]
Patch

The test change looks good. I'll let Chris review the actual code change.

[chris fleizach]

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

[Simon Fraser (smfr)]

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.

[Eric Liang]

Created attachment 360886 [details]
Patch

[Eric Liang]

Created attachment 360903 [details]
Patch

[Simon Fraser (smfr)]

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.

[Eric Liang]

Created attachment 360930 [details]
Patch

[chris fleizach]

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->

[Eric Liang]

Created attachment 360936 [details]
Patch

[chris fleizach]

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

[Eric Liang]

Created attachment 360939 [details]
Patch

[Eric Liang]

Created attachment 360942 [details]
Patch

[chris fleizach]

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

[Eric Liang]

Created attachment 361067 [details]
Patch

[chris fleizach]

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

[Eric Liang]

Created attachment 361070 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361070 [details]
Patch

Clearing flags on attachment: 361070

Committed r241190: <https://trac.webkit.org/changeset/241190>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[chris fleizach]

@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

[Eric Liang]

Looking into it.