Enpei Reporter
	Description • 11 years ago

When 2 SIM are PIN enabled and reboot device, "Skip" and "OK" buttons will only show about 1 second above keypad on SIM 2 PIN code page after entering SIM 1 PIN code.

* Build Number  
Fugu
Gaia      ef64aa12b65723a29b3d5dbe115820a557763fab
Gecko     f12cc94c28d3eadde2054dda6a19b072c6618ccd
BuildID   20140306053055
Version   30.0a1

* Reproduce Steps
1. Enable both SIM cards PIN.
2. Reboot device.
3. Enter SIM 1 PIN code. Check SIM 2 PIN code page pop up next.

* Expected Result
Like SIM 1 enter PIN code page, "Send" and "Ok" buttons are above keypad that user can choose either after enter numbers. 

* Actual Result
When SIM 2 enter PIN code page show, the buttons are shown above keypad but only for about 1 second. User has to tap anywhere on the page to minimize keypad to access the buttons.

* Occurrence rate
100%

	Julien Wajsberg [:julienw]
	Comment 1 • 11 years ago

For me, the contrary happens: in the PIN code panel for SIM1 the "Skip" and "OK" buttons are hidden, but in the PIN code panel for SIM2 it's fine.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 2 • 11 years ago

This happens on Tarako as well.  I am guessing it may happen on 1.3.

	yang.zhao
	Comment 4 • 11 years ago

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #2)
> This happens on Tarako as well.  I am guessing it may happen on 1.3.

Does anyone do see this bug?

	Eric Chang [:ericcc] [:echang]
	Comment 5 • 11 years ago

Looks okay with v1.4 on Fugu.
GAIA_REV=441c4bcd8ac4f8c01a9bc5a2f8d64eaa87844803
GECKO_REV=0c18beacb37a7e1e80bcf6d46cc8ff9a18c0c653
GAIA_BRANCH=mozillaorg/v1.4
GECKO_BRANCH=mozillaorg/v1.4
BUILD_TAG=jenkins-B2G.v1.4.0.fugu-25
BuildID=20140416041552

	Eric Chang [:ericcc] [:echang]
	Comment 6 • 11 years ago

But the "Skip" and "OK" buttons are missing above keypad on SIM 1 PIN sometimes

	Julien Wajsberg [:julienw]
	Comment 7 • 11 years ago

