1260190 - Disable e10s for accessibility users on OS X

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Similarly to how we do it for Windows, we should block e10s for users currently running an accessibility tool on OS X.

I was told that right now it's believed that there are few users of accessibility on Mac, due to our support being poor there. However, a quick test of VoiceOver shows that it is able to read out the page's text content and links, which is better than nothing. And with e10s on, the webpage's content is entirely skipped.

So we should not leave users lost as to why is that the case, and just disable e10s on Mac too. Since there are very few users, this in practice won't affect many people anyway. And we can remove this limitation when we're more satisfied with how it's working on Mac.

(I believe this should just be a matter of removing a couple of ifdefs)

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1260190 - Disable e10s for accessibility users on OS X. r=jimm

Review commit: https://reviewboard.mozilla.org/r/43241/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43241/

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 8736374 [details]
MozReview Request: Bug 1260190 - Disable e10s for accessibility users on OS X. r=jimm

https://reviewboard.mozilla.org/r/43241/#review40057

::: widget/nsBaseWidget.cpp:1879
(Diff revision 1)
>    // make sure it's not created at unsafe times.
>    nsCOMPtr<nsIAccessibilityService> accService =
>      services::GetAccessibilityService();
>    if (accService) {
> -#ifdef XP_WIN
> +#if defined(XP_WIN) || defined(XP_MACOSX)
>      mAccessibilityInUseFlag = true;

While you're in here, would you mind adding a bit of code here that sets the pref the first time we set mAccessibilityInUseFlag? something like - 

if (!mAccessibilityInUseFlag) {
  update kAccessibilityLastRunDatePref
  mAccessibilityInUseFlag = true;
}

Comment 3

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/43241/#review40057

> While you're in here, would you mind adding a bit of code here that sets the pref the first time we set mAccessibilityInUseFlag? something like - 
> 
> if (!mAccessibilityInUseFlag) {
>   update kAccessibilityLastRunDatePref
>   mAccessibilityInUseFlag = true;
> }

I've done this. I believe this means we can remove the code that sets this on Shutdown(). Do you agree? I've done it and I'm posting the patch again for you to take a look

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8736374 [details]
MozReview Request: Bug 1260190 - Disable e10s for accessibility users on OS X. r=jimm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43241/diff/1-2/

Comment 5

[Jim Mathies [:jimm]]

https://reviewboard.mozilla.org/r/43241/#review40741

::: widget/nsBaseWidget.cpp:1876
(Diff revision 2)
>    if (accService) {
> -#ifdef XP_WIN
> +#if defined(XP_WIN) || defined(XP_MACOSX)
> +    if (!mAccessibilityInUseFlag) {
> -    mAccessibilityInUseFlag = true;
> +      mAccessibilityInUseFlag = true;
> +      uint32_t now = PRTimeToSeconds(PR_Now());
> +      Preferences::SetInt(kAccessibilityLastRunDatePref, now);

This adds a slight risk of the timestamp becoming out of date for really long running sessions. You could put this code in a helper and call it twice, once here where we flip the flag and once when we shut down if the flag it set.

might be total overkill though. :)

r+ either way.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Yeah, it's a bit overkill, so I'll go with just setting it on a11y initialization. From one session to the next it's guaranteed that it will block due to the loadedInLastSession pref. And the timeout for the lastRanDate pref is one week, so a difference for a long session is unlikely to matter. And it's a bit less work to do on shutdown :)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1733bd822f0f

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8736374 [details]
MozReview Request: Bug 1260190 - Disable e10s for accessibility users on OS X. r=jimm

Approval Request Comment
[Feature/regressing bug #]: e10s accessibility filtering for mac
[User impact if declined]: e10s won't get disabled for users using accessibility features on mac
[Describe test coverage new/current, TreeHerder]: code is already been used exclusively on windows, this enables the same code for mac
[Risks and why]: contained to the e10s-filtering code
[String/UUID change made/needed]: none

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d40c605a68b2

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1733bd822f0f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d40c605a68b2

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8736374 [details]
MozReview Request: Bug 1260190 - Disable e10s for accessibility users on OS X. r=jimm

This should help e10s experiments planned for Beta 47, taking it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54b32c56b38f