Bug 193878

Summary: AX: VoiceOver allows to interact with a disabled <select> element
Product: WebKit Reporter: Eric Liang <ericliang>
Component: AccessibilityAssignee: 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 Flags
Patch
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Archive of layout-test-results from ews117 for mac-highsierra
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Eric Liang 2019-01-26 19:39:18 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-01-26 19:39:31 PST
<rdar://problem/47578083>
Comment 2 Eric Liang 2019-01-26 19:41:46 PST
<rdar://problem/42622018>
Comment 3 Eric Liang 2019-01-26 19:50:38 PST
Created attachment 360261 [details]
Patch
Comment 4 chris fleizach 2019-01-26 20:12:07 PST
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 5 Eric Liang 2019-01-26 20:57:59 PST
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 6 EWS Watchlist 2019-01-26 21:32:20 PST
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
Comment 7 EWS Watchlist 2019-01-26 21:32:31 PST
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
Comment 8 Eric Liang 2019-01-27 00:37:57 PST
Created attachment 360277 [details]
Patch
Comment 9 Eric Liang 2019-01-27 00:41:10 PST
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.
Comment 10 Eric Liang 2019-01-27 00:44:03 PST
Created attachment 360278 [details]
Patch
Comment 11 Eric Liang 2019-01-27 00:45:36 PST
Created attachment 360279 [details]
Patch
Comment 12 chris fleizach 2019-01-27 01:12:02 PST
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 13 chris fleizach 2019-01-27 01:14:04 PST
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 14 EWS Watchlist 2019-01-27 01:47:52 PST
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
Comment 15 EWS Watchlist 2019-01-27 01:47:54 PST
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 16 EWS Watchlist 2019-01-27 01:55:11 PST
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
Comment 17 EWS Watchlist 2019-01-27 01:55:13 PST
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 18 EWS Watchlist 2019-01-27 02:29:26 PST
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
Comment 19 EWS Watchlist 2019-01-27 02:29:28 PST
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 20 Eric Liang 2019-01-27 17:30:59 PST
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.
Comment 21 Ryosuke Niwa 2019-01-28 01:17:30 PST
(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 22 Ryosuke Niwa 2019-01-28 01:18:10 PST
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.
Comment 23 Eric Liang 2019-01-28 21:46:22 PST
Created attachment 360439 [details]
Patch
Comment 24 Ryosuke Niwa 2019-01-29 00:51:38 PST
Comment on attachment 360439 [details]
Patch

The test change looks good. I'll let Chris review the actual code change.
Comment 25 chris fleizach 2019-01-29 08:36:30 PST
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 26 Simon Fraser (smfr) 2019-01-29 13:32:58 PST
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.
Comment 27 Eric Liang 2019-02-01 13:08:09 PST
Created attachment 360886 [details]
Patch
Comment 28 Eric Liang 2019-02-01 14:29:25 PST
Created attachment 360903 [details]
Patch
Comment 29 Simon Fraser (smfr) 2019-02-01 15:07:48 PST
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.
Comment 30 Eric Liang 2019-02-01 17:40:44 PST
Created attachment 360930 [details]
Patch
Comment 31 chris fleizach 2019-02-01 17:44:30 PST
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->
Comment 32 Eric Liang 2019-02-01 17:52:32 PST
Created attachment 360936 [details]
Patch
Comment 33 chris fleizach 2019-02-01 17:53:52 PST
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
Comment 34 Eric Liang 2019-02-01 18:03:13 PST
Created attachment 360939 [details]
Patch
Comment 35 Eric Liang 2019-02-01 18:20:14 PST
Created attachment 360942 [details]
Patch
Comment 36 chris fleizach 2019-02-02 07:34:38 PST
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
Comment 37 Eric Liang 2019-02-04 10:02:04 PST
Created attachment 361067 [details]
Patch
Comment 38 chris fleizach 2019-02-04 10:04:47 PST
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
Comment 39 Eric Liang 2019-02-04 10:15:50 PST
Created attachment 361070 [details]
Patch
Comment 40 WebKit Commit Bot 2019-02-08 00:37:31 PST
Comment on attachment 361070 [details]
Patch

Clearing flags on attachment: 361070

Committed r241190: <https://trac.webkit.org/changeset/241190>
Comment 41 WebKit Commit Bot 2019-02-08 00:37:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Truitt Savell 2019-02-08 15:18:11 PST
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
Comment 43 chris fleizach 2019-02-08 17:26:06 PST
@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
Comment 44 Eric Liang 2019-02-08 17:29:28 PST
Looking into it.