1125369 - [Messages][Edit] Users can prompt the delete confirmation screen multiple times at once

Status: VERIFIED FIXED

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

Description

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Multiple Delete Screen Logcat

Description:
When the user is deleting messeges or threads, they can be given the delete confirmation screen multiple times by double or triple tapping on the trash can icon.

Prerequisite: Have at least 1 message thread in the messages app

Repro Steps:
1) Update a Flame to 20150123010227
2) Open Messages app> Tap "..."
3) Choose "Select Threads"
4) Select 1 or more threads
5) Double, or triple tap on the trash can Icon


Actual:
User is shown 2-3 prompts of the delete confirmation screen

Expected:
The delete confirmation screen is shown immediatley after tapping on the icon, and the user is only shown 1 confirmation screen.

Environmental Variables:
Device: Flame 3.0 (319Mb)(Kitkat)(Full Flash)
Build ID: 20150123010227
Gaia: cba2f0bf49b882e0044c3cc583de8fcf83d2ffa4
Gecko: 494632b9afed
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Repro frequency: 15/15 100%
See attached: Logcat, Video - http://youtu.be/rIsYxxtp-4w

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This issue DOES occur on Flame 2.2

User is shown 2-3 prompts of the delete confirmation screen

Environmental Variables:
Device: Flame 2.2 (319Mb)(Kitkat)(Full Flash)
BuildID: 20150123002505
Gaia: 237008137f6d72b9cad25ff4faff14ff2c40ac63
Gecko: be24dd482a83
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (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 occur on Flame 2.1

The delete confirmation screen is shown immediatley after tapping on the icon, and the user is only shown 1 confirmation screen.

Environmental Variables:
Device: Flame 2.1 (319Mb)(Kitkat)(Full Flash)
Build ID: 20150123001230
Gaia: 234ec27050481f4787f1f4750ec26f62ce5cc2c0
Gecko: 4cdcc0e85fc0
Gonk: e7c90613521145db090dd24147afd5ceb5703190
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]]

Derek, what's the user impact here?  If we delete on the first confirmation and cancel on the second, what happens?  Or vice versa?

Comment 3

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Oops, thought I included this in the description. There are 3 possible outcomes here:

1.) If the user cancels all 3 confirmation screens, they will be brought back to the edit screen as expected.

2.) If the user chooses delete on 1 of the confirmation screens and cancels the rest, the threads/messages will be deleted as expected

3.) If the user chooses delete on 2 or more of the confirmation screens, they will be stuck on a screen that says "Deleting..." until the app is force closed.

Comment 4

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Functional regression that can lead to a broken state, see outcome 3) in Comment 3.

Requesting a window.

Comment 5

[Steve Chung [:steveck]]

(In reply to Peter Bylenga [:PBylenga] from comment #4)
> [Blocking Requested - why for this release]:
> Functional regression that can lead to a broken state, see outcome 3) in
> Comment 3.
> 
> Requesting a window.

I think it might relate to bug 1105857, because it replace the native comfirm dialog with ordinary html element, which will not block the user behavior before any reply from dialog.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

Moving to the correct component.

Comment 7

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

triage: regression

Comment 8

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #5)
> (In reply to Peter Bylenga [:PBylenga] from comment #4)
> > [Blocking Requested - why for this release]:
> > Functional regression that can lead to a broken state, see outcome 3) in
> > Comment 3.
> > 
> > Requesting a window.
> 
> I think it might relate to bug 1105857, because it replace the native
> comfirm dialog with ordinary html element, which will not block the user
> behavior before any reply from dialog.

Nope, we didn't use native confirm for thread deletion.+ I see that the issue existed before we landed patch for bug 1105857, as far as I can tell patch for bug 1091299 is the culprit. And I'm afraid that we have similar issues in other places as well.

Hey Manuel, could you please look into the issue we have? Looks like it's related to your patch for bug 1091299?

But in general I think our button handlers should be smart enough to avoid this issue.

Thanks!

Comment 9

[Julien Wajsberg [:julienw]]

Either this bug or bug 1078295.

Comment 10

[Pi Wei Cheng [:piwei] (inactive)]

