1084252 - Airplane mode dialog not displayed when trying to call a contact with airplane mode activated

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Viorela Ioia [:viorela]]

Calling a contact with airplane mode activated has no effect - airplane mode dialog is not displayed on the screen.

#STR:
1. Launch Settings app
2. Switch on airplane mode
3. Launch Dialer app
4. Write a phone number, then tap Call button 

#Expected results:
"Airplane mode activated" message is displayed on the screen 

#Actual results: 
Tapping Call button has no effect; there's no message (to inform the users that airplane mode is activated) displayed on the screen

I was able to reproduce the failure locally on v2.2, by running test_dialer_airplane_mode.py, and also manually.
This is not reproducible on v2.1.

Regression range, base on mozilla-inbound builds:
Last working:
Gaia-Rev        841d0d7d1b879f0ff4b5a8727f5dd23c7b0000a9
Gecko-Rev       https://hg.mozilla.org/integration/mozilla-inbound/rev/451ef271a247
Build-ID        20141016070246
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141016.104416
FW-Date         Thu Oct 16 10:44:27 EDT 2014
Bootloader      L1TC10011800

First failing: 
Gaia-Rev        5c636a7a54b2c86d8ff6bc1aa1e5f9594c7bc586
Gecko-Rev       https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0a473895ff
Build-ID        20141016075045
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141016.112136
FW-Date         Thu Oct 16 11:21:46 EDT 2014
Bootloader      L1TC10011800

Jenkins report:
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk-319.mozilla-central.ui.functional.non-smoke/59/HTML_Report/

Comment 1

[Viorela Ioia [:viorela]]

Attachment: logcat.txt

Comment 2

[Viorela Ioia [:viorela]]

Attachment: screenshot.png

Comment 3

[Johan Lorenzo [:jlorenzo]]

Setting status flag per comment 0.

Nominating this regression manually reproducible.

Comment 4

[Viorela Ioia [:viorela]]

This issue is still reproducible in latest master build:
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk-319.mozilla-central.ui.functional.non-smoke/69/HTML_Report/

Device firmware (date) 	21 Oct 2014 04:15:39
Device firmware (incremental) 	eng.cltbld.20141021.071529
Device firmware (release) 	4.4.2
Device identifier 	flame
Gaia date 	20 Oct 2014 12:16:40
Gaia revision 	457a54fc3200
Gecko build 	20141021040206
Gecko revision 	29fbfc1b31aa
Gecko version 	36.0a1

Repro rare of the issue: 5 out of 5

Comment 5

[Chris Kreinbring [:CKreinbring]]

Regression window
Last working
BuildID: 20141015113215
Gaia: 841d0d7d1b879f0ff4b5a8727f5dd23c7b0000a9
Gecko: a280a03c9f3c
Platform Version: 36.0a1
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First broken
BuildID: 20141016065647
Gaia: 5c636a7a54b2c86d8ff6bc1aa1e5f9594c7bc586
Gecko: 9fe9bace9ed5
Platform Version: 36.0a1
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Working Gaia / Broken Gecko = No repro
Gaia: 841d0d7d1b879f0ff4b5a8727f5dd23c7b0000a9
Gecko: 9fe9bace9ed5
Broken Gaia / Working Gecko = Repro
Gaia: 5c636a7a54b2c86d8ff6bc1aa1e5f9594c7bc586
Gecko: a280a03c9f3c
Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/841d0d7d1b879f0ff4b5a8727f5dd23c7b0000a9...5c636a7a54b2c86d8ff6bc1aa1e5f9594c7bc586

B2G Inbound
Last working
BuildID: 20141015070318
Gaia: a18559e844b6aec946309a3bee6a32ca06ab4649
Gecko: fe4916e0e9df
Platform Version: 36.0a1
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First broken
BuildID: 20141015102818
Gaia: 93fb5070782a549977cf8c485c5d93de54ec657e
Gecko: 069db53e0f68
Platform Version: 36.0a1
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Working Gaia / Broken Gecko = No repro
Gaia: a18559e844b6aec946309a3bee6a32ca06ab4649
Gecko: 069db53e0f68
Broken Gaia / Working Gecko = Repro
Gaia: 93fb5070782a549977cf8c485c5d93de54ec657e
Gecko: fe4916e0e9df
Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/a18559e844b6aec946309a3bee6a32ca06ab4649...93fb5070782a549977cf8c485c5d93de54ec657e

Comment 6

[Joshua Mitchell (Inactive)]

Possibly broken by either Bug 1082741 or Bug 1078295 

Going to NI both patch authors to save time

Comment 7

[(Inactive after June) George Duan [:gduan] [:喬智]]

I tried to revert my patch and the issue still exists. However, I also tried to reset to master and revert chris' patch, then the issue's gone.

Comment 8

