Bug 196326 – [WK2] AccessibilityEventsEnabled setting does not persist after a WebProcess crash

• Component: WebKit2
• Status: ASSIGNED

[Chris Dumez]

AccessibilityEventsEnabled setting does not persist after a WebProcess crash.

[Chris Dumez]

Created attachment 366128 [details]
Patch

[Chris Dumez]

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

[Alex Christensen]

Comment on attachment 366128 [details]
Patch

Can we not make a unit test using _killWebContentProcessAndResetState ?

[chris fleizach]

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.

[Chris Dumez]

(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

[Chris Dumez]

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

[Geoffrey Garen]

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

[Chris Dumez]

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

[chris fleizach]

(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

[Chris Dumez]

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

[Geoffrey Garen]

Comment on attachment 366128 [details]
Patch

r=me