b2g-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141112043812
Gaia: c1f97d511b199998ce002d1c2c1fa78435687141
Gecko: e0b32a35fe6f
Version: 36.0a1 (2.2 Master) 
Firmware Version: L1TC100118D0
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141112045811
Gaia: 4b84873a4bef3e1d37515e0363a931a561aa8adb
Gecko: 624487c0693b
Version: 36.0a1 (2.2 Master) 
Firmware Version: L1TC100118D0
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Gaia & Last Working Gecko - issue DOES repro
Gaia: 4b84873a4bef3e1d37515e0363a931a561aa8adb
Gecko: e0b32a35fe6f

First Broken Gecko & Last Working Gaia - issue does NOT repro
Gaia: c1f97d511b199998ce002d1c2c1fa78435687141
Gecko: 624487c0693b

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/c1f97d511b199998ce002d1c2c1fa78435687141...4b84873a4bef3e1d37515e0363a931a561aa8adb


Confirmed that patches for Bug 1091299 is the cause of this issue.

Comment 11

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

Carsten, can you take a look at this please? Looks like the fix that landed for Bug 1091299 may be the culprit here.

Comment 12

[Carsten Book [:Tomcat]]

Ktucker: i needinfo now the guys working on this one patch. i did only the checkin as part of sheriffduty task but i'm not a developer :)

Comment 13

[Manuel Casas Barrado [:mancas]]

Attachment: Proposed patch

Julien, this issue is happening because the pointer-events that we added in bug 1091299 are being ignored. The tabs button has its own pointer-events defined so this rule is more specific than the one we added to the body so this ones is no taken into account.

Until we make the refactor of all dialogs to make a single instance of an object, we could use this workaround if you want.

Thanks

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8555068 [details] [review]
Proposed patch

The same issue actually happens in the thread.

Instead of adding more code to workaround an issue in the building blocks, let's see if we can remove the pointer-events rules in the BB :)

