Closed
Bug 1026306
Opened 8 years ago
Closed 8 years ago
when mobile data isn't connected, wifi hotspot setting silently fails and disables itself
Categories
(Firefox OS Graveyard :: Gaia::System, enhancement)
Tracking
(blocking-b2g:2.0M+, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)
People
(Reporter: dietrich, Assigned: mancas)
References
Details
Attachments
(9 files, 2 obsolete files)
20.75 KB,
image/png
|
jelee
:
ui-review+
|
Details |
20.63 KB,
image/png
|
jelee
:
ui-review+
|
Details |
46 bytes,
text/x-github-pull-request
|
alive
:
review+
flod
:
feedback+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
809.92 KB,
application/pdf
|
Details | |
1.10 MB,
video/mp4
|
Details | |
2.23 MB,
video/mp4
|
Details |
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•8 years ago
|
||
We need more info from UX. What's your thought Jenny?
Flags: needinfo?(jelee)
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•8 years ago
|
Assignee: nobody → b.mcb
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8493023 -
Flags: review?(21)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8493024 -
Flags: ui-review?(jelee)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8493025 -
Flags: ui-review?(jelee)
Attachment #8493024 -
Flags: ui-review?(jelee) → ui-review+
Attachment #8493025 -
Flags: ui-review?(jelee) → ui-review+
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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•8 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)
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
I don't familiar with this part of system app. In fact I have never seen this file....
Flags: needinfo?(timdream) → needinfo?(arthur.chen)
Comment 12•8 years ago
|
||
Sorry, I am also not familiar with `evdo0` and totally have no idea about it.
Flags: needinfo?(im)
Comment 13•8 years ago
|
||
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•8 years ago
|
||
Agree, but I think the question here is if the phone can tether with any kind of connection.
Flags: needinfo?(arthur.chen)
Comment 15•8 years ago
|
||
Hi Jessica, is there any limitation on the network type of data connection when tethering?
Flags: needinfo?(arthur.chen) → needinfo?(jjong)
Comment 16•8 years ago
|
||
(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•8 years ago
|
||
Attachment #8493023 -
Attachment is obsolete: true
Attachment #8498698 -
Flags: review?(arthur.chen)
Comment 18•8 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)
Comment 19•8 years ago
|
||
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•8 years ago
|
Flags: needinfo?(jocheng)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Comment 21•8 years ago
|
||
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•8 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 23•8 years ago
|
||
Comment on attachment 8498698 [details] [review] Prompt user when try to enable hotspot r+ with nits
Attachment #8498698 -
Flags: review?(alive) → review+
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(b.mcb)
Keywords: checkin-needed
Comment 25•8 years ago
|
||
Hi Manuel, Yes, do you need to raise approval request for 2.0? Thanks!
Flags: needinfo?(jocheng) → needinfo?(b.mcb)
Comment 26•8 years ago
|
||
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
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
Comment 27•8 years ago
|
||
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)
Comment 28•8 years ago
|
||
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?
Updated•8 years ago
|
Component: Gaia::Settings → Gaia::System
Comment 29•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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 32•8 years ago
|
||
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)
Comment 33•8 years ago
|
||
(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•8 years ago
|
||
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 35•8 years ago
|
||
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-
Comment 36•8 years ago
|
||
(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•8 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 38•8 years ago
|
||
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 41•8 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)
Updated•8 years ago
|
Attachment #8502313 -
Flags: review?(alive) → review+
Comment 42•8 years ago
|
||
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•8 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•8 years ago
|
||
Assignee | ||
Comment 45•8 years ago
|
||
Comment 46•8 years ago
|
||
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•8 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)
Comment 48•8 years ago
|
||
(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•8 years ago
|
Keywords: checkin-needed
Comment 49•8 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/5db876e7ffa7c64e2984eec9fb51da979571481e
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (24Oct)
Updated•8 years ago
|
blocking-b2g: - → 2.0M?
Comment 50•8 years ago
|
||
Comment 51•8 years ago
|
||
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•8 years ago
|
blocking-b2g: 2.0M? → 2.0M+
Comment 52•8 years ago
|
||
v2.0m: https://github.com/mozilla-b2g/gaia/commit/cde17dcb0cf74d18df458d1d9db3aa700aa967aa
Comment 53•8 years ago
|
||
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?]
Flags: needinfo?(ktucker)
Keywords: verifyme
Comment 54•8 years ago
|
||
(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.
Updated•8 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 55•8 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
Comment 56•8 years ago
|
||
Comment 57•8 years ago
|
||
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•8 years ago
|
||
Updated•8 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
You need to log in
before you can comment on or make changes to this bug.
Description
•