1042196 - Provide a wifi toggle widget on error pages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

See bug 1042194 for the framework piece here. Among the first widgets I wanted to add was a wifi toggle.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This implements this using the new infrastructure. I added a little "in-progress" animation when the button is clicked as it can take awhile. Once we hear back from Java that things are connected, we auto-refresh the page.

This just uses a button as opposed to the slider from https://bug1042199.bugzilla.mozilla.org/attachment.cgi?id=8460516 . I'm torn on whether I like that or not. The slider was neat, but made me want to flip it back and forth. I don't think we need to support turning off wifi on these pages. Will attach a screenshot. Curious what UX thinks?

Build at: http://people.mozilla.org/~wjohnston/error.apk
Animation test page at: http://people.mozilla.org/~wjohnston/wifi.html

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think this has the potential to be very useful to users and that could go a long way especially in these situations of frustration. 

For the time being I don't have much design feedback to give other than maybe sorting out the padding to balance it out a bit.

Let's try having equal padding above and beneath the copy for "1. and 2." and then using the same margin between the 2 buttons.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This is simplified somewhat to do its listening for online events entirely in JS. No more timer in Java. Instead, we'll keep "loading" in JS until we get an online event, then we'll reload the page.

It can happen that the wifi indicator turns on a little while before we're really online. I tried just polling the network status instead, and it does say the link is up at that point, but trying to use the network will fail. I wonder if maybe the button should change states more frequently to indicate an ongoing process. i.e. we could change the text as it goes "Connecting..." "Finding networks", etc. Or we could show more of a progress bar...

Comment 5

[:Margaret Leibovic]

Comment on attachment 8468902 [details] [diff] [review]
Patch v2

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

::: mobile/android/modules/NetErrorHelper.jsm
@@ +76,5 @@
> +        }
> +      }
> +  },
> +
> +  handleEvent: function(event) {

Drive-by: Maybe we should change this API to just pass the node to the handler, rather than the event.

@@ +84,5 @@
> +    }
> +
> +    if (!node) {
> +      return;
> +    }

Something would be broken if this is ever the case.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8468902 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/base/GeckoNetworkManager.java b/mobile/android/base/GeckoNetworkManager.java

