Bug 196326 - [WK2] AccessibilityEventsEnabled setting does not persist after a WebProcess crash
Summary: [WK2] AccessibilityEventsEnabled setting does not persist after a WebProcess ...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-27 16:31 PDT by Chris Dumez
Modified: 2020-10-12 14:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.73 KB, patch)
2019-03-27 16:34 PDT, Chris Dumez
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-03-27 16:31:53 PDT
AccessibilityEventsEnabled setting does not persist after a WebProcess crash.
Comment 1 Chris Dumez 2019-03-27 16:34:52 PDT
Created attachment 366128 [details]
Patch
Comment 2 Chris Dumez 2019-03-27 16:35:56 PDT
Comment on attachment 366128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366128&action=review

> Source/WebKit/UIProcess/WebPageProxy.h:2211
> +    bool m_areAccessibilityEventsEnabled { true };

I set it to true by default because it was true by default in WebProcess side:
Source/WebCore/page/Settings.yaml:
accessibilityEventsEnabled:
  initial: true
  conditional: ACCESSIBILITY_EVENTS
Comment 3 Alex Christensen 2019-03-27 16:41:09 PDT
Comment on attachment 366128 [details]
Patch

Can we not make a unit test using _killWebContentProcessAndResetState ?
Comment 4 chris fleizach 2019-03-27 16:46:27 PDT
Comment on attachment 366128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366128&action=review

> Source/WebKit/Shared/WebPageCreationParameters.cpp:47
> +    encoder << areAccessibilityEventsEnabled;

not sure what current naming guidelines are, but "are" seems unnecessary.

"accessbilityEventsEnabled" seems like it might be enough

>> Source/WebKit/UIProcess/WebPageProxy.h:2211
>> +    bool m_areAccessibilityEventsEnabled { true };
> 
> I set it to true by default because it was true by default in WebProcess side:
> Source/WebCore/page/Settings.yaml:
> accessibilityEventsEnabled:
>   initial: true
>   conditional: ACCESSIBILITY_EVENTS

correct.
Comment 5 Chris Dumez 2019-03-27 16:49:04 PDT
(In reply to chris fleizach from comment #4)
> Comment on attachment 366128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366128&action=review
> 
> > Source/WebKit/Shared/WebPageCreationParameters.cpp:47
> > +    encoder << areAccessibilityEventsEnabled;
> 
> not sure what current naming guidelines are, but "are" seems unnecessary.
> 
> "accessbilityEventsEnabled" seems like it might be enough

This is per WebKit coding style though:
https://webkit.org/code-style-guidelines/#names-bool
Comment 6 Chris Dumez 2019-03-27 16:49:57 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 366128 [details]
> Patch
> 
> Can we not make a unit test using _killWebContentProcessAndResetState ?

What do I observe? I do not know what accessibility events are :)
Comment 7 Geoffrey Garen 2019-03-27 16:52:17 PDT
(In reply to Chris Dumez from comment #6)
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 366128 [details]
> > Patch
> > 
> > Can we not make a unit test using _killWebContentProcessAndResetState ?
> 
> What do I observe? I do not know what accessibility events are :)

Chris F, can you make a recommendation here?
Comment 8 Chris Dumez 2019-03-27 16:57:03 PDT
(In reply to Geoffrey Garen from comment #7)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Alex Christensen from comment #3)
> > > Comment on attachment 366128 [details]
> > > Patch
> > > 
> > > Can we not make a unit test using _killWebContentProcessAndResetState ?
> > 
> > What do I observe? I do not know what accessibility events are :)
> 
> Chris F, can you make a recommendation here?

Note that for testing, we would also need an SPI to actually set the state to enabled or disabled. Currently, there is only SPI to ask to update the state and it relies on _AXSWebAccessibilityEventsEnabled() internally, the state is not passed by the client.
Comment 9 chris fleizach 2019-03-28 07:34:57 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Geoffrey Garen from comment #7)
> > (In reply to Chris Dumez from comment #6)
> > > (In reply to Alex Christensen from comment #3)
> > > > Comment on attachment 366128 [details]
> > > > Patch
> > > > 
> > > > Can we not make a unit test using _killWebContentProcessAndResetState ?
> > > 
> > > What do I observe? I do not know what accessibility events are :)
> > 
> > Chris F, can you make a recommendation here?
> 
> Note that for testing, we would also need an SPI to actually set the state
> to enabled or disabled. Currently, there is only SPI to ask to update the
> state and it relies on _AXSWebAccessibilityEventsEnabled() internally, the
> state is not passed by the client.


Here's the setter

extern void _AXSSetWebAccessibilityEventsEnabled(Boolean enabled);


and then you can observe kAXSWebAccessibilityEventsEnabledNotification on the local port
Comment 10 Chris Dumez 2019-03-28 13:32:48 PDT
(In reply to chris fleizach from comment #9)
> (In reply to Chris Dumez from comment #8)
> > (In reply to Geoffrey Garen from comment #7)
> > > (In reply to Chris Dumez from comment #6)
> > > > (In reply to Alex Christensen from comment #3)
> > > > > Comment on attachment 366128 [details]
> > > > > Patch
> > > > > 
> > > > > Can we not make a unit test using _killWebContentProcessAndResetState ?
> > > > 
> > > > What do I observe? I do not know what accessibility events are :)
> > > 
> > > Chris F, can you make a recommendation here?
> > 
> > Note that for testing, we would also need an SPI to actually set the state
> > to enabled or disabled. Currently, there is only SPI to ask to update the
> > state and it relies on _AXSWebAccessibilityEventsEnabled() internally, the
> > state is not passed by the client.
> 
> 
> Here's the setter
> 
> extern void _AXSSetWebAccessibilityEventsEnabled(Boolean enabled);
> 
> 
> and then you can observe kAXSWebAccessibilityEventsEnabledNotification on
> the local port

Ok, this helps with turning the setting on and off from an API test. I am still unclear how I can write an API test for this though. I found the following related layout test:
LayoutTests/accessibility/ios-simulator/accessibility-events-setting.html

Seems like I can set a onaccessibleclick event handler on a button and observe if it gets called upon click or not. So far so good. However, it does not seem to get called for regular clicks.
It seems it only works for "accessible clicks". How do I do an accessible click from my API test? It seems like the layout test is currently relying on some WebKitTestRunner infra to trigger such a click.
Comment 11 Geoffrey Garen 2020-10-12 14:12:37 PDT
Comment on attachment 366128 [details]
Patch

r=me