So you see like me :) (except it's nearly 100% for me).

	Peipei Cheng (needinfo if you need my action)
	Comment 8 • 11 years ago

This also happens on v1.4.

	Gregor Wagner [:gwagner]
	Comment 9 • 11 years ago

David, can your team take this bug?

	Jason Smith [:jsmith]
	Comment 10 • 11 years ago

This has been around multiple releases that have already shipped, so this isn't a blocker.

	Jason Smith [:jsmith]
	Comment 11 • 11 years ago

See comment 10 - this is not a blocker.

	James Zhang (Spreadtrum)
	Comment 12 • 11 years ago

Hi Arvin, can you take a look?
tarako and dolphin also have this issue.

	helloarvin
	Comment 13 • 11 years ago

I will check it later.

	Wei Gao (Spreadtrum) Assignee
	Comment 14 • 10 years ago

Hi all
I checked the code and found neglecting the height of keyboard when update screen is the root cause. 
After I add a judgment of keyboard, it works normal.

Please help to review this patch.
Thanks.

	Wei Gao (Spreadtrum) Assignee
	Comment 15 • 10 years ago

I also checked the code in FirefoxOS_v1.4 and found it is the same as FirefoxOS_v1.3.

Please also review this patch on v1.4
Thanks.

	Wei Gao (Spreadtrum) Assignee
	Comment 16 • 10 years ago

Hi Alive

Thanks for your review.
Can you help to land this patch on 1.3t and 1.4?
Thanks.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 17 • 10 years ago

Please create a pull request, it's not difficult.

	Wei Gao (Spreadtrum) Assignee
	Comment 18 • 10 years ago

Hi Alive

I have submited a pull request, please help to review.
Thanks a lot.

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

	Wei Gao (Spreadtrum) Assignee
	Comment 19 • 10 years ago

Please ignore the pull request in Comment18, I will recommit later. 
Sorry.

	Wei Gao (Spreadtrum) Assignee
	Comment 20 • 10 years ago

Hi Alive

I have submited a pull request, please help to review.
Sorry to keep you waiting so long

Thanks very much.

	Wei Gao (Spreadtrum) Assignee
	Comment 21 • 10 years ago

Hi Alive

This is another pull request on v1.4
Please help to review as well.

Thanks.

	Wei Gao (Spreadtrum) Assignee
	Comment 22 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #17)
> Please create a pull request, it's not difficult.

Thanks a great for your review.

	Wei Gao (Spreadtrum) Assignee
	Comment 23 • 10 years ago

Hi Alive

After I commit pull requests, what should I need to do next?
I find the patch has not been merged into v1.3t and v1.4.
Is anything I forget to do?
As I am a new, I don't know the process of mozilla for merging code.
Can you help me?

Thanks.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 24 • 10 years ago

(In reply to Wei Gao (Spreadtrum) from comment #23)
> Hi Alive
> 
> After I commit pull requests, what should I need to do next?
> I find the patch has not been merged into v1.3t and v1.4.
> Is anything I forget to do?
> As I am a new, I don't know the process of mozilla for merging code.
> Can you help me?
> 
> Thanks.

This is not a 1.4+ or 1.3t+ bug, so no uplift will go.
You should ask for 1.3t? to do so or ask approval.

	Wayne Chang [:wchang]
	Comment 25 • 10 years ago

Wei,

When this issue happens, does it block the user from moving forward due to the lack of OK/Cancel button?
(after enter PIN, can user continue to use phone?)


If the user cannot proceed forward when meeting this issue, then we should have this as blocking. 

================================================================
Jason,

(In reply to Jason Smith [:jsmith] from comment #10)
> This has been around multiple releases that have already shipped, so this
> isn't a blocker.

This has been around but we've only just started enable multi-sim since 1.3 so I think the condition is different here.

	Wei Gao (Spreadtrum) Assignee
	Comment 26 • 10 years ago

(In reply to Wayne Chang [:wchang] from comment #25)
> Wei,
> 
> When this issue happens, does it block the user from moving forward due to
> the lack of OK/Cancel button?
> (after enter PIN, can user continue to use phone?)
> 
> If the user cannot proceed forward when meeting this issue, then we should
> have this as blocking. 

Hi Wayne
User can continue to use phone, if the keyboard hide, OK/Cancel button will show. But I think it is really a problem for user, and I think we should improve the user experience for this.
How do you think about it?
Thanks.

	Wayne Chang [:wchang]
	Comment 27 • 10 years ago

After discussing with Joe we believe we should take this since the bug creates a broken UI and may frustrate the users.

Wei, please request for approval‑gaia‑v1.4? on your 1.4 patch and fill in the approval form.

	Wei Gao (Spreadtrum) Assignee
	Comment 28 • 10 years ago

Comment on attachment 8438983 [details]
pull request on v1.4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):OK/Cancel button issue
[User impact] if declined:impact user experience
[Testing completed]:yes
[Risk to taking this patch] (and alternatives if risky):no
[String changes made]:no

Hi Wayne
Thanks for your review.

	Wayne Chang [:wchang]
	Comment 29 • 10 years ago

Comment on attachment 8438983 [details]
pull request on v1.4

Redirecting to Lawrence as Preeti is away.
Please see Wei's approval request from previous comment.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 30 • 10 years ago

(In reply to Wei Gao (Spreadtrum) from comment #28)
> [Bug caused by] (feature/regressing bug #):OK/Cancel button issue

Do you have a reference to the bug in which the feature was added or the regression was introduced?

> [User impact] if declined:impact user experience

Can you elaborate? How does this bug impact the user experience?

> [Testing completed]:yes

Can you elaborate? How have you tested the feature?

As well, I would prefer to see this land on 1.3 and receive some level of testing before landing on 1.4. Is this patch ready to land on 1.3?

	Wei Gao (Spreadtrum) Assignee
	Comment 31 • 10 years ago

(In reply to Lawrence Mandel [:lmandel] from comment #30)

I am sorry for I don't know the answer for the approval request is so strict, I will answer again.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):-
[User impact] if declined: When pin page for sim1 show, the OK/Cancel button is on the top of keyboard. User can click OK when he input the pin number directly. But when pin page for sim2 show, the OK/Cancel button can't be found, as it is hidden by keyboard. I think the user will consider it very strange.
[Testing completed]:yes, for I used tarako(6821) and dolphin(7715) to test it, and it work normal with this patch.
[Risk to taking this patch] (and alternatives if risky):I think there is no risk for our system. Because when refresh the interface, the height of keyboard originally should be considered.
[String changes made]:no

Thanks.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 32 • 10 years ago

Thanks for the additional details Wei.

As I said in comment 30, I would like to see this bug land elsewhere before approving for 1.4. Has this bug landed on Master? (If so, let's resolve it.) Does it need to land on 2.0? Is it ready to land on 1.3?

	Julien Wajsberg [:julienw]
	Comment 33 • 10 years ago

Alive, is it ready to be merged on master?

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 34 • 10 years ago

Green so for sure.

	Carsten Book [:Tomcat]
	Comment 35 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/02a7e1a73fffc9678db77635214c5cd218965f3b

	bhavana bajaj [:bajaj]
	Comment 36 • 10 years ago

(In reply to Lawrence Mandel [:lmandel] from comment #32)
> Thanks for the additional details Wei.
> 
> As I said in comment 30, I would like to see this bug land elsewhere before
> approving for 1.4. Has this bug landed on Master? (If so, let's resolve it.)
> Does it need to land on 2.0? Is it ready to land on 1.3?

Looks like a low risk change so we should consider landing on 2.0 as well. I'll approve for 1.4+ and setting NI on :ryan to help with landings across branches.

	Wei Gao (Spreadtrum) Assignee
	Comment 37 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #35)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> 02a7e1a73fffc9678db77635214c5cd218965f3b

Thank you for your help.
Could you help to merge this to v1.4 also?
Thanks a great.

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

	Julien Wajsberg [:julienw]
	Comment 38 • 10 years ago

This will be done by sheriffs today (friday was a holiday in USA)

	Wei Gao (Spreadtrum) Assignee
	Comment 39 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #38)
> This will be done by sheriffs today (friday was a holiday in USA)

Oh, I see, Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #35)
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> 02a7e1a73fffc9678db77635214c5cd218965f3b

This was to v1.3t, not master. And from the looks of it, master/v2.0 are going to need a different patch than v1.4/v1.3t did.

v1.4:   https://github.com/mozilla-b2g/gaia/commit/342838c34f0d9b3d541ca024abae339c28e3d31e

	Wei Gao (Spreadtrum) Assignee
	Comment 41 • 10 years ago

Hi Alive

As I have no phone with master b2g in it. So I can't test it by myself.
I wonder can this patch be granted on mater.
Please help to review.
Thanks a great.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 42 • 10 years ago

Comment on attachment 8452214 [details] [diff] [review]
[master]_updateHeight_SystemDialog.patch

Review of attachment 8452214 [details] [diff] [review]:
-----------------------------------------------------------------

Please have a pull request hence we could see if Continuos Integration catches some errors.

	Wei Gao (Spreadtrum) Assignee
	Comment 43 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #42)
> Please have a pull request hence we could see if Continuos Integration
> catches some errors.

Dear Alive

I have commit a PR, but I don't know why the CI test show 'KeyboardManager' is not defined?
I see 'KeyboardManager' is a global variable.
Could you help me?
Thanks a lot.

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

	Julien Wajsberg [:julienw]
	Comment 44 • 10 years ago

You need to add a "needinfo" request to get someone to answer.

Alive, please see previous comment.

	Wei Gao (Spreadtrum) Assignee
	Comment 45 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #44)
> You need to add a "needinfo" request to get someone to answer.
> 
> Alive, please see previous comment.

Oh, that's right.
Thanks for your suggestion.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 46 • 10 years ago

You need to include a mocked KeyboardManager in system_dialog_test.
https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

Unit test means no global.

	Wei Gao (Spreadtrum) Assignee
	Comment 47 • 10 years ago

> You need to include a mocked KeyboardManager in system_dialog_test.
> https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/
> Gaia_unit_tests

Thanks for your modification suggestion.
Please help to review again.
Thanks a great.

	Wei Gao (Spreadtrum) Assignee
	Comment 48 • 10 years ago

Maybe there still have this question?

TEST-UNEXPECTED-FAIL | apps/system/js/system_dialog.js | line 116, col 26, 'KeyboardManager' is not defined.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 49 • 10 years ago

Comment on attachment 8453596 [details]
pull request on master

Please squash your 5 commits into 1 before merging

	Wei Gao (Spreadtrum) Assignee
	Comment 50 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #49)
> Comment on attachment 8453596 [details]
> pull request on master
> 
> Please squash your 5 commits into 1 before merging

Hi Alive

I am sorry to trouble.
But should I have to recommit a PR or is there a button in github to squash all my 5 commits into 1?
Thank you very much.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 51 • 10 years ago

(In reply to Wei Gao (Spreadtrum) from comment #50)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #49)
> > Comment on attachment 8453596 [details]
> > pull request on master
> > 
> > Please squash your 5 commits into 1 before merging
> 
> Hi Alive
> 
> I am sorry to trouble.
> But should I have to recommit a PR or is there a button in github to squash
> all my 5 commits into 1?
> Thank you very much.

No button, do it manually.
https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/

Google will help you.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 52 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #51)
> (In reply to Wei Gao (Spreadtrum) from comment #50)
> > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #49)
> > > Comment on attachment 8453596 [details]
> > > pull request on master
> > > 
> > > Please squash your 5 commits into 1 before merging
> > 
> > Hi Alive
> > 
> > I am sorry to trouble.
> > But should I have to recommit a PR or is there a button in github to squash
> > all my 5 commits into 1?
> > Thank you very much.
> 
> No button, do it manually.
> https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/
> 
> Google will help you.

Then git push -f origin your-branch.

	Wei Gao (Spreadtrum) Assignee
	Comment 53 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #51)
> No button, do it manually.
> https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/
> 
> Google will help you.

Oh, yes, I have made it done.
Thanks for your suggestion.
https://github.com/mozilla-b2g/gaia/pull/21515

	Wei Gao (Spreadtrum) Assignee
	Comment 54 • 10 years ago

Dear Alive

There still have this question?
TEST-UNEXPECTED-FAIL | apps/system/js/system_dialog.js | line 116, col 26, 'KeyboardManager' is not defined.

I don't know what's the matter?
Could you help me please?
Thank you.
https://github.com/mozilla-b2g/gaia/pull/21515

	GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]
	Comment 55 • 10 years ago

Hi Wei Gao,
   Since |system_dialog.js| uses KeyboardManger in this patch, so each test cases relate to |system_dialog.js| need mock KeyboardManager.

   For example, you need to add same script what you added in |system_dialog_test.js| into 'fxa_dialog_test'.

  requireApp('system/test/unit/mock_keyboard_manager.js');

  var mocksForFxAccountsDialog = new MocksHelper([
    'AppWindowManager',
    'LayoutManager',
    'SystemDialogManager',
    'KeyboardManager'
  ]).init();

Thanks.

	Evelyn Hung
	Comment 56 • 10 years ago

(In reply to Wei Gao (Spreadtrum) from comment #54)
> Dear Alive
> 
> There still have this question?
> TEST-UNEXPECTED-FAIL | apps/system/js/system_dialog.js | line 116, col 26,
> 'KeyboardManager' is not defined.
> 
You introduce KeyboardManager without saying it's a global variable, so jshint doesn't not recognize it. Please add a comment in the file to explicitly say it's a global variable, like what this link did.
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/layout_manager.js#L1

	Wei Gao (Spreadtrum) Assignee
	Comment 57 • 10 years ago

(In reply to GaryChen [:GaryChen][:PYChen][:陳柏宇] from comment #55)
> Hi Wei Gao,
>    Since |system_dialog.js| uses KeyboardManger in this patch, so each test
> cases relate to |system_dialog.js| need mock KeyboardManager.
> 

(In reply to Evelyn Hung [:evelyn] from comment #56)
> You introduce KeyboardManager without saying it's a global variable, so
> jshint doesn't not recognize it. Please add a comment in the file to
> explicitly say it's a global variable, like what this link did.
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> layout_manager.js#L1

Dear GaryChen and Evelyn

Thanks for your so kindly help. That's perfectly correct.
Everything you mentioned above should all be done.
I am glad to see the CI test pass now.
Thanks for your suggestion again.

	Wei Gao (Spreadtrum) Assignee
	Comment 58 • 10 years ago

Comment on attachment 8453596 [details]
pull request on master

Hi Alive

I think it can work normal now.
Could you help to review again.
Thanks a great.

	Wei Gao (Spreadtrum) Assignee
	Comment 59 • 10 years ago

Hi Julien

I have made a PR on master, what should I do next?
Is a checkin-needed flag needed for it?
But I can't find this flag.
Could you help me?
Thanks a lot.

	Julien Wajsberg [:julienw]
	Comment 60 • 10 years ago

The flag is in the "keywords" section at the top of the bug.

	Wei Gao (Spreadtrum) Assignee
	Comment 61 • 10 years ago

(In reply to Julien Wajsberg [:julienw] (PTO Monday 14th July) from comment #60)
> The flag is in the "keywords" section at the top of the bug.

Oh, I see, Your are so kind.
I am very glad to get your help.
Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 10 years ago

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

Not sure if this is wanted for v2.0 or not.

	Julien Wajsberg [:julienw]
	Comment 63 • 10 years ago

I think it is, we'd need an approval then.

Wei, can you please do the approval request for v2.0 on the master patch? I think you know the process now :)

	Wei Gao (Spreadtrum) Assignee
	Comment 64 • 10 years ago

(In reply to Julien Wajsberg [:julienw] (PTO Monday 14th July) from comment #63)
> I think it is, we'd need an approval then.
> 
> Wei, can you please do the approval request for v2.0 on the master patch? I
> think you know the process now :)

Yes, I know how to do that.
Well, I owe it all to your kindly help. :)
I will do it now.

	Wei Gao (Spreadtrum) Assignee
	Comment 65 • 10 years ago

Comment on attachment 8453596 [details]
pull request on master

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):-
[User impact] if declined:When pin page for sim1 show, the OK/Cancel button is on the top of keyboard. User can click OK when he input the pin number directly. But when pin page for sim2 show, the OK/Cancel button can't be found, as it is hidden by keyboard. I think the user will consider it very strange.
[Testing completed]:yes, pass on tarako(6821) and dolphin(7715).
[Risk to taking this patch] (and alternatives if risky):I think there is no risk for 2.0. When refresh the interface, the height of keyboard originally should be considered.
[String changes made]:no

	Wei Gao (Spreadtrum) Assignee
	Comment 66 • 10 years ago

Hi Julien

I am sorry to trouble you, but I have deleted my "weigao123:Bug980217-master" branch which is related to |pull request on master|.
https://github.com/mozilla-b2g/gaia/pull/21515

Do I need to recommit a PR? or it can still be merged on 2.0?
That's my wonder?
Could you help me?
Thanks.

	Julien Wajsberg [:julienw]
	Comment 67 • 10 years ago

Don't worry, there is no need to create a new PR, we can uplift using the commit in comment 62 :)

	Ryan VanderMeulen [:RyanVM]
	Comment 68 • 10 years ago

v2.0: https://github.com/mozilla-b2g/gaia/commit/041c794bdce3a81767d1c60e776227f5c4d42651

	Ryan VanderMeulen [:RyanVM]
	Comment 69 • 10 years ago

Accidentally left a stray line in system_dialog.js that angered the CI gods.
v2.0: https://github.com/mozilla-b2g/gaia/commit/6ccd97a605e7f4ccad2fa92ac9575b35028e07a7

	Wei Gao (Spreadtrum) Assignee
	Comment 70 • 10 years ago

Dear Julien

Maybe this patch doesn't suit master/v2.0 as system_dialog has been restructured. But v1.3t and v1.4 can work fine. I don't know why that happened.
So could I revert the commit on master/v2.0 firstly?
I am sorry to make so much trouble.

https://bugzilla.mozilla.org/show_bug.cgi?id=1041528

	Julien Wajsberg [:julienw]
	Comment 71 • 10 years ago

Alive, can you have a look here?

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 72 • 10 years ago

v2.0 backout: 
b11775fcbfe076a3fc560c2041f5b2fe1b345009
a25a63310c294e79610f04d9df7bc38b329dee88

Marking this as won't fix on 2.0 branch -- we can decide the blocker status on 2.0 on the cloned bug.

	Shally Li（leave from Mozilla）
	Comment 73 • 10 years ago

This issue has been successfully verified on Flame 2.1.
See attachment: verified_v2.1.mp4
Reproducing rate: 0/5

Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880