Disable e10s for accessibility users on OS X

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

47 Branch
mozilla48
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm9+, firefox46 wontfix, firefox47 fixed, firefox48 fixed)

Details

Attachments

(1 attachment)

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)
Assignee: nobody → felipc
tracking-e10s: --- → +
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;
}
Attachment #8736374 - Flags: review?(jmathies) → review+
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 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/
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.
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 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
Attachment #8736374 - Flags: approval-mozilla-aurora?

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1733bd822f0f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Attachment #8736374 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
Depends on: 1327380
You need to log in before you can comment on or make changes to this bug.