1166495 - [Settings][Screen lock] Passcode lock screen transition animation flows in the wrong direction

Status: VERIFIED FIXED

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

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20150519_1357.txt

Description:
A sub-menu page should appear from the right side of the screen. However, Passcode Lock page appears from the left side. The transition animation should flow consistently in the right direction.

Repro Steps:
1) Update a Flame to 20150519010201.
2) Navigate to Settings > Screen Lock, and select the Passcode Lock toggle.
3) Observe the transition animation.


Actual:
Passcode screen appears from the left side.

Expected:
Passcode screen appears from the right side.

Environmental Variables:
Device: Flame 3.0 (KK, 319mb, full flash)
Build ID: 20150519010201
Gaia: 762cbd16712484f93f485e89f5363686540a3db7
Gecko: f65cc0022a0e
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0


Repro frequency: 7/7
See attached: video clip, logcat
https://youtu.be/7gFCmle90Zo

Comment 1

[Yeojin Chung [:YeojinC]]

This issue also reproduces on Flame 2.2.

Result: Passcode screen appears from the left side.


Environmental Variables:
Device: Flame 2.2 (KK, 319mb, full flash)
Build ID: 20150519002500
Gaia: 732acec6f37d13ccea6b0ddc48904a53a2970894
Gecko: 1389e6b8c065
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

===============================
This issue does NOT reproduce on Flame 2.1.

Result: Passcode screen appears from the right side.

Environmental Variables:
Device: Flame 2.1 (KK, 319mb, full flash)
Build ID: 20150519001201
Gaia: c80865cb0bf73f1b97defbc646083b404feb3ac4
Gecko: 062ac5cbb41e
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Visible regression.

Requesting a window.

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Bug 1055897 seems to have caused this issue.

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.2
BuildID: 20141124012706
Gaia: 355cf9731c0bc55ad19f77807fdf29f7f0f2840b
Gecko: 35467a4f8b11
Version: 36.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken 
Environmental Variables:
Device: Flame 2.2
BuildID: 20141124015807
Gaia: 2196e28c907b7fcae37baed509622f242745ff00
Gecko: a0f60519fa38
Version: 36.0a1 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 355cf9731c0bc55ad19f77807fdf29f7f0f2840b
Gecko: a0f60519fa38

First Broken gaia / Last Working gecko - Issue DOES occur
Gaia: 2196e28c907b7fcae37baed509622f242745ff00
Gecko: 35467a4f8b11

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/355cf9731c0bc55ad19f77807fdf29f7f0f2840b...2196e28c907b7fcae37baed509622f242745ff00

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

EJ, can you take a look at this please? This might have been caused by the work done for bug 1055897.

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks, let me take a look at this bug first.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] EragonJ:bug-1166495 > mozilla-b2g:master

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8610344 [details] [review]
[gaia] EragonJ:bug-1166495 > mozilla-b2g:master

Arthur, can you give me some ideas about this patch ? For this patch, I am trying to add two more API so that dialogPanel can programmatically cancel / submit itself. And also, I totally re-wrote the panel in this patch.

Any idea would be appreciated and I'll add tests later when I get f+ for this patch, thanks !

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8610344 [details] [review]
[gaia] EragonJ:bug-1166495 > mozilla-b2g:master

Good job! Please check my comments in github.

Comment 10

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8610344 [details] [review]
[gaia] EragonJ:bug-1166495 > mozilla-b2g:master

Arthur, I just fixed all tests and updated the patch based on our offline discussion, can you help me review that ? Thanks !

Comment 11

[[e-mail]]

Since arthur and ej are inactive now, @evelyn could you take over the review? I could help fix the follow up issue.

Comment 12

[Evelyn Hung]

Sure, I may need a few days to catch up the whole logic. Thanks for following up this bug!

Comment 13

[Evelyn Hung]

Comment on attachment 8610344 [details] [review]
[gaia] EragonJ:bug-1166495 > mozilla-b2g:master

Steal this review from Arthur.

Comment 14

[Evelyn Hung]

Comment on attachment 8610344 [details] [review]
[gaia] EragonJ:bug-1166495 > mozilla-b2g:master

The patch can't be applied to current codebase. Fred, could you take this patch over and rebase it? Thank you!

Comment 15

[[e-mail]]

Sorry Evelyn I'm stocked in BT related bugs, will handle this issue as soon as I resolved the other issue.

