1164640 - [e10s] Reset the a11y disabled flag

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jim Mathies [:jimm]]

Lets get our first prompt on aurora going. In this work we should also reset the a11y disabled flag on both mc and aurora.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

The prompt was already enabled on aurora in bug 1161260 (see 3rd patch), and users should see it when aurora updates to 40 (it hasn't been published yet afaict).

So the only thing to do is to reset the a11y flag on m-c. No need to do it on Aurora because approximately no one should have seen it on aurora as e10s was not supported before.

Comment 2

[Jim Mathies [:jimm]]

Ah great, thanks felipe! Morphing.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: New a11y flag

This does a few things:

- rename the flag, in practice making it reset the value for all users
- fixes a typo I introduced in bug 1161260. s/UpdateServices/Services/
- improves the handling of this flag when we decide to show the warning: Previously, we were treating both "absense of pref" and "pref set to false" as the same thing, so there was no way for someone to prevent the warning from keeping showing up if they want to remain with e10s enabled. We offer an option for "Don't disable" on the popup, so effectively we were ignoring it.

I mentioned in comment 1 that it wouldn't be necessary to uplift this, but with the typo fix and the improved handling I think this will be a good candidate for Aurora as well.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8607163 [details] [diff] [review]
New a11y flag

Review of attachment 8607163 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with an optional suggestion.

::: browser/components/nsBrowserGlue.js
@@ +2817,5 @@
>        let updateChannel = UpdateChannel.get();
>        let channelAuthorized = updateChannel == "nightly" || updateChannel == "aurora";
>  
>        skipE10sChecks = !channelAuthorized ||
> +                       Services.prefs.getBoolPref("browser.tabs.remote.disabled-for-a11y");

Yikes! Yes, let's get that uplifted.

@@ +2995,5 @@
>  
>    _showE10sAccessibilityWarning: function() {
> +    let prefExistsAndIsFalse = false;
> +    try {
> +      prefExistsAndIsFalse = Services.prefs.getBoolPref("browser.tabs.remote.disabled-for-a11y") == false;

We only ever use prefExistsAndIsFalse up here, so can't we just keep it in the scope of the try? We also get to avoid this kinda weird composite boolean thing at the same time:

try {
  if (Services.prefs.getBoolPref("browser.tabs.remote.disabled-for-a11y")) {
    // We should have thrown if the pref did not exist
    return;
  }
} catch(e) {
  // The pref did not exist yet, so we'll continue.
}

Comment 5

[Pulsebot]

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

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8607163 [details] [diff] [review]
New a11y flag

Review of attachment 8607163 [details] [diff] [review]:
-----------------------------------------------------------------

what happens to the old pref?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8607163 [details] [diff] [review]
New a11y flag

Approval Request Comment
[Feature/regressing bug #]: allow aurora users with non-blacklisted a11y tools to enable e10s.
[User impact if declined]: 
[Describe test coverage new/current, TreeHerder]: Code is active on Nightly
[Risks and why]: 
[String/UUID change made/needed]: none

Comment 9

[Sylvestre Ledru [:Sylvestre]]

(In reply to :Felipe Gomes from comment #8)
> [User impact if declined]: 
> [Risks and why]: 
Could you fill these two items? Thanks

Comment 10

[:Felipe Gomes (needinfo for replies!)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> (In reply to :Felipe Gomes from comment #8)

> > [User impact if declined]: This has a typo fix that would make us offer to turn e10s on even for users who have a blacklisted accessibility tool.

> > [Risks and why]: This is running in central, I think it is fairly safe. The risk would be contained to the offer popups of e10s.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34cc0e31bdb6