1026306 - when mobile data isn't connected, wifi hotspot setting silently fails and disables itself

Status: VERIFIED FIXED

Description

[Dietrich Ayala (:dietrich)]

master + m-c, June 16, Nexus 4

STR:

1. turn off mobile data
2. turn on wifi hotspot

Expected: alert saying there's no mobile data connection

Actual: wait a couple of seconds and the toggle flips back to disabled

Comment 1

[Manuel Casas Barrado [:mancas]]

We need more info from UX. What's your thought Jenny?

Comment 2

[Jenny Lee]

I see we already implemented showing dialog for when there's no SIM card under the same situation described in comment 1. see https://bugzilla.mozilla.org/show_bug.cgi?id=962398#c13

We can do the same thing here, just need to modify the string:

     Wi-Fi hotspot is disabled because there is no data connection. 
 
     [Ok]

For USB tethering, show:

     USB tethering is disabled because there is no data connection. 
 
     [Ok]

Thanks!

Comment 3

[Manuel Casas Barrado [:mancas]]

Created attachment 8493023 [details] [review]
Prompt user when try to enable hotspot

Comment 4

[Manuel Casas Barrado [:mancas]]

Created attachment 8493024 [details]
usb-tethering.png

Comment 5

[Manuel Casas Barrado [:mancas]]

Created attachment 8493025 [details]
wifi-tethering.png

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8493023 [details] [review]
Prompt user when try to enable hotspot

I never really touch this code. Let's find a more appropriate reviewer.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8493023 [details] [review]
Prompt user when try to enable hotspot

Sorry, my version had been refactored and I am not a peer of system app. It would be nice to be reviewed by Kevin since he is a peer and he refactored the code.

Please review this, Kevin and thanks :).

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8493023 [details] [review]
Prompt user when try to enable hotspot

I will try to give this a review, but would still appreciate your feedback John.

Manuel - I have one question about the dataType on github, and I was also wondering if you could try crafting a unit test for this? I think it would be good to check that we reset the value of 'tethering.usb.enabled' and 'tethering.wifi.enabled, and also that we make the call to ModalDialog. Let me know if you need help with this. Thanks!

Comment 9

[Manuel Casas Barrado [:mancas]]

Hey Kevin! I'm not satisfied with the comparison of data Type. Do you know with which types of connection it is impossible to enable tethering?

After a long research in google, it seems the mobile phone can enable tethering regardless of the connection because of he is acting as a modem. What do you think about that? We could just check if the device has data connection instead of asking about the type of connection.

Comment 10

[Kevin Grandon :kgrandon]

At first glance I would opt for the "safe" route and just check navigator.isOnline, but there may be a better way of doing this. The mobile stuff is not my specialty, so I'm going to ask Tim and John if they have any ideas, or would know who to ask.

Tim/John - the pull request currently has a check for alerting the user when trying to enable the hotspot of: 
+ } else if (dataType != 'evdo0' && true === evt.settingValue)

Do you guys know of a less fragile check to perform? Would navigator.isOnline suffice?

Comment 11

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

I don't familiar with this part of system app. In fact I have never seen this file....

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sorry, I am also not familiar with `evdo0` and totally have no idea about it.

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

If I understand the scenario correctly the best way to check if the data connection is established is using `navigator.mozMobileConnections[0].data.connected`[1].

[1]: https://developer.mozilla.org/en-US/docs/Web/API/MozMobileConnectionInfo.connected

Comment 14

[Manuel Casas Barrado [:mancas]]

Agree, but I think the question here is if the phone can tether with any kind of connection.

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Jessica, is there any limitation on the network type of data connection when tethering?

Comment 16

[Jessica Jong [:jessica] (inactive)]