Comment 16

[[e-mail]]

take it for track

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] gasolin:issue-1166495 > mozilla-b2g:master

Comment 18

[[e-mail]]

WIP: rebased to master. The PR removed panel/screen_lock_passcode

But there are some update there
https://github.com/mozilla-b2g/gaia/commits/master/apps/settings/js/panels/screen_lock_passcode

Will merge the difference then ask for review.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: Major UX inconsistency, QA recommend to block.

Comment 20

[[e-mail]]

Attachment: screenpasslock.png

The header UI seems changed from current view. (The left < has been changed to X and the transition from left has been changed to dialog transition)

Ask harly if its an expected change.

Comment 21

[Harly Hsu[:harly]]

Yes, this is the expected UI and transition. If we have ok/create/done at the right side of header, we should use X instead of <. Thanks

Comment 22

[[e-mail]]

Comment on attachment 8639677 [details] [review]
[gaia] gasolin:issue-1166495 > mozilla-b2g:master

the PR has been rebased and added PasscodeHelper code with tests. 

It works fine on device (with my limited knowledge about screenlock). Please help review it.

The commit has splited into EJ's original patch and mine, will squash to single commit while merge.

Comment 23

[[e-mail]]

@gerry this is a legacy but large patch, I've patched/tested it and seems works fine on device. Could you help pull the patch and test on device to help us get more confidence?

Comment 24

[Gerry Chang [:gchang]]

Hi Fred, 
We've run the passcode related regression tests for this patch and found 1 issue. The regression test suite is in http://mzl.la/1DP0e41
The issue is:
User can use incorrect passcode to disable "Passcode Lock" and "Lock Screen" item. Can you take a look at this?

Comment 25

[[e-mail]]

Gerry thanks for pre-test! I also found when passcode lock is set, user can 'change passcode', disable "Passcode Lock" or "Lock Screen" with any passcode.

Now the promise issue of _checkPasscode is fix, test on device and it works now.

Comment 26

[Evelyn Hung]

Comment on attachment 8639677 [details] [review]
[gaia] gasolin:issue-1166495 > mozilla-b2g:master

Overall the patch looks fine but there are some code movements which I'm not sure if it's necessary. Please check my comments on Github. Thanks.

Comment 27

[[e-mail]]

Comment on attachment 8639677 [details] [review]
[gaia] gasolin:issue-1166495 > mozilla-b2g:master

Thanks for review. As offline discussion I've removed the string changes from the patch and keep it a separate commit. I'll squash all commits once review is granted.

Please kindly review it again, thanks!

Comment 28

[Evelyn Hung]

Comment on attachment 8639677 [details] [review]
[gaia] gasolin:issue-1166495 > mozilla-b2g:master

Looks great, Fred. Please also create a remove-string bug and label it good-first-bug for newbies. Thank you.  :)

Comment 29

[[e-mail]]

squashed and merged https://github.com/mozilla-b2g/gaia/commit/ebd099a608587e205426143f5ac195034df28bbe

remove-string bug is also filed in bug 1195709, thanks!

Comment 30

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on the latest build of Flame KK 2.5 and Aires KK 2.5 by the STR in comment 0.

Actual results: Passcode screen has "X" icon instead of "<" icon for returning to the previous page  and is shown as a dialog transit.
See attachment: verified_FlameKK_v2.5.3gp
Reproduce rate: 0/10


Device: Flame KK 2.5 (Pass)
Build ID               20150819030215
Gaia Revision          1e1197e0e8e64307aa382ffba4711d1c661de7ca
Gaia Date              2015-08-18 16:54:35
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/f384789a29dcfd514d25d4a16a97ec5309612d78
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150819.063618
Firmware Date          Wed Aug 19 06:36:31 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5(Pass)
Build ID               20150820005800
Gaia Revision          89e0096a3de0378e3eda77e6a2a0bb5ca03eb8bb
Gaia Date              2015-08-19 18:28:14
Gecko Revision         https://hg.mozilla.org/integration/mozilla-inbound/rev/29b2df16e961fbe9a379362ecba6f888d1754bc3
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150820.002839
Firmware Date          Thu Aug 20 00:28:46 UTC 2015
Bootloader             s1

Comment 31

[Shally Li（leave from Mozilla）]

Attachment: verified_FlameKK_v2.5.3gp