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

VERIFIED FIXED in 2.2 S7 (6mar)

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: dharris, Assigned: mancas)

Tracking

({regression})

unspecified
2.2 S7 (6mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [3.0-Daily-Testing], URL)

Attachments

(2 attachments)

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
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(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?
Flags: needinfo?(pbylenga) → needinfo?(dharris)
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.
Flags: needinfo?(dharris)
[Blocking Requested - why for this release]:
Functional regression that can lead to a broken state, see outcome 3) in Comment 3.

Requesting a window.
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
QA Contact: pcheng
(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.
Moving to the correct component.
Component: Gaia::Contacts → Gaia::SMS
triage: regression
blocking-b2g: 2.2? → 2.2+
(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!
Flags: needinfo?(b.mcb)
Either this bug or bug 1078295.
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Carsten, can you take a look at this please? Looks like the fix that landed for Bug 1091299 may be the culprit here.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(cbook)
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 :)
Flags: needinfo?(cbook) → needinfo?(felash)
(Assignee)

Updated

4 years ago
Assignee: nobody → b.mcb
Flags: needinfo?(b.mcb)
(Assignee)

Comment 13

4 years ago
Posted file 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
Attachment #8555068 - Flags: review?(felash)
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) ?
Flags: needinfo?(felash) → needinfo?(pivanov)
Attachment #8555068 - Flags: review?(felash)
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 :)
I can't remember ... but if that was the case ... user-select is better :)
Flags: needinfo?(pivanov)
(Assignee)

Comment 17

4 years ago
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
Flags: needinfo?(felash)
See Also: → 1125409
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).
(Assignee)

Comment 19

4 years ago
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
(Assignee)

Comment 20

4 years ago
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?
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.
Flags: needinfo?(felash)
(Assignee)

Comment 22

4 years ago
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?
Flags: needinfo?(felash)
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 !
Flags: needinfo?(felash) → needinfo?(pivanov)
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
Flags: needinfo?(pivanov)
(Assignee)

Comment 25

4 years ago
Hey Pavel, are you working on this issue?
Flags: needinfo?(pivanov)
(Assignee)

Comment 26

4 years ago
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!
Attachment #8555068 - Flags: review?(felash)
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?
Per comment 27, ni to Manuel. Thanks!
Flags: needinfo?(b.mcb)
Hey Manuel,

not yet I will try to take a look at this next week if you still need me
Flags: needinfo?(pivanov)
(Assignee)

Comment 30

4 years ago
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! =)
Flags: needinfo?(b.mcb) → needinfo?(felash)
(Assignee)

Updated

4 years ago
Blocks: 1125409
See Also: → 1130294
Hi,

Bug 1130294 has been created to work on Idea 1 gathered in Comment 23. Thanks!
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 :)
Flags: needinfo?(felash)
Attachment #8555068 - Flags: review?(felash)
(Assignee)

Comment 33

4 years ago
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.
Flags: needinfo?(felash)
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...)
Flags: needinfo?(felash)
(Assignee)

Comment 35

4 years ago
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
Attachment #8555068 - Flags: review?(felash)
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 !
Attachment #8555068 - Flags: review?(felash)
(Assignee)

Comment 37

4 years ago
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?
Flags: needinfo?(felash)
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...
Flags: needinfo?(felash)
(Assignee)

Comment 39

4 years ago
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...
Attachment #8555068 - Flags: review?(felash)
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!
Attachment #8555068 - Flags: review?(pivanov)
Attachment #8555068 - Flags: review?(kgrandon)
Attachment #8555068 - Flags: review?(felash)
Attachment #8555068 - Flags: review+
Attachment #8555068 - Flags: feedback?(chrislord.net)
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.
Attachment #8555068 - Flags: review?(kgrandon) → review+
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+
Attachment #8555068 - Flags: review?(pivanov) → review+
Kevin, Pavel, I answered you here: https://github.com/mozilla-b2g/gaia/pull/27697/files#r25235632

I think you're missing the point...
Comment on attachment 8555068 [details] [review]
Proposed patch

Just checked it out, everything seems to be in order :)
Attachment #8555068 - Flags: feedback?(chrislord.net) → feedback+
landed on master: 808e0fced54d63be88f9993ef12e16956beb9f54
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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
Attachment #8555068 - Flags: approval-gaia-v2.2?
Target Milestone: --- → 2.2 S7 (6mar)
Attachment #8555068 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.