(In reply to Arthur Chen [:arthurcc] from comment #15)
> Hi Jessica, is there any limitation on the network type of data connection
> when tethering?

As far as I know, there is no limitation on network type, you can tether on edge, umts, hspa, etc. But tethering can only work when default mobile data connection is available, so comment 13 is right, you can check it with `navigator.mozMobileConnections[0].data.connected`.

However, if operator restricts tethering on DUN connection (a special type of data connection) only, that is another case, cause DUN connection is setup only when tethering is enabled. You can check if DUN is required or not with 'ro.tethering.dun_required' system property.

Comment 17

[Manuel Casas Barrado [:mancas]]

Created attachment 8498698 [details] [review]
Prompt user when try to enable hotspot

Comment 18

[Josh Cheng [:josh]]

Triage:
Hi Manuel,
Once it got approved, could you please prepare patch for on 2.0? Thanks!

Comment 19

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

This can't block, this is a feature request, not a bug. We can't uplift every feature since it will destabilize the branch already released.

Comment 20

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

What were you trying to say...?

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498698 [details] [review]
Prompt user when try to enable hotspot

The only changes to settings app should be removed. Flagging Alive for reviewing the part in system app.

Comment 22

[Josh Cheng [:josh]]

Hi Tim, during woodduck triage we figure out that this is generic bug. That's why I asked whether we can try land it on 2.0. If it is now too risky to commit in 2.0. We like the fix to land on 2.0M branch first since this is also bug reported by partner.
Thanks.

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8498698 [details] [review]
Prompt user when try to enable hotspot

r+ with nits

Comment 24

[Manuel Casas Barrado [:mancas]]

Hey, do you still want a patch for 2.0?

Comment 25

[Josh Cheng [:josh]]

Hi Manuel,
Yes, do you need to raise approval request for 2.0?
Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/cdf54e571cca86f67d0ce0488e9fe0c70b18d1f2

Please request Gaia v2.0 and v2.1 approval on this patch when you get a chance :)

Comment 27

[Francesco Lodolo [:flod]]

Sorry but this doesn't work at all.

-noHopspotWhenAPMisOn=Turn off airplane mode to use Wi-Fi hotspot
+noHopspotWhenAPMisOn=Turn off airplane mode to use {{setting}}

1st issue: you can't change a string like this
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings

Also: {{setting}} is once used as an object, once as a subject. This might work for English, but it definitely won't for other locales: even if you don't need to change the word to adapt to the case, you might need to use lowercase/uppercase, or use a different article.

We need 4 different strings, not substitutions. My suggestion would be to back-out this and land a proper patch.

This patch also has strings, so I have no idea how do you plan to land it on 2.1 or 2.0 (no idea if 2.0M works as 1.3T in terms of localization).

Comment 28

[Kevin Grandon :kgrandon]

Sending this back into triage. I don't think we can block on this given that there's strings here, and this is a new feature.

See comment 19 and comment 27.

Comment 29

[Kevin Grandon :kgrandon]

I've also noticed that this causes a problem when restarting the device when the hotspot is enabled - the dialog is shown about not having a data connection. I feel that this may be a different bug though. We may want to check with product about the desired behavior in this case.

Comment 30

[Kevin Grandon :kgrandon]

Backing out per comment 27 and potentially comment 29. Cleaing ni? on Alive, and leaving it for the patch author. The main thing we need to do here is re-land with new keys.

Backout: https://github.com/mozilla-b2g/gaia/commit/fa6463e2781b5a1aed95f55d16525ceec5b4a502

Comment 31

[Manuel Casas Barrado [:mancas]]

Created attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

Different strings have been added for each case.

The patch for 2.0 and 2.1 will be ready soon, once alive gives the r+ here.

Thanks!

Comment 32

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

The only concern is the l10n change, Pike please feedback.

Comment 33

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #32)
> Comment on attachment 8502313 [details] [review]
> Prompt user when try to enable hotspot
> 
> The only concern is the l10n change, Pike please feedback.

BTW please squash/rebase your 3 commits into only one.

Comment 34

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

Please squash/rebase your commits before requesting review.

Comment 35

[Francesco Lodolo [:flod]]

Comment on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

Stealing feedback for now (and thanks to :alive for pointing out the added period).

As commented on Github: since we're adding 2 new strings, adding them with a typo in the name doesn't make any sense (the removed string had the same typo).

Also: why are you adding a final period to the existing string? Was it wrong before based on specs?

Comment 36

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #35)
> Also: why are you adding a final period to the existing string? Was it wrong
> before based on specs?

Adding one more note: either the old string was missing a period, or the new ones have an extra period at the end. I don't know if we have specs for notifications around, but it would be worth it to check them.

Comment 37

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

Francesco, I've fixed the typo. In response of your previous comment, I added the final period to do it according with the new dialog. However, I agree with you and we must check the spec for this feature. Who can help us with the specs?

Comment 38

[Francesco Lodolo [:flod]]

Comment on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

String-wise it looks good now, unfortunately I don't have an answer about specs.

Comment 39

[Manuel Casas Barrado [:mancas]]

Hey Jenny, can you help us with the specs?

Comment 41

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

Hey Alive, please take a look at the code since I've changed the unit test.

Thanks!

Comment 42

[Wayne Chang [:wchang]]

Ditto comment 28 - Josh we shouldn't block on this or similar feature/bugs for 2.0 given where we are with 2.0.

If any one needs this, please renominate for a more suitable target release.

