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

VERIFIED FIXED in Firefox OS v2.0

Status

Firefox OS
Gaia::System
--
enhancement
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: dietrich, Assigned: mancas)

Tracking

unspecified
2.1 S7 (24Oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0M+, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

Details

Attachments

(9 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
We need more info from UX. What's your thought Jenny?
Flags: needinfo?(jelee)

Comment 2

3 years ago
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!
Flags: needinfo?(jelee)
(Assignee)

Updated

3 years ago
Assignee: nobody → b.mcb
(Assignee)

Comment 3

3 years ago
Created attachment 8493023 [details] [review]
Prompt user when try to enable hotspot
Attachment #8493023 - Flags: review?(21)
(Assignee)

Comment 4

3 years ago
Created attachment 8493024 [details]
usb-tethering.png
Attachment #8493024 - Flags: ui-review?(jelee)
(Assignee)

Comment 5

3 years ago
Created attachment 8493025 [details]
wifi-tethering.png
Attachment #8493025 - Flags: ui-review?(jelee)

Updated

3 years ago
Attachment #8493024 - Flags: ui-review?(jelee) → ui-review+

Updated

3 years ago
Attachment #8493025 - Flags: ui-review?(jelee) → ui-review+
See Also: → bug 1069160
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.
Attachment #8493023 - Flags: review?(21) → review?(im)
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 :).
Attachment #8493023 - Flags: review?(im) → review?(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!
Attachment #8493023 - Flags: review?(kgrandon)
Flags: needinfo?(b.mcb)
(Assignee)

Comment 9

3 years ago
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.
Flags: needinfo?(b.mcb) → needinfo?(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?
Flags: needinfo?(timdream)
Flags: needinfo?(kgrandon)
Flags: needinfo?(im)
I don't familiar with this part of system app. In fact I have never seen this file....
Flags: needinfo?(timdream) → needinfo?(arthur.chen)
Sorry, I am also not familiar with `evdo0` and totally have no idea about it.
Flags: needinfo?(im)
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
Flags: needinfo?(arthur.chen)
(Assignee)

Comment 14

3 years ago
Agree, but I think the question here is if the phone can tether with any kind of connection.
Flags: needinfo?(arthur.chen)
Hi Jessica, is there any limitation on the network type of data connection when tethering?
Flags: needinfo?(arthur.chen) → needinfo?(jjong)
(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.
Flags: needinfo?(jjong)
(Assignee)

Comment 17

3 years ago
Created attachment 8498698 [details] [review]
Prompt user when try to enable hotspot
Attachment #8493023 - Attachment is obsolete: true
Attachment #8498698 - Flags: review?(arthur.chen)

Comment 18

3 years ago
Triage:
Hi Manuel,
Once it got approved, could you please prepare patch for on 2.0? Thanks!
blocking-b2g: --- → 2.0+
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
Flags: needinfo?(b.mcb)
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.
Flags: needinfo?(jocheng)

Updated

3 years ago
Flags: needinfo?(jocheng)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
What were you trying to say...?
Flags: needinfo?(jocheng)
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.
Attachment #8498698 - Flags: review?(arthur.chen) → review?(alive)

Comment 22

3 years ago
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.
Flags: needinfo?(jocheng)
Comment on attachment 8498698 [details] [review]
Prompt user when try to enable hotspot

r+ with nits
Attachment #8498698 - Flags: review?(alive) → review+
(Assignee)

Updated

3 years ago
Flags: needinfo?(b.mcb)
Keywords: checkin-needed
(Assignee)

Comment 24

3 years ago
Hey, do you still want a patch for 2.0?
Flags: needinfo?(jocheng)

Comment 25

3 years ago
Hi Manuel,
Yes, do you need to raise approval request for 2.0?
Thanks!
Flags: needinfo?(jocheng) → needinfo?(b.mcb)
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 :)
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.2: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
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).
Flags: needinfo?(alive)
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.
Severity: normal → enhancement
blocking-b2g: 2.0+ → 2.0?
Component: Gaia::Settings → Gaia::System
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.
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
Status: RESOLVED → REOPENED
Flags: needinfo?(alive)
Resolution: FIXED → ---
(Assignee)

Comment 31

3 years ago
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!
Attachment #8498698 - Attachment is obsolete: true
Attachment #8502313 - Flags: review?(alive)
Flags: needinfo?(b.mcb)
Comment on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

The only concern is the l10n change, Pike please feedback.
Attachment #8502313 - Flags: feedback?(l10n)
(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 on attachment 8502313 [details] [review]
Prompt user when try to enable hotspot

Please squash/rebase your commits before requesting review.
Attachment #8502313 - Flags: review?(alive)
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?
Attachment #8502313 - Flags: feedback?(l10n) → feedback-
(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.
(Assignee)

Comment 37

3 years ago
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?
Attachment #8502313 - Flags: feedback- → feedback?(francesco.lodolo)
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.
Attachment #8502313 - Flags: feedback?(francesco.lodolo) → feedback+
(Assignee)

Comment 39

3 years ago
Hey Jenny, can you help us with the specs?
Flags: needinfo?(jelee)

Updated

3 years ago
Duplicate of this bug: 1069160
(Assignee)

Comment 41

3 years ago
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!
Attachment #8502313 - Flags: review?(alive)
Attachment #8502313 - Flags: review?(alive) → review+
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.
blocking-b2g: 2.0? → -

Comment 43

3 years ago
(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!
Flags: needinfo?(jelee)
(Assignee)

Comment 44

3 years ago
Created attachment 8507752 [details] [review]
Patch for v2.1
(Assignee)

Comment 45

3 years ago
Created attachment 8507754 [details] [review]
Patch for v2.0
Patches for 2.0 and 2.1 with strings? It's my understanding that this was already ruled out for anything but master.
(Assignee)

Comment 47

3 years ago
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?
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/5db876e7ffa7c64e2984eec9fb51da979571481e
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (24Oct)
blocking-b2g: - → 2.0M?
Created attachment 8514081 [details] [review]
PR for v2.0m
Depends on: 1091936

Comment 51

3 years ago
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!

Updated

3 years ago
blocking-b2g: 2.0M? → 2.0M+
v2.0m: https://github.com/mozilla-b2g/gaia/commit/cde17dcb0cf74d18df458d1d9db3aa700aa967aa
status-b2g-v2.0M: affected → fixed
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: fixed → verified
Flags: needinfo?(ktucker)
Keywords: verifyme
(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.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)

Comment 55

3 years ago
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
status-b2g-v2.0M: fixed → verified

Comment 56

3 years ago
Created attachment 8524237 [details]
Verify_Woodduck_WIFIhotspot.MP4

Updated

3 years ago
Blocks: 1080481
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
status-b2g-v2.0: affected → verified
status-b2g-v2.1: affected → verified
Keywords: verifyme
Created attachment 8559659 [details]
verified_v2.1.MP4
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.