Hey Pavel, do you remember why you added such pointer-events properties in the tabs.css (comes bug https://github.com/mozilla-b2g/gaia/pull/14444) ?

Comment 15

[Julien Wajsberg [:julienw]]

Pavel, Kaze told me it was probably to prevent the user from selecting the text. If that's the case we should probably replace with user-select :)

Comment 16

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I can't remember ... but if that was the case ... user-select is better :)

Comment 17

[Manuel Casas Barrado [:mancas]]

I agree with you, however, I've found a barrier in the global css style. If you look at |edit_mode.css5| you'll see that the form which contains the tab buttons, has |pointer-events: none| because of that, we need to set the buttons property to |initial| if we want that the events go through the buttons.

In that case, the body property, set when the dialog is transitioning, will not have any effect on the buttons, and the issue could be reproducible. So, my question is, can we change the |edit_mode| css?

Thanks

Comment 18

[Julien Wajsberg [:julienw]]

We can change the edit_mode.css of course but we need to take care to not regress something. Please look at the blame to know when and why the pointer-events were added.

Keeping needinfo (I wanted to look at it myself but couldn't find the time today).

Comment 19

[Manuel Casas Barrado [:mancas]]

Julien, as you can see, the property was added from the beginning of the file. I'm going to talk with Arnau, to make a deep research

https://github.com/mozilla-b2g/gaia/commit/1f673e6685441c73cc914d8601efa193ae004423#diff-f64fff0436785d8b4e746672e6887a4eR16

Comment 20

[Manuel Casas Barrado [:mancas]]

Per offline discussion with Arnau, the pointer-events property in the |edit_mode| file should not be changed because it is a modal and must prevent the click event go through the elements below the modal.

So the solution is to catch the event in the button and prevent other click until the transition has ended.

Do you agree Julien?

Comment 21

[Julien Wajsberg [:julienw]]

To be honest I don't really understand why the edit mode should be a modal like this. We could just show the gaia-header on top, the buttons on bottom, and that's all. The buttons are already in "position: absolute", we could do the same for gaia-header, and keep the "edit form" element with a "height: 0" somewhere.

However this is more than we'd want to do here. So I'm fine with your solution. Maybe we'd need to do the same in other applications too, would you have the time to have a look?

Also I know we'll likely remove the confirmation dialog at one moment, so I don't really mind adding more code around this if we'll remove it later.

Comment 22

[Manuel Casas Barrado [:mancas]]

I agree with you. Of course this fix is necessary in other apps so, we could put the "Util code" somewhere in shared folder in order to make it accessible for every app that need it. Notice that we have this problem in the majority of our buttons. See Bug 1125409 for another case of this issue.

WDYT?

Comment 23

[Julien Wajsberg [:julienw]]

I thought a little more of this and I had 2 more ideas:

Idea 1:
We could redo the edit_mode way of working.
Instead of having absolutely positioned elements inside a absolutely full-screen element with "pointer-events: none" we could have an element with 3 parts using a flexible layout.
Only the middle part would get "pointer-events: none" and as a result we wouldn't need to override with "pointer-events: auto" in the top and bottom parts.

I don't think it would be a good idea to do this in v2.2 but it could be for master. NI Pavel for more thought.

Idea 2:
In the past, we were setting `style.pointerEvents = "none"` directly on the element to prevent a costly restyle that would make us miss the animation. But now we have bug 927349 (also in v2.2) that should prevents this. So we could use a cleaner CSS class.
How about adding a "dialog-animating" class to <body>, and using this class to set "pointer-events: none" where needed?

We can also have a second class "disable-when-dialog-animates" (or something like this) to add whenever we need it, and have a single CSS rule ".dialog-animating .disable-when-dialog-animates { pointer-events: none; }"


Tell me what you think about it !

Comment 24

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I like Idea 1. I can work on it. I just need to talk with Julien about what exactly we need.

P.S. Julien, I will ping you on IRC tomorrow for more info

Comment 25

[Manuel Casas Barrado [:mancas]]

Hey Pavel, are you working on this issue?

Comment 26

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8555068 [details] [review]
Proposed patch

Hey Julien, this patch is for v2.2. If we are going to implement the second idea, we need to fix the issue in a special patch for branch 2.2

Due to the fact that we have a lot of bugs related to this issue, I've moved the utility function that prevent the pointer events in a DOM element, to a shared file in order to make it accessible from every app that need it.

Could you take a look at the patch? Once you give me your approval, I will fix the unit test and add a new ones.

Thanks!

Comment 27

[Julien Wajsberg [:julienw]]

Manuel, in comment 23 for idea 2, I advised to use a class on body because now we can. Do you think it's a bad idea?

Comment 28

[Noemí Freire (:noemi)]

Per comment 27, ni to Manuel. Thanks!

Comment 29

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Manuel,

not yet I will try to take a look at this next week if you still need me

Comment 30

[Manuel Casas Barrado [:mancas]]

Julien, I prefer to add the css class instead of use the style directly on the DOM. I've added the class but what you should focus on the shared file and the utility function to make it general and accessible for everyone.

Also, notice that the css class are located in the |root.css| file inside the SMS App. I want to move this class to a shared file, but where is the best place to put these rules? WDYT if we create a new file more general like |dialogs.css| or something like that to this purpose.

Thanks again for your time! =)

Comment 31

[Noemí Freire (:noemi)]

Hi,

Bug 1130294 has been created to work on Idea 1 gathered in Comment 23. Thanks!

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8555068 [details] [review]
Proposed patch

Answered on github

I think this is the right approach until we change how the edit_forms are done. But this could be more simple :)

Comment 33

[Manuel Casas Barrado [:mancas]]

Thanks for your comments Julien.

I left some answers on github, please take a look. I'm still thinking that the css we use here should be place somwhere in the shared folder. Remember that we have other bugs like this one, that can be reproduce in other apps, not only in the SMS app.

Comment 34

[Julien Wajsberg [:julienw]]

Hey Manuel,

I know the option menu (in shared) also uses the same trick. Maybe you can change also shared/js/option_menu.js and add this bit of CSS in shared/style/option_menu.css? Then in the SMS app we could just use the class, but we don't need the additional CSS.

Would this work for you?

