Provide a wifi toggle widget on error pages

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
11 months ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Firefox 34
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap ?

Firefox Tracking Flags

(firefox34+ verified, firefox35 verified, relnote-firefox 34+)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Posted patch WIP Patch (obsolete) — Splinter Review
See bug 1042194 for the framework piece here. Among the first widgets I wanted to add was a wifi toggle.
Depends on: 1042194
Blocks: 940453
Attachment #8460374 - Attachment is patch: true
Posted patch Patch (obsolete) — Splinter Review
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
Attachment #8460374 - Attachment is obsolete: true
Attachment #8466513 - Flags: review?(liuche)
Flags: needinfo?(alam)
Posted image Screenshot (obsolete) —
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.
Flags: needinfo?(alam)
Posted patch Patch v2 (obsolete) — Splinter Review
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...
Attachment #8466513 - Attachment is obsolete: true
Attachment #8466513 - Flags: review?(liuche)
Attachment #8468902 - Flags: review?(liuche)
Assignee: nobody → wjohnston
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 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.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8468902 - Attachment is obsolete: true
Attachment #8468902 - Flags: review?(liuche)
Attachment #8470993 - Flags: review?(liuche)
Posted image Screenshot
Updated screenshot. There's also a build with this at:
http://people.mozilla.com/~wjohnston/wifi.apk
Attachment #8466520 - Attachment is obsolete: true
Attachment #8470993 - Flags: review?(liuche) → review?(mark.finkle)
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
Attachment #8470993 - Flags: review?(mark.finkle) → review-
Flags: in-moztrap?(fennec)
Posted patch PatchSplinter Review
(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.
Attachment #8470993 - Attachment is obsolete: true
Attachment #8475428 - Flags: review?(mark.finkle)
(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.
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.
Flags: needinfo?(wjohnston)
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
Attachment #8475428 - Flags: review?(mark.finkle) → review+
(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.
[Tracking Requested - why for this release]:
https://hg.mozilla.org/mozilla-central/rev/d53b90e407ce
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(wjohnston)
Resolution: FIXED → ---
'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.
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;">
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".
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.
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".
Flags: needinfo?(wjohnston)
Flags: needinfo?(francesco.lodolo)
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
Flags: needinfo?(francesco.lodolo)
Posted patch Follow upSplinter Review
This revs the ids.
Attachment #8477532 - Flags: review?(mark.finkle)
Attachment #8477532 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/8f00edcf2962
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Flags: qe-verify+
OS: Linux → Android
Hardware: x86_64 → ARM
Version: unspecified → Trunk
Depends on: 1059096
Depends on: 1061030
Verified as fixed in builds:
- 35.0a1 (2014-09-05);
- 34.0a2 (2014-09-05);
Device: Google Nexus 7 (Android 4.4.4).
Status: RESOLVED → VERIFIED
Depends on: 1070624
Depends on: 1071740
Release noted as "New: Toggle wifi on error pages".
Depends on: 1088952
Depends on: 1083213
No longer depends on: 1083213
Based on comment 29 I will remove the qe-verify flag, thanks.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.