Closed Bug 980217 Opened 6 years ago Closed 5 years ago

[DSDS] 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.

Categories

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

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 wontfix, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 wontfix, b2g-v2.1 verified)

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.1 --- verified

People

(Reporter: echu, Assigned: wei.gao)

References

Details

(Whiteboard: dsdsrun1.4-1 [sprd305080] [partner-blocker] [dolphin])

Attachments

(5 files, 2 obsolete files)

Attached image 2014-03-06-15-48-13.png
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%
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.
This happens on Tarako as well.  I am guessing it may happen on 1.3.
(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?
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
But the "Skip" and "OK" buttons are missing above keypad on SIM 1 PIN sometimes
So you see like me :) (except it's nearly 100% for me).
blocking-b2g: --- → 2.0?
This also happens on v1.4.
David, can your team take this bug?
Flags: needinfo?(dscravaglieri)
This has been around multiple releases that have already shipped, so this isn't a blocker.
blocking-b2g: 2.0? → backlog
blocking-b2g: backlog → 1.4?
See comment 10 - this is not a blocker.
blocking-b2g: 1.4? → backlog
Hi Arvin, can you take a look?
tarako and dolphin also have this issue.
Assignee: nobody → arvin.zhang
Flags: needinfo?(arvin.zhang)
Whiteboard: dsdsrun1.4-1 → dsdsrun1.4-1 [sprd305080]
I will check it later.
Flags: needinfo?(arvin.zhang)
Attached patch bug980217_updateHeight.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(ehung)
Assignee: arvin.zhang → wei.gao
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.
Attachment #8429830 - Flags: review?(timdream)
Flags: needinfo?(ehung)
Attachment #8429830 - Flags: review?(timdream) → review?(alive)
Whiteboard: dsdsrun1.4-1 [sprd305080] → dsdsrun1.4-1 [sprd305080] [partner-blocker] [dolphin]
Attachment #8429830 - Flags: review?(alive) → review+
Hi Alive

Thanks for your review.
Can you help to land this patch on 1.3t and 1.4?
Thanks.
Flags: needinfo?(dscravaglieri) → needinfo?(alive)
Please create a pull request, it's not difficult.
Flags: needinfo?(alive)
Hi Alive

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

https://github.com/mozilla-b2g/gaia/pull/20358
Flags: needinfo?(alive)
Please ignore the pull request in Comment18, I will recommit later. 
Sorry.
Flags: needinfo?(alive)
Attached file pull request
Hi Alive

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

Thanks very much.
Attachment #8429830 - Attachment is obsolete: true
Attachment #8438963 - Flags: review?(alive)
Attached file pull request on v1.4
Hi Alive

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

Thanks.
Attachment #8438983 - Flags: review?(alive)
Attachment #8438963 - Flags: review?(alive) → review+
Attachment #8438983 - Flags: review?(alive) → review+
(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.
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.
Flags: needinfo?(alive)
(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.
Flags: needinfo?(alive)
blocking-b2g: backlog → 1.3T?
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.
Flags: needinfo?(wei.gao)
(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.
Flags: needinfo?(wei.gao)
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.
blocking-b2g: 1.3T? → 1.3T+
Comment on attachment 8438983 [details] [review]
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.
Attachment #8438983 - Flags: approval-gaia-v1.4?(wchang)
Comment on attachment 8438983 [details] [review]
pull request on v1.4

Redirecting to Lawrence as Preeti is away.
Please see Wei's approval request from previous comment.
Attachment #8438983 - Flags: approval-gaia-v1.4?(wchang) → approval-gaia-v1.4?(lmandel)
(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?
(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.
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?
Alive, is it ready to be merged on master?
Flags: needinfo?(alive)
Green so for sure.
Flags: needinfo?(alive)
Keywords: checkin-needed
master: https://github.com/mozilla-b2g/gaia/commit/02a7e1a73fffc9678db77635214c5cd218965f3b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S5 (4july)
(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.
Attachment #8438983 - Flags: approval-gaia-v1.4?(lmandel) → approval-gaia-v1.4+
Flags: needinfo?(ryanvm)
(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
Flags: needinfo?(cbook)
This will be done by sheriffs today (friday was a holiday in USA)
Flags: needinfo?(cbook)
(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.
(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
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm) → needinfo?(wei.gao)
Resolution: FIXED → ---
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.
Attachment #8452214 - Flags: review?(alive)
Flags: needinfo?(wei.gao)
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.
Attachment #8452214 - Flags: review?(alive) → review+
(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
You need to add a "needinfo" request to get someone to answer.

Alive, please see previous comment.
Flags: needinfo?(alive)
(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.
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.
Flags: needinfo?(alive)
Attached file pull request on master
> 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.
Attachment #8452214 - Attachment is obsolete: true
Attachment #8453596 - Flags: review?(alive)
Maybe there still have this question?

TEST-UNEXPECTED-FAIL | apps/system/js/system_dialog.js | line 116, col 26, 'KeyboardManager' is not defined.
Comment on attachment 8453596 [details] [review]
pull request on master

Please squash your 5 commits into 1 before merging
Attachment #8453596 - Flags: review?(alive) → review+
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #49)
> Comment on attachment 8453596 [details] [review]
> 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.
Flags: needinfo?(alive)
(In reply to Wei Gao (Spreadtrum) from comment #50)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #49)
> > Comment on attachment 8453596 [details] [review]
> > 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.
Flags: needinfo?(alive)
(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] [review]
> > > 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.
(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
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
Flags: needinfo?(alive)
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.
(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
(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.
Comment on attachment 8453596 [details] [review]
pull request on master

Hi Alive

I think it can work normal now.
Could you help to review again.
Thanks a great.
Attachment #8453596 - Flags: review+ → review?(alive)
Flags: needinfo?(alive)
Attachment #8453596 - Flags: review?(alive) → review+
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.
Flags: needinfo?(felash)
The flag is in the "keywords" section at the top of the bug.
Flags: needinfo?(felash)
(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.
Master: https://github.com/mozilla-b2g/gaia/commit/896dc88a58f14879cb4ce584f74904d132eef24e

Not sure if this is wanted for v2.0 or not.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(bbajaj)
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
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 :)
Flags: needinfo?(wei.gao)
(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.
Flags: needinfo?(wei.gao)
Comment on attachment 8453596 [details] [review]
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
Attachment #8453596 - Flags: approval-gaia-v2.0?(lmandel)
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.
Flags: needinfo?(felash)
Don't worry, there is no need to create a new PR, we can uplift using the commit in comment 62 :)
Flags: needinfo?(felash)
Attachment #8453596 - Flags: approval-gaia-v2.0?(lmandel) → approval-gaia-v2.0+
Flags: needinfo?(bbajaj)
Accidentally left a stray line in system_dialog.js that angered the CI gods.
v2.0: https://github.com/mozilla-b2g/gaia/commit/6ccd97a605e7f4ccad2fa92ac9575b35028e07a7
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
Flags: needinfo?(felash)
Alive, can you have a look here?
Flags: needinfo?(felash) → needinfo?(alive)
Flags: needinfo?(alive)
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.
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
Attachment #8453596 - Flags: approval-gaia-v2.0+
You need to log in before you can comment on or make changes to this bug.