796447 - [Settings] Mobile Data settings should never be on in Airplane Mode.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P3)

Description

[GH to BZ]

[GitHub issue by evelynhung on 2012-07-30T23:35:32Z, https://github.com/mozilla-b2g/gaia/issues/2985]
(refer issue #2212)
Although Wifi and bluetooth could be turned on in airplane mode, mobile data is always off in backend implementation. However, in Settings app and quick settings, it still could be clicked on, but it actually won't work as user expected. Need UX review on this.

Comment 1

[GH to BZ]

[GitHub comment by evelynhung on 2012-07-30T23:39:03Z]
I think the button should be greyout in airplane mode

Comment 2

[GH to BZ]

[GitHub comment by jcarpenter on 2012-07-31T02:43:38Z]
Agreed. Airplane mode should prevent data transmission. We need to communicate to the user that Settings > Network & Connectivity > Cellular & Data > Data Connection toggle is off and cannot be activated when Airplane mode is On.

@lco

Comment 3

[GH to BZ]

[GitHub comment by lco on 2012-08-01T20:31:55Z]
Fine, we should grey out the data switch if the user is in airplane mode. cc @fabi1cazenave

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Do we have a *gray out* state in our template?

Comment 5

[Larissa Co [:lco]]

Adding Patryk to check on the grey out state. We've done this for a few things in Settings, but I don't know if this was an official visual design. 

Patryk, we'd need a greyed out switch and maybe a color spec for text that's greyed out.

Comment 6

[Patryk Adamczyk [:patryk] UX]

No we don't have a gray out state currently, but I'll get Peter to add one.

Comment 7

[Patryk Adamczyk [:patryk] UX]

(In reply to Patryk Adamczyk [:patryk] UX from comment #6)
> No we don't have a gray out state currently, but I'll get Peter to add one.

Or add one myself on Monday.

Comment 8

[Dietrich Ayala (:dietrich)]

There's been no activity here for 7 weeks. Evelyn, if you're not planning on getting to it soon, can you re-assign to someone else, or un-assign yourself so someone else can take the bug?

Comment 9

[Evelyn Hung]

okay, un-assign myself first!

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch

With this patch I remove the data connectivity if the airplane mode is enabled.

Comment 11

[Evelyn Hung]

Comment on attachment 686634 [details] [diff] [review]
patch

>diff --git a/apps/settings/index.html b/apps/settings/index.html
>index bf3a496..177e33e 100644
>--- a/apps/settings/index.html
>+++ b/apps/settings/index.html
>@@ -487,7 +487,7 @@
>         <h2 data-l10n-id="dataConnectivity"> Data Connectivity </h2>
>       </header>
>       <ul>
>-        <li>
>+        <li id="data-connectivity">
>           <label>
>             <input type="checkbox" data-type="switch" name="ril.data.enabled"/>
>             <span></span>
>diff --git a/apps/settings/js/carrier.js b/apps/settings/js/carrier.js
>index d3b0e6e..99a04e6 100644
>--- a/apps/settings/js/carrier.js
>+++ b/apps/settings/js/carrier.js
>@@ -176,6 +176,26 @@ var Carrier = (function newCarrier(window, document, undefined) {
>     document.getElementById('dataNetwork-desc').textContent = name;
>   }
> 
>+  function updateDataConnectivity() {
>+    var key = 'ril.radio.disabled';
>+
>+    var panel = document.querySelector('#data-connectivity');
nit: 'panel' is a little misleading, it's an element/item. please rename it.
>+    if (!panel)
>+      return;
>+
>+    var settings = Settings.mozSettings;
>+    if (!settings)
>+      return;
>+
>+    var req = settings.createLock().get(key);
>+    req.onsuccess = function() {
>+      panel.hidden = req.result[key];
per comment 3, we intend to *grey-out* this setting but not hide it.
I'm not sure what our visual Patryk provide for grey-out an element, you may need to ask him.
>+    };
>+    settings.addObserver(key, function(evt) {
>+      panel.hidden = evt.settingValue;
same above
>+    });
>+  }
>+
>   // 2G|3G network selection
>   document.getElementById('preferredNetworkType').onchange =
>     restartDataConnection;
>@@ -333,6 +353,7 @@ var Carrier = (function newCarrier(window, document, undefined) {
>     // startup
>     init: function carrier_init() {
>       gMobileConnection.addEventListener('datachange', updateConnection);
>+      updateDataConnectivity();
>       updateConnection();
>       updateSelectionMode();
>       this.fillAPNList(); // XXX this should be done later -- not during init()

r-, sorry, please try to grey-out the setting, do not remove it in airplane mode, it may confuse our users.

Comment 12

[Evelyn Hung]

Hi Patryk, could you comment for where to find a grey-out visual or example? Thanks!

Comment 13

[Andrea Marchesini [:baku]]

> r-, sorry, please try to grey-out the setting, do not remove it in airplane
> mode, it may confuse our users.

Sure. I need a CSS class name or an example.

Comment 14

[Patryk Adamczyk [:patryk] UX]

Attachment: Spec

In the settings app.
Please just apply:
1. 60% opacity to the icons.
2. Text labels should be #797e80
3. Wifi seems to have a "Disabled" label underneath the "Wi-Fi" section label, please remove it, its inconsistent with the other 3 apps.
4. The toggle switch in the GPS row, should be at 50% opacity.

In the notification tray. Please make the "off" state of the icons 50% opacity. Not that we are updating the icons with bug 803449, should be in this week.

Comment 15

[Patryk Adamczyk [:patryk] UX]

Attachment: Spec (Updated)

In the settings app.
Please just apply:
1. 60% opacity to the icons.
2. Text labels should be #797e80
3. Wifi seems to have a "Disabled" label underneath the "Wi-Fi" section label, please replace it with "Off" and don't disable that row. Airplane mode should just force wifi off.
4. The toggle switch in the GPS row, should be at 50% opacity.

In the notification tray. Please make the "off" state of the icons 50% opacity. Not that we are updating the icons with bug 803449, should be in this week.

Comment 16

[Andrea Marchesini [:baku]]

Wifi, GPS and Bluetooth should not have opacity. Just data connectivity. is it?

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch b

Comment 18

[Larissa Co [:lco]]

(In reply to Andrea Marchesini (:baku) from comment #16)
> Wifi, GPS and Bluetooth should not have opacity. Just data connectivity. is
> it?

I didn't think  you should be able turn on GPS and Bluetooth on the plane. Please correct me if I'm wrong though.

Comment 19

[Andrea Marchesini [:baku]]

you should not... or you should not be able? :)
I think technically it's possible to use GPS/Bluetooth on a plane.

Comment 20

[Evelyn Hung]

Comment on attachment 689229 [details] [diff] [review]
patch b

r=me. 
Yes, I agree we should only disable data settings here.

This patch resolved Settings app part, but lost System app part.
There is a data setting control in Quick Setting of System app. I think we need another patch to resolve the problem completely.
I'd like to r+ this patch first, and wait your second patch for System app.

Thanks for your great work!

Comment 21

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/6885 pull request

Comment 22

[Andrea Marchesini [:baku]]

Attachment: quick settings

Comment 23

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/6919 this is for the first patch.

Comment 24

[Evelyn Hung]

Comment on attachment 689700 [details] [diff] [review]
quick settings

I'd like to pass the review request to Tim. :)

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 689700 [details] [diff] [review]
quick settings

r=me with nit fixed

>diff --git a/apps/system/js/quick_settings.js b/apps/system/js/quick_settings.js
>index f3e6538..fd696e6 100644
>--- a/apps/system/js/quick_settings.js
>+++ b/apps/system/js/quick_settings.js
>@@ -19,6 +19,16 @@ var QuickSettings = {
> 
>     var self = this;
> 
>+    // nonitor airplane status

// Disable data button if airplane mode is enabled

>+    SettingsListener.observe('ril.radio.disabled', true, function(value) {
>+      self.data.dataset.airplaneMode = value;
>+      if (value) {
>+        self.data.classList.add('quick-settings-airplane-mode');
>+      } else {
>+        self.data.classList.remove('quick-settings-airplane-mode');
>+      }
>+    });
>+
>     // monitor data status
>     SettingsListener.observe('ril.data.enabled', true, function(value) {
>       self.data.dataset.enabled = value;
>@@ -68,16 +78,17 @@ var QuickSettings = {
>             break;
> 
>           case this.data:
>-            var enabled = (this.data.dataset.enabled == 'true');
>-            // the actual mozSettings request is async,
>-            // but we want to be responsive to user input
>-            // and double click so we'll change the UI state here
>-            this.data.dataset.enabled = !enabled;
>-
>-            SettingsListener.getSettingsLock().set({
>-              'ril.data.enabled': !enabled
>-            });
>-
>+            if (this.data.dataset.airplaneMode != 'true') {

if (this.data.dataset.airplaneMode !== 'true')
  return;

>+              var enabled = (this.data.dataset.enabled == 'true');
>+              // the actual mozSettings request is async,
>+              // but we want to be responsive to user input
>+              // and double click so we'll change the UI state here
>+              this.data.dataset.enabled = !enabled;
>+
>+              SettingsListener.getSettingsLock().set({
>+                'ril.data.enabled': !enabled
>+              });
>+            }
>             break;
> 
>           case this.bluetooth:
>diff --git a/apps/system/style/quick_settings/quick_settings.css b/apps/system/style/quick_settings/quick_settings.css
>index 4178595..783488d 100644
>--- a/apps/system/style/quick_settings/quick_settings.css
>+++ b/apps/system/style/quick_settings/quick_settings.css
>@@ -70,3 +70,6 @@
> #quick-settings > #quick-settings-full-app {
>   background-image: url(images/settings.svg);
> }
>+.quick-settings-airplane-mode {
>+  opacity: 0.5;
>+}

Comment 26

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/6919

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/commit/4f35e5e00656aa7f79eac8ad7243f128117dde58

Merged.

Comment 28

[Darren]

Verified as fixed in build 201301020070202 on Unagi.

Comment 29

[Darren]

Build was 20130102070202 on Unagi

Had one to many 0's