[Chris Lord [:cwiiis]]

Looking.

Comment 9

[Chris Lord [:cwiiis]]

Bug fixed, just trying to craft a marionette test to catch this...

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Fix missing airplane mode dialog

To guard against this CSS affecting unwanted bits, I've added an explicit data property in option_menu.js and sms/js/dialog.js and styled against that instead.

I tried to craft a marionette test to verify this dialog, but unfortunately it's not possible to dial in b2g-desktop (even with airplane mode on). I've been assured on IRC that there are python tests that get run on device that test exactly this, so I'm leaving the fix alone.

Comment 11

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression, blocking QA testing.

Comment 12

[Steve Chung [:steveck]]

Except this waring dialog, there is still one option menu style form sim picker in both apps(message & dialer). You might need to apply this subtype for sim picker. And maybe there is more form in dialer that might need this change. Since this patch actually changes behavior in dialer, we will need Doug's review as well.

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

I think we still need option menu transition for sim picker, but I didn't have another available sim currently. Will give it a try tomorrow.

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

Redirecting to Tamara.

Comment 15

[Tamara Hills [:thills]]

My only comment is can we add a test to telephony_helper_test.js to make sure that the dialog is displayed when we are in airplane mode.

Comment 16

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

Forgot to feedback myself for when review is done.

Comment 17

[Chris Lord [:cwiiis]]