(sorry for the delay, I'm in the middle of a work meetup during 2 weeks...)

Comment 35

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8555068 [details] [review]
Proposed patch

Thanks for your response Julien. Now, this patch will solve every issue related with the multiples dialogs, just using the css class.

Please take a look when you get a chance

Comment 36

[Julien Wajsberg [:julienw]]

Comment on attachment 8555068 [details] [review]
Proposed patch

This looks good but you missed one case: the "delete" button in a thread (as opposed to the thread list).

You'll also need to update unit tests, see https://treeherder.mozilla.org/logviewer.html#?job_id=503797&repo=gaia-try

And add some for dialog.js too :)

Thanks !

Comment 37

[Manuel Casas Barrado [:mancas]]

Good catch Julien. I took into account your comments about the unit test and I've fixed them. However, dialog_test also cover the transitioned case of the dialog, so what do you want to test in addition?

Comment 38

[Julien Wajsberg [:julienw]]

I think it's fine, nothing more is needed :)

I'm surprised that the failures of option_menu_test.js didn't show up in the previous report...

Comment 39

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8555068 [details] [review]
Proposed patch

Ok Julien, could you give me r+ in order to land the patch? I'm also surprised about the option_menu test, I don't know what have happened but, in my local machine, the test was failing...

Comment 40

[Julien Wajsberg [:julienw]]

Comment on attachment 8555068 [details] [review]
Proposed patch

r=me for the SMS part

I'd like a second look from Chris Lord who did the initial patch in option_menu, and from Kevin and Pavel as /shared peers.

Some background for you guys.

* Chris: remember we used direct style changes on document.body so that we don't have restyles killing the animation? This is now working fine because as you probably know we actually wait for the restyle before starting the animation (in 2.2 as well). I checked various cases already, still I'd like that you check as well. Just to be sure we're not regressing anything :)

* Kevin, Pavel: "pointer-event: none" is inherited... unless it's overriden somewhere (like in the edit mode). So the new class "disable-when-dialog-animates" is a way to override the override... It's not really nice but it's the nicest I could find until we change how the edit mode is done, which is more than we're willing to do for v2.2.

Tell me what you think!

Comment 41

[Kevin Grandon :kgrandon]

Comment on attachment 8555068 [details] [review]
Proposed patch

My preference is to use a `disabled` attribute instead of the className which is a little bit *too* specific for me, and not as generalized as it could be.

I wouldn't block the review on it, but I would like to see what the opinions of Pavel/Chris on that as well.

Comment 42

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8555068 [details] [review]
Proposed patch

As Kevin mention already we can use a `disabled` attribute and here:
https://github.com/mozilla-b2g/gaia/pull/27697/files#diff-b6652bd58975a394536b092c73e787a5R17
It will be better if we can self explain why we do this

LGTM r+

Comment 43

[Julien Wajsberg [:julienw]]

Kevin, Pavel, I answered you here: https://github.com/mozilla-b2g/gaia/pull/27697/files#r25235632

I think you're missing the point...

Comment 44

[Chris Lord [:cwiiis]]

Comment on attachment 8555068 [details] [review]
Proposed patch

Just checked it out, everything seems to be in order :)

Comment 45

[Julien Wajsberg [:julienw]]

landed on master: 808e0fced54d63be88f9993ef12e16956beb9f54

Comment 46

[Julien Wajsberg [:julienw]]

Comment on attachment 8555068 [details] [review]
Proposed patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1091299 and other bugs
[User impact] if declined: when double tapping, we have twice the same dialogs
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 47

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/6ecd206b7e4b0ca95e12fbae2d9a3f360eae69bc

Comment 48

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

This issue is verified fixed for the latest Nightly 3.0 and 2.2 builds.

Actual Results:  The user cannot bring up multiple delete confirmations at the same time.

Environmental Variables:
Device: Flame 3.0 KK (Full Flash) (319 MB)
BuildID: 20150303010233
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 0b3c520002ad
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Environmental Variables:
Device: Flame 2.2 KK (Full Flash) (319 MB)
BuildID: 20150303002527
Gaia: 3d188c414e30acc392253d5389a42352fcfbc183
Gecko: c89aad487aa5
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0