Comment 43

[Jenny Lee]

(In reply to Manuel Casas Barrado [:mancas] from comment #39)
> Hey Jenny, can you help us with the specs?

Hi Manuel,
We are undergoing new common control style audit lately, the spec is almost ready but I want to hold up releasing it until styling is more settled. 

1. turn off mobile data
2. turn on wifi hotspot (toggle on, wifi hotspot setting disabled)/USB tethering
3. show dialog as mentioned above ** with a period at the end **
4. wifi hotspot toggle turned off

Thanks!

Comment 44

[Manuel Casas Barrado [:mancas]]

Created attachment 8507752 [details] [review]
Patch for v2.1

Comment 45

[Manuel Casas Barrado [:mancas]]

Created attachment 8507754 [details] [review]
Patch for v2.0

Comment 46

[Francesco Lodolo [:flod]]

Patches for 2.0 and 2.1 with strings? It's my understanding that this was already ruled out for anything but master.

Comment 47

[Manuel Casas Barrado [:mancas]]

Hey Francesco, forget about the 2.0 and 2.1 patches. Is there any problems with strings in the main PR, or can we merge it?

Comment 48

[Francesco Lodolo [:flod]]

(In reply to Manuel Casas Barrado [:mancas] from comment #47)
> Hey Francesco, forget about the 2.0 and 2.1 patches. Is there any problems
> with strings in the main PR, or can we merge it?

Comment 38 is still valid: strings look good to me, no idea about the final period.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/5db876e7ffa7c64e2984eec9fb51da979571481e

Comment 50

[Kai-Zhen Li [:seinlin][:kli]]

Created attachment 8514081 [details] [review]
PR for v2.0m

Comment 51

[Jenny Lee]

Created attachment 8516601 [details]
[Settings] Internet Sharing v1.1.pdf

Hi all, please see attached for updated Internet sharing spec including error handling. Note that the new building block style is not in scope of 2.2, thanks!

Comment 52

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/cde17dcb0cf74d18df458d1d9db3aa700aa967aa

Comment 53

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.1.

Result: The error dialog is displayed when the user attempts to turn on Wi-Fi Hotspot without mobile data enabled.

Device: Flame 2.2 Master (319mb, KK, Shallow Flash)
BuildID: 20141117040203
Gaia: ddf5b92f43ec27c93ad4fea4fd1207da8936b8e7
Gecko: 21b745197618
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

========================================
Leaving verifyme for 2.1 uplift.

Comment 54

[Yeojin Chung [:YeojinC]]

(In reply to Yeojin Chung [:YeojinC] from comment #53)
> This issue is verified fixed on Flame 2.1.
> 
Correction: It was 2.2 (NOT 2.1) that I verified. Sorry about the confusion.

Comment 55

[Coler]

Verify passed, this issue can't be repro on Woodduck 2.0.
Attached: Verify_Woodduck_WIFIhotspot.mp4
Reproducing rate: 0/5

Woodduck 2.0 Build:
Gaia-Rev        60146ec47cd38a8be8ed22e0116902eceb9ac067
Gecko-Rev       cdfbe9866cf0b71b33454926638ce0cd8bb1fb00
Build-ID        20141117050313
Version         32.0

Comment 56

[Coler]

Created attachment 8524237 [details]
Verify_Woodduck_WIFIhotspot.MP4

Comment 57

[Shally Li（leave from Mozilla）]

Hi All,

    This bug has been successfully verified on latest Flame v2.0&2.1, Wifi hotspot will be turned on successfully but it has different verified result compared with above results in Comment 53 & Comment 55.
    See attachment: verified_v2.1.mp4.
    Reproduce rate: 0/5.

STR:
1. Insert SIM card and turn off Mobile data connection.
2. Turn on wifi hotspot in Settings.
**No alert messages and wifi hotspot will be turned on.

Flame 2.0 build:
Build ID               20150204000202
Gaia Revision          2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gaia Date              2015-01-22 21:13:40
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/69057b33ef5b
Gecko Version          32.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150204.035840
Firmware Date          Wed Feb  4 03:58:51 EST 2015
Bootloader             L1TC000118D0

Flame 2.1 build:
Build ID               20150204002437
Gaia Revision          17bf14f12e43043654498330d610d469b8b55e64
Gaia Date              2015-02-03 05:19:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/bdebcc47ec7a
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150204.035620
Firmware Date          Wed Feb  4 03:56:31 EST 2015
Bootloader             L1TC000118D0

Comment 58

[Shally Li（leave from Mozilla）]

Created attachment 8559659 [details]
verified_v2.1.MP4