>+    public void handleMessage(final String event, final NativeJSObject message,
>+                              final EventCallback callback) {
>+        if (event.equals("Wifi:Enable")) {
>+            final WifiManager mgr = (WifiManager) mApplicationContext.getSystemService(Context.WIFI_SERVICE);
>+            final Timer timer = new Timer();

What does the timer do?

>diff --git a/mobile/android/modules/NetErrorHelper.jsm b/mobile/android/modules/NetErrorHelper.jsm

>+  onPageShown: function(browser) {

>+        var nodes = browser.contentDocument.querySelectorAll("#wifi");

let

>+    // Show indeterminate progress while we wait for the network.
>+    node.disabled = true;
>+    node.classList.add("thinking");

"thinking" ?

>+    node.ownerDocument.addEventListener('online', this.onOnline.bind(this, Cu.getWeakReference(node)));

"online"

>diff --git a/mobile/android/themes/core/netError.css b/mobile/android/themes/core/netError.css
>diff --git a/mobile/locales/en-US/overrides/netError.dtd b/mobile/locales/en-US/overrides/netError.dtd

>+  <li>If you are unable to load any pages, check your device's data or Wi-Fi connection.
>+    <button id='wifi'>Enable wifi</button>

Looks like we use "Wi-Fi" not "wifi"

>+    <button id='wifi'>Enable wifi</button>

Same

>+<!ENTITY proxyResolveFailure.longDesc3 "

>+    <button id='wifi'>Enable wifi</button>

Same

>+  <li>If you are unable to load any pages, check your mobile device's data or Wi-Fi connection.
>+    <button id='wifi'>Enable wifi</button>

Same

Lastly, I think we should add some telemetry to this. Probably in the JS so we know the exact reason for the WiFi toggle.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated. I switched this over to use "network:link-status-changed" Observer notifications since they seem to fire around the same time that the wifi indicator appears on your phone. Still, reloading at that point fails sometimes, but I think the phone showing WiFi is working and then nothing happening for 5 seconds is a worse experience on our end. It looks like we're just frozen.

I also did some style updates to try and create a more uniform spacing between things vertically throughout the error pages. I'll post some shots of the wifi toggle. This affects other error pages as well though. I went through pretty carefully to make sure they still look fine, but there's some risk there.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Updated screenshot. There's also a build with this at:
http://people.mozilla.com/~wjohnston/wifi.apk

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8470993 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/GeckoNetworkManager.java b/mobile/android/base/GeckoNetworkManager.java

>+    public void handleMessage(final String event, final NativeJSObject message,
>+                              final EventCallback callback) {
>+        if (event.equals("Wifi:Enable")) {

I'm fine with it, but "Wifi:Enable" might be a bit vague since the code is more like "Wifi:TryYourHardestToConnect"

>+            final WifiManager mgr = (WifiManager) mApplicationContext.getSystemService(Context.WIFI_SERVICE);
>+
>+            if (!mgr.isWifiEnabled()) {
>+                mgr.setWifiEnabled(message.getBoolean("enabled"));

Do we need the boolean? You can't use the boolean to disable WiFi.

>+                // If Wifi is enabled, maybe you need to select a network
>+                Intent intent = new Intent(android.provider.Settings.ACTION_WIFI_SETTINGS);
>+                intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
>+                mApplicationContext.startActivity(intent);

Someday, I'd like to add something to check for "connected, but not sending data" - but that is a new bug.

>diff --git a/mobile/android/modules/NetErrorHelper.jsm b/mobile/android/modules/NetErrorHelper.jsm
>+handlers.wifi = {
>+  onPageShown: function(browser) {
>+      // If we have a connection, don't bother showing the wifi toggle.
>+      let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
>+      if (network.isLinkUp && network.linkStatusKnown) {
>+        let nodes = browser.contentDocument.querySelectorAll("#wifi");
>+        for (let i = 0; i < nodes.length; i++) {
>+          nodes[i].style.display = "none";
>+        }
>+      }

I wonder if we should invert this. We could hide the wifi stuff and only show it if needed. I suppose it's fine for now. If we notice any "flashes of wifi" we can address it.

>+  handleClick: function(event) {

>+    UITelemetry.addEvent("wifitoggle.1", "neterror", null);

I want to change this up a bit. I think we will be adding more actions on neterror, so let's make it like this:

      UITelemetry.addEvent("neterror.1", "button", "wifitoggle");

>+    this.node = Cu.getWeakReference(node);
>+    Services.obs.addObserver(this, "network:link-status-changed", false);

Should we "weak ref" this observer? In case the page closes without removing it?

>+  observe: function(subject, topic, data) {
>+    let node = this.node.get();
>+    if (!node) {
>+      return;
>+    }

You should remove the observer though, right? Maybe do it right away regardless of what happens?

>+    if (network.isLinkUp && network.linkStatusKnown) {
>+      // If everything worked, reload the page
>+      UITelemetry.addEvent("wifitoggle.reload.1", "neterror", null);

        UITelemetry.addEvent("neterror.1", null, "wifitoggle.reload");

>+      Services.obs.removeObserver(this, "network:link-status-changed");

As above, maybe we should remove this right away. And also make it a weakref?

>+      // Even at this point, Android sometimes lies about the real state of the network and this reload request fails.
>+      // Add a 500ms delay before refreshing the page.
>+      node.ownerDocument.defaultView.setTimeout(function() {
>+        node.ownerDocument.location.reload(false);

Is there a chance of racing the page closing? I could close the page and 'node' would be bogus. Maybe you should get the node again and check for null?

>diff --git a/mobile/android/themes/core/netError.css b/mobile/android/themes/core/netError.css

>+  --moz-vertical-spacing: 10px;
>+  --moz-background-height: 32px;

Are CSS variables OK to use?

r- to see a new patch and give you a chance to answer some questions

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Do we need the boolean? You can't use the boolean to disable WiFi.
No. When this looked like a "toggle" it made more sense. Removed.

> Should we "weak ref" this observer? In case the page closes without removing
> it?
Seems like a good idea to me.

> You should remove the observer though, right? Maybe do it right away
> regardless of what happens?
I was worried we'd get a network-link-status event for some other reason here. It seems smart to wait until we have the one we want, so I left this in.

> Is there a chance of racing the page closing? I could close the page and
> 'node' would be bogus. Maybe you should get the node again and check for
> null?
I'm not sure I understand. We already have the node here since we called get on the WeakRef. Its closured into this function, so there's really no point in getting it again?

> Are CSS variables OK to use?
Should be. This stylesheet isn't applied to normal pages. Just error pages, so there's no leakage of data.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #10)

> > Is there a chance of racing the page closing? I could close the page and
> > 'node' would be bogus. Maybe you should get the node again and check for
> > null?
> I'm not sure I understand. We already have the node here since we called get
> on the WeakRef. Its closured into this function, so there's really no point
> in getting it again?

You get the node, then do a setTimeout(..., 500) so there are 500 milliseconds before you use node again, inside the closure. If the page closes in that 500 milliseconds you might have a problem.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - I guess you could argue that since the setTimeout is tied to the document, if the document is killed/closed, then the setTimeout should be harmless.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8475428 [details] [diff] [review]
Patch

>diff --git a/mobile/android/modules/NetErrorHelper.jsm b/mobile/android/modules/NetErrorHelper.jsm

>+  handleClick: function(event) {

>+    UITelemetry.addEvent("neterror.1", "button", "wifitoggle");
>+    // Show indeterminate progress while we wait for the network.

nit: Add a blank line before the comment

Comment 14

[Wesley Johnston (:wesj)]

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

Comment 15

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> Wes - I guess you could argue that since the setTimeout is tied to the
> document, if the document is killed/closed, then the setTimeout should be
> harmless.

Yeah, as near as I can tell, all timers are cancelled when the page is closed or changed. That said, we have a strong reference to the node at this point, so I'm not worried about not having it when this closes. You did make me a bit worried that if you navigated to a new page, we'd redirect to it. But as you said, the timer is cancelled in that case so it never actually fires.

Comment 16

[Wesley Johnston (:wesj)]

[Tracking Requested - why for this release]:

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Francesco Lodolo [:flod]]

If I'm correct, besides being wrong, this is going to create a nice yellow screen of death in localized builds

    5.12 -<!ENTITY connectionFailure.longDesc "&sharedLongDesc2;">
    5.13 +<!ENTITY connectionFailure.longDesc "&sharedLongDesc3;">

Sorry but I'll ask a back-out, better safe than sorry.

Comment 19

[Francesco Lodolo [:flod]]

'besides being wrong" was referring to the patch not changing the string ID.

So we're going to fallback to en-US for sharedLongDesc3, but then we have an existing string connectionFailure.longDesc that references a non existing entity sharedLongDesc2 if locales don't notice the change.

Comment 20

[Nigel Babu [:nigelb]]

Backed out in https://hg.mozilla.org/mozilla-central/rev/0b9dd32d1e16 on flod's suggestion.

Comment 21

[Francesco Lodolo [:flod]]

Note that there are actually a lot more of them

http://hg.mozilla.org/mozilla-central/diff/d53b90e407ce/mobile/locales/en-US/overrides/netError.dtd

    1.12 -<!ENTITY connectionFailure.longDesc "&sharedLongDesc2;">
    1.13 +<!ENTITY connectionFailure.longDesc "&sharedLongDesc3;">

    1.46 -<!ENTITY netInterrupt.longDesc "&sharedLongDesc2;">
    1.47 +<!ENTITY netInterrupt.longDesc "&sharedLongDesc3;">

    1.77 -<!ENTITY netReset.longDesc "&sharedLongDesc2;">
    1.78 +<!ENTITY netReset.longDesc "&sharedLongDesc3;">

    1.81 -<!ENTITY netTimeout.longDesc "&sharedLongDesc2;">
    1.82 +<!ENTITY netTimeout.longDesc "&sharedLongDesc3;">

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

While reviewing this, I did not suggest changing the "connectionFailure.longDesc" (and friends) entity because they themselves do not hold a string. They merely point to another entity and that entity was "bumped".

Comment 23

[Francesco Lodolo [:flod]]

It doesn't contain a string but a reference to another string, which is kind of worse, especially for a .DTD

This is the scenario if a localizer doesn't catch up with the change: it will ship with something into sharedLongDesc3, either the updated translation or a fallback to English, but it will have a broken file because connectionFailure.longDesc & C. don't have anything to fall back to.

Comment 24

[Wesley Johnston (:wesj)]

So the issue is locales will pick up the correct sharedLongDesc3 but not update connectionFailure.longDesc (or some other entity that points to sharedLongDesc2)? Do locales actually change connectionFailure.longDesc (I don't think they should be localizing that... they should just be picking up the en-US version...)

How do you recommend we fix it? The locales system is awful enough that I'll need some guidance. I don't think we can settle for "never change the text on error pages".

Comment 25

[Francesco Lodolo [:flod]]

At build time, missing strings in a localization will be replaced by English strings: that ensures that a build always has all the strings it needs.

sharedLongDesc3 will be missing in the localization file, so it will be either correctly localized or in English.

connectionFailure.longDesc already exists, tools won't report it as missing, so localizers won't notice that the content has been updated unless the tool they're using has that feature.

To fix this, you simply need to "rev" those entities too.
<!ENTITY connectionFailure.longDesc2 "&sharedLongDesc3;">
etc.

More details about string changes
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Follow up

This revs the ids.

Comment 27

[Wesley Johnston (:wesj)]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f00edcf2962

Comment 29

[Flaviu Cos]

Verified as fixed in builds:
- 35.0a1 (2014-09-05);
- 34.0a2 (2014-09-05);
Device: Google Nexus 7 (Android 4.4.4).

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Release noted as "New: Toggle wifi on error pages".

Comment 31

[Stefan Deiac]

Based on comment 29 I will remove the qe-verify flag, thanks.