(In reply to Steve Chung [:steveck] from comment #13)
> Comment on attachment 8509504 [details] [review]
> Fix missing airplane mode dialog
> 
> I think we still need option menu transition for sim picker, but I didn't
> have another available sim currently. Will give it a try tomorrow.

I'm not sure I fully understand this comment - This is another dialog that also needs transitions? I would suppose that if that's what you meant, that should be a separate bug from this one (which should just fix the dialogs I broke), unless it's a particularly easy fix.

Comment 19

[Steve Chung [:steveck]]

(In reply to Chris Lord [:cwiiis] from comment #17)
> (In reply to Steve Chung [:steveck] from comment #13)
> > Comment on attachment 8509504 [details] [review]
> > Fix missing airplane mode dialog
> > 
> > I think we still need option menu transition for sim picker, but I didn't
> > have another available sim currently. Will give it a try tomorrow.
> 
> I'm not sure I fully understand this comment - This is another dialog that
> also needs transitions? I would suppose that if that's what you meant, that
> should be a separate bug from this one (which should just fix the dialogs I
> broke), unless it's a particularly easy fix.

Well, the sim picker actually applied option menu styling in Bug 1078295, but it also failed to display like this bug(so patch addressed this issue as well), since these dialog's display controlling is different from the shared option menu. I only have a suggestion about the attachment option menu: if the styling should belong to the element created dynamically, we should not apply subtype to attachment menu directly, but use shared option menu to create for attachment menu instead. Please create another bug by utilizing the option menu widget for attachment menu, thanks.

Comment 20

[Tamara Hills [:thills]]

Hi Chris,

The patch works fine for me. 

I was trying to trace through this in the code just to make sure I really understand what's happening.  I'm not sure how the OptionMenu changes are getting invoked from the dialer side of things.

In the dialer I see that we get the error back from gecko and it goes into TelephonyMessages and identifies it in displayMessage a callAirplaneModeMessage.  Then it calls ConfirmDialog.show.

The strange thing is that I can't see Options_menu.js changes getting called anywhere.  Do you know how the OptionMenu constructor is getting called?  I'm sure I'm missing something as my console.log statements are not getting printed out.

Thanks,

-tamara

Comment 21

[Chris Lord [:cwiiis]]

(In reply to Steve Chung [:steveck] from comment #19)
> Well, the sim picker actually applied option menu styling in Bug 1078295,
> but it also failed to display like this bug(so patch addressed this issue as
> well), since these dialog's display controlling is different from the shared
> option menu. I only have a suggestion about the attachment option menu: if
> the styling should belong to the element created dynamically, we should not
> apply subtype to attachment menu directly, but use shared option menu to
> create for attachment menu instead. Please create another bug by utilizing
> the option menu widget for attachment menu, thanks.

Apologies, I didn't quite follow this - I guess the short is just that attachment menu doesn't use the otpion_menu code, but it should? I'll file a bug for this after confirmation.

Comment 22

[Steve Chung [:steveck]]

(In reply to Chris Lord [:cwiiis] from comment #21)
> (In reply to Steve Chung [:steveck] from comment #19)
> > Well, the sim picker actually applied option menu styling in Bug 1078295,
> > but it also failed to display like this bug(so patch addressed this issue as
> > well), since these dialog's display controlling is different from the shared
> > option menu. I only have a suggestion about the attachment option menu: if
> > the styling should belong to the element created dynamically, we should not
> > apply subtype to attachment menu directly, but use shared option menu to
> > create for attachment menu instead. Please create another bug by utilizing
> > the option menu widget for attachment menu, thanks.
> 
> Apologies, I didn't quite follow this - I guess the short is just that
> attachment menu doesn't use the otpion_menu code, but it should? I'll file a
> bug for this after confirmation.

Yes, attachment menu should created by shared otpion_menu code instead of leaving it in message html. Sorry if my explanation is not clear enough.

Comment 23

[Tamara Hills [:thills]]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

Hi Chris,

Thanks for your explanations.  

Is it possible we can add a test for this to make sure that the Airplane Mode Dialog styling is not styled as per the option_menu styling now that you added the data subtype?

Thanks,

-tamara

Comment 24

[Chris Lord [:cwiiis]]

(In reply to Tamara Hills [:thills] from comment #23)
> Comment on attachment 8509504 [details] [review]
> Fix missing airplane mode dialog
> 
> Hi Chris,
> 
> Thanks for your explanations.  
> 
> Is it possible we can add a test for this to make sure that the Airplane
> Mode Dialog styling is not styled as per the option_menu styling now that
> you added the data subtype?
> 
> Thanks,
> 
> -tamara

I'll interpret this as needing a test that when the airplane mode dialog is shown, it doesn't have visibility: hidden - testing that a particular element *doesn't* get a particular style, when we know it doesn't because the CSS doesn't match, wouldn't really be of any use.

Comment 25

[Chris Lord [:cwiiis]]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

This stops the attachment menu using option_dialog.css, as suggested (we should have it use option_menu or another widget in another bug) and as suggested, adds a test to make sure that the options_menu styling doesn't affect confirm dialogs.

I wanted to make a more generic airplane-mode dialing test, but it's impossible to do that with marionette tests and near-impossible to do it with unit testing it seems, so I guess this will have to do.

Comment 29

[Tamara Hills [:thills]]

Attachment: short.png

Comment 30

[Tamara Hills [:thills]]

Attachment: long.png

Comment 31

[Tamara Hills [:thills]]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

Hi Chris,

Thanks for adding the test.  It looks good.  While I was testing I ran into an issue and wanted to see if you can have look at this before we move it forward. 

Intermittently, I'm getting the styling on the attachment short.png and sometimes I get the styling on the attachment from screenshot long.png

I tested for a while and could not figure out what was the trigger for getting either the short or the long version of the confirm dialog.

Would you be able to check it out and let me know what you see?

Thanks,

-tamara

Comment 32

[Chris Lord [:cwiiis]]

(In reply to Tamara Hills [:thills] from comment #31)
> Comment on attachment 8509504 [details] [review]
> Fix missing airplane mode dialog
> 
> Hi Chris,
> 
> Thanks for adding the test.  It looks good.  While I was testing I ran into
> an issue and wanted to see if you can have look at this before we move it
> forward. 
> 
> Intermittently, I'm getting the styling on the attachment short.png and
> sometimes I get the styling on the attachment from screenshot long.png
> 
> I tested for a while and could not figure out what was the trigger for
> getting either the short or the long version of the confirm dialog.
> 
> Would you be able to check it out and let me know what you see?
> 
> Thanks,
> 
> -tamara

Can you only reproduce this with the patch applied? I just want to make sure that we're not conflating unrelated issues here.

Comment 34

[Steve Chung [:steveck]]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

Just one nit about the comment, it looks fine within message side.

Comment 35

[Tamara Hills [:thills]]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

Hi Chris,

I tested with the revert and everything seems to show the short version.  This is what I get most of the time with your patch as well.

:drs will need to review as I'm doing my first reviews ;)

Thanks,
-tamara

Comment 36

[Tamara Hills [:thills]]

Chris,

I forgot to mention that I tested all the duplicates and everything works!

thanks,
-tamara

Comment 37

[Chris Lord [:cwiiis]]

Excellent, addressed nit and merged: https://github.com/mozilla-b2g/gaia/commit/2f195566bea601af4d1d5d9fc5f0ff737da40f67

Comment 38

[Chris Lord [:cwiiis]]

Damnit, I messed up my rebase and the nit didn't get committed, so  merged a separate commit for it:

https://github.com/mozilla-b2g/gaia/commit/9a951d723eb1e70d0db9ab1bf6cab9e34a96ff38

Comment 39

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8509504 [details] [review]
Fix missing airplane mode dialog

Late feedback+, FWIW.

Comment 40

[jbolton]

Issue verified as fixed on Flame 2.2.

Actual Result: Airplane mode dialogue is now displayed when trying to make a call with airplane mode on.

Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141121040204
Gaia: 25388c6bce932657ebf93adedf31881bfaf88c15
Gecko: 3366c0fcf9c2
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0