949905 - [fugu][buri] a white screen appears after tapping on "Passcode Lock" multiple times

Status: RESOLVED FIXED

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

Description

[Peipei Cheng (needinfo if you need my action)]

Attachment: STR.3gp

This can be reproduced on both v1.2 and v1.3.

STR:
1. go to Settings -> Screen lock
2. Enable Lock screen
3. Quickly click "Passcode lock" for several times
   --> A white screen shows up.

Please see attached video for details.

Comment 1

[ying.xu]

transition was error if we click the screen during a transition.

In file settings.css，change transition time to 5s or more longer.
Would see the bug obviously.

section[role="region"] {
  visibility: hidden;
  transform: translateX(+100%);
  transition: transform 5s linear, visibility 5s linear;
}

Comment 2

[Jesse]

Attachment: too_many_clicks_hang_up_settings.patch

Tzu-lin,

    We need prevent users from interrupting the transition of panels. Do you think it's a right way?

    Besides, we should wait until transition end to show keyboard , not immediately ( cause a significant white screen).

Comment 3

[James Zhang (Spreadtrum)]

It's fugu v1.2 major bug.

Comment 4

[Steven Yang [:styang]]

Evelyn, could you take a look on this issue?

Comment 5

[Evelyn Hung]

(In reply to Jesse from comment #2)
> Created attachment 8349157 [details] [diff] [review]
> too_many_clicks_hang_up_settings.patch
> 
> Tzu-lin,
> 
>     We need prevent users from interrupting the transition of panels. Do you
> think it's a right way?
> 
I don't think we need this trick.

>     Besides, we should wait until transition end to show keyboard , not
> immediately ( cause a significant white screen).

The root case is here. yes, we need to wait panel transition finished to set input focus.

Comment 6

[Evelyn Hung]

Comment on attachment 8349157 [details] [diff] [review]
too_many_clicks_hang_up_settings.patch

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

I think we don't need `pointer-events: none` trick to prevent user input while panel is transitioning. Please keep panelready listener and callback but remove other modification. Thanks.

::: apps/settings/js/phone_lock.js
@@ +128,5 @@
>    },
>  
> +  onPasscodePanelLoad: function onPasscodePanelLoad(e){
> +    window.removeEventListener('panelready', onPasscodePanelLoad);
> +    if (e.detail.current === '#phoneLock-passcode' && Settings.currentPanel === '#phoneLock-passcode') {

`e.detail.current` and `Settings.currentPanel` should be always equal.

@@ +129,5 @@
>  
> +  onPasscodePanelLoad: function onPasscodePanelLoad(e){
> +    window.removeEventListener('panelready', onPasscodePanelLoad);
> +    if (e.detail.current === '#phoneLock-passcode' && Settings.currentPanel === '#phoneLock-passcode') {
> +      PhoneLock.passcodeInput.focus();

Try no using object name directly in the code.

@@ +137,5 @@
>    changeMode: function pl_changeMode(mode) {
>      this.hideErrorMessage();
>      this.MODE = mode;
>      this.passcodePanel.dataset.mode = mode;
> +    window.removeEventListener('panelready', this.onPasscodePanelLoad);

should remove event listener immediately after event is fired.

@@ -134,2 @@
>      if (Settings.currentPanel != '#phoneLock-passcode') {
> -      Settings.currentPanel = '#phoneLock-passcode'; // show dialog box

Why you remove this line?

@@ +142,4 @@
>      if (Settings.currentPanel != '#phoneLock-passcode') {
> +      window.addEventListener('panelready', this.onPasscodePanelLoad);
> +      Settings.currentPanel = '#phoneLock-passcode';
> +    } else {

There is no else case here, every time when the dialog shows, we need to listen to this `panelready` event.

Comment 7

[Jesse]

There is still a little possibility that the bug show up again , if we only defer the focus event.

Thus I add pointer-event to prevent any action from users to workaround the problem.

It's a gecko bug,but we need a workaround on v1.2 branch.

Sorry for the redundance in my patch, I will remove some lines if you agree with me on adding 'pointer-event'. Otherwise please find a better solution to fix the issue without adding 'pointer-event'.


(In reply to Evelyn Hung [:evelyn] from comment #6)
> Comment on attachment 8349157 [details] [diff] [review]
> too_many_clicks_hang_up_settings.patch
> 
> Review of attachment 8349157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we don't need `pointer-events: none` trick to prevent user input
> while panel is transitioning. Please keep panelready listener and callback
> but remove other modification. Thanks.
> 
> ::: apps/settings/js/phone_lock.js
> @@ +128,5 @@
> >    },
> >  
> > +  onPasscodePanelLoad: function onPasscodePanelLoad(e){
> > +    window.removeEventListener('panelready', onPasscodePanelLoad);
> > +    if (e.detail.current === '#phoneLock-passcode' && Settings.currentPanel === '#phoneLock-passcode') {
> 
> `e.detail.current` and `Settings.currentPanel` should be always equal.
> 
> @@ +129,5 @@
> >  
> > +  onPasscodePanelLoad: function onPasscodePanelLoad(e){
> > +    window.removeEventListener('panelready', onPasscodePanelLoad);
> > +    if (e.detail.current === '#phoneLock-passcode' && Settings.currentPanel === '#phoneLock-passcode') {
> > +      PhoneLock.passcodeInput.focus();
> 
> Try no using object name directly in the code.
> 
> @@ +137,5 @@
> >    changeMode: function pl_changeMode(mode) {
> >      this.hideErrorMessage();
> >      this.MODE = mode;
> >      this.passcodePanel.dataset.mode = mode;
> > +    window.removeEventListener('panelready', this.onPasscodePanelLoad);
> 
> should remove event listener immediately after event is fired.
> 
> @@ -134,2 @@
> >      if (Settings.currentPanel != '#phoneLock-passcode') {
> > -      Settings.currentPanel = '#phoneLock-passcode'; // show dialog box
> 
> Why you remove this line?
> 
> @@ +142,4 @@
> >      if (Settings.currentPanel != '#phoneLock-passcode') {
> > +      window.addEventListener('panelready', this.onPasscodePanelLoad);
> > +      Settings.currentPanel = '#phoneLock-passcode';
> > +    } else {
> 
> There is no else case here, every time when the dialog shows, we need to
> listen to this `panelready` event.

Comment 8

[Evelyn Hung]

(In reply to Jesse from comment #7)
> There is still a little possibility that the bug show up again , if we only
> defer the focus event.
> 

Can you explain more about the possibility? and if so, I guess the same problem happens on SIM pin panel.

Comment 9

[Jesse]

Attachment: too_many_clicks_cause_settings_bad_ui.patch

It's a gecko issue that I have not dove into. So I can't give u an exact explanation about the possibility. But it does reproduce the issue if we only 
setup the correct focus action. 

I did many trials in gaia,finally I had no choice but to workaround it by preventing clicks while the transition.

Comment 10

[Evelyn Hung]

(In reply to Jesse from comment #9)
> Created attachment 8349814 [details] [diff] [review]
> too_many_clicks_cause_settings_bad_ui.patch
> 
> It's a gecko issue that I have not dove into. So I can't give u an exact
> explanation about the possibility. But it does reproduce the issue if we
> only 
> setup the correct focus action. 

Not sure how much possibility of the problem is if we simply delay focus setting after panel ready. I also tried many times but failed to reproduce it. If it's a gecko issue, do we have a bug for tracking this?

Anyway, I'm fine to take this patch for v1.2f, but please note there is a risk - if the transitionend is failed to fire or catch, the panel will be locked and user have to kill Settings to workaround the issue.

Comment 11

[Evelyn Hung]

Comment on attachment 8349814 [details] [diff] [review]
too_many_clicks_cause_settings_bad_ui.patch

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

Please address my comment below.

::: apps/settings/js/phone_lock.js
@@ +140,5 @@
>      this.hideErrorMessage();
>      this.MODE = mode;
>      this.passcodePanel.dataset.mode = mode;
>      if (Settings.currentPanel != '#phoneLock-passcode') {
> +      window.addEventListener('panelready', onPasscodePanelLoad);

Please add a comment to describe why we do this.

@@ +142,5 @@
>      this.passcodePanel.dataset.mode = mode;
>      if (Settings.currentPanel != '#phoneLock-passcode') {
> +      window.addEventListener('panelready', onPasscodePanelLoad);
> +      Settings.currentPanel = '#phoneLock-passcode';
> +    } else {

no else case here, please also remove the next line.

Comment 12

[Jesse]

1. We do have the else case here.  The older comments describe that sometimes we can't listen for the ontransitionend event because some of the passcode mode changes, such as edit->new, do not trigger transition events.
 
   if (Settings.currentPanel != '#phoneLock-passcode') {
      window.addEventListener('panelready', onPasscodePanelLoad);
      Settings.currentPanel = '#phoneLock-passcode';
    } else {
      setTimeout(function(self) { self.passcodeInput.focus(); }, 0, this);
    }

2. If we prevent users from touching anything,it's almost impossible that transitionend event failed to fire, at least it's harder to reproduce than the white screen issue. What do u think?

Comment 13

[Jesse]

Attachment: too_many_clicks_cause_settings_bad_ui.patch

Add comments.

Comment 14

[Evelyn Hung]

(In reply to Jesse from comment #12)
> 1. We do have the else case here.  The older comments describe that
> sometimes we can't listen for the ontransitionend event because some of the
> passcode mode changes, such as edit->new, do not trigger transition events.
>  
>    if (Settings.currentPanel != '#phoneLock-passcode') {
>       window.addEventListener('panelready', onPasscodePanelLoad);
>       Settings.currentPanel = '#phoneLock-passcode';
>     } else {
>       setTimeout(function(self) { self.passcodeInput.focus(); }, 0, this);
>     }
> 

By tracing the code, you will see in edit->new case, the keyboard never goes down, so I think it's no need to add this setTimeout. If you still want to, you should leave the comment intact.

> 2. If we prevent users from touching anything,it's almost impossible that
> transitionend event failed to fire, at least it's harder to reproduce than
> the white screen issue. What do u think?

"If we prevent users from touching anything,it's almost impossible that transitionend event failed to fire". To me, the statement is not correct. An event is failed to fire because of some reason, not only caused by user interrupt, but caused by going some wrong state in platform. I just raise my concern here.

Comment 15

[Evelyn Hung]

Comment on attachment 8349938 [details] [diff] [review]
too_many_clicks_cause_settings_bad_ui.patch

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

r+ for v1.2f. Thanks!

Comment 16

[Evelyn Hung]

Attachment: https://github.com/mozilla-b2g/gaia/pull/14947

For master and v1.3

Comment 17

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #15)
> Comment on attachment 8349938 [details] [diff] [review]
> too_many_clicks_cause_settings_bad_ui.patch
> 
> Review of attachment 8349938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for v1.2f. Thanks!

Merged into v1.2f: https://github.com/mozilla-b2g/gaia/commit/5212c61900f7f9259badbbe946a34c1b65dd57bf

Comment 18

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

Comment on attachment 8351306 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/14947

r=me. Thanks for the patch.

Comment 19

[Evelyn Hung]

merged into master: https://github.com/mozilla-b2g/gaia/commit/3e1a3a28ebae40191930f701f5786e5eebc3527b

Comment 20

[Kevin Grandon :kgrandon]

This commit introduced a few syntax errors which is causing travis to be red. Can we land a follow-up or revert it?

https://travis-ci.org/mozilla-b2g/gaia/builds/15920779

Comment 21

[Evelyn Hung]

(In reply to Kevin Grandon :kgrandon from comment #20)
> This commit introduced a few syntax errors which is causing travis to be
> red. Can we land a follow-up or revert it?
> 
> https://travis-ci.org/mozilla-b2g/gaia/builds/15920779

I think you were talking the commit for v1.2f: 
https://github.com/mozilla-b2g/gaia/commit/5212c61900f7f9259badbbe946a34c1b65dd57bf

a follow lint patch merged: https://github.com/mozilla-b2g/gaia/commit/b33c428e6b4ca7e699c51c433fe23aaaa3c655f8

Sorry for making travis red. :(

Comment 22

[yang.zhao]

hi,Evelyn
  could you please cherry pick this patch to v1.3t? thank you!(In reply to Evelyn Hung [:evelyn] from comment #21)
> (In reply to Kevin Grandon :kgrandon from comment #20)
> > This commit introduced a few syntax errors which is causing travis to be
> > red. Can we land a follow-up or revert it?
> > 
> > https://travis-ci.org/mozilla-b2g/gaia/builds/15920779
> 
> I think you were talking the commit for v1.2f: 
> https://github.com/mozilla-b2g/gaia/commit/
> 5212c61900f7f9259badbbe946a34c1b65dd57bf
> 
> a follow lint patch merged:
> https://github.com/mozilla-b2g/gaia/commit/
> b33c428e6b4ca7e699c51c433fe23aaaa3c655f8
> 
> Sorry for making travis red. :(