1005866 - [Messages] Confirmation button when deleting messages and threads should be in Red and say "Delete"

Status: VERIFIED FIXED

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

Description

[Maria Angeles Oteo (:oteo)]

Attachment: Delete_threads.png

Tested in master
Hamachi
Gecko:6fafefc
Gaia:2650621

STRs:
1) Enter in SMS aplication.
2) Tap "..." button
3) Select "Delete messages" and select one/several thread/s
4) Tap "Delete" button

Actual result:
Screen : "Delete selected message threads?" and "Cancel" and "OK" blue button.


Expected result:
Screen: "Delete selected message threads? and "Cancel" and "Delete" red button. 
(see attached Delete_threads.png screenshot)
"OK" button within delete confirmation message screen should be red and the text should be "Delete" instead of "OK"

This action is risky so the user sould notice it clearly, and we will align the behaviour with the already implemented in other applications when removing call log entries, contacts or applicaions in the Homescreen where the botton is in in red and the text is "Delete"

Comment 1

[Maria Angeles Oteo (:oteo)]

Attachment: Delete_messages.png

Be aware that we need to fix this bug also when deleting messages inside a thread:

Current result:
Screen : "Delete selected messages?" and "Cancel" and "OK" blue button.

Expected result:
Screen: "Delete selected messages? and "Cancel" and "Delete" red button. 
(see attached Delete_messages.png screenshot)
"OK" button within delete confirmation message screen should be red and the text should be "Delete" instead of "OK"

Comment 2

[Noemí Freire (:noemi)]

Nominating to 2.0? in order to guarantee the consistency across the different apps.

Comment 3

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

triage: per comment2

Comment 4

[Fernando Campo (:fcampo)]

Attachment: Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

The approach is call the shared confirm.js so we are able to use the 'danger' class for the button.

Asking for f? to Jose cause he have experience with ConfirmDialog, and I'm getting a race condition on some tests, where I don't receive the event 'animationend', hence the dialog doesn't hide. Maybe we should use a timeout as fallback?

Comment 5

[Jose Manuel Cantera]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Fernando,

I don't think we need promises to show the dialog. That could be the cause why your tests are not working properly. 

thanks

Comment 6

[Fernando Campo (:fcampo)]

(In reply to Jose Manuel Cantera from comment #5)
> Comment on attachment 8430210 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732
> 
> Fernando,
> 
> I don't think we need promises to show the dialog. That could be the cause
> why your tests are not working properly. 
> 
> thanks
Oh, I just used them because it was the way you were dealing with the confirmation on contacts deletion [https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts_bulk_delete.js], and I wanted to sync the code style, but I'll change it to normal callback.
Thanks for the tip

Comment 7

[Fernando Campo (:fcampo)]

mmm, I actually solve the issue, didn't have anything to do with Promises but with animations.css

If you still think it's better not to use Promises tell me and I'll get rid of it, but now it's working with them.

Comment 8

[Jason Smith [:jsmith]]

This is definitely not a blocker. There's literally no user impact here at all. Polish at best.

Comment 9

[Jose Manuel Cantera]

(In reply to Fernando Campo (:fcampo) from comment #7)
> mmm, I actually solve the issue, didn't have anything to do with Promises
> but with animations.css
> 
> If you still think it's better not to use Promises tell me and I'll get rid
> of it, but now it's working with them.

I personally would prefer to ger rid of promises if they are not really needed

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Redirecting to Steve.

I'm not sure we should use this shared library.

On one side, it implements the specified animation for this piece of UX, and I don't like duplicating things.

On another side,
* its code is very tightly coupled to the DOM structure (no use of classes to find the various elements, only one ID is used)
* the "show" method has a lot of parameters without using an option object
* it's not unit tested.
* its localizability is not very good (it does not use data-l10n-id and so does not switch the language if the user changes the language) although I could live with this since the current approach does the same

My proposal would be to use a simple DOM element here, with the text already in data-l10n-id attributes, and simply show/hide, but possibly reuse the CSS for this. To me, only the last 40 lines are useful, sorry :/

Comment 11

[bhavana bajaj [:bajaj]]

(In reply to Jason Smith [:jsmith] from comment #8)
> This is definitely not a blocker. There's literally no user impact here at
> all. Polish at best.

I agree, removing this from the nom queue as this seems more of a polish issue. If we are ready by a low risk fix and this may not be fixed by FL date (June 9th) please seek approval for uplift consideration.

Comment 12

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] (away until 2nd June) from comment #10)
> Comment on attachment 8430210 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732
> 
> Redirecting to Steve.
> 
> I'm not sure we should use this shared library.
> 
> On one side, it implements the specified animation for this piece of UX, and
> I don't like duplicating things.
> 
> On another side,
> * its code is very tightly coupled to the DOM structure (no use of classes
> to find the various elements, only one ID is used)
> * the "show" method has a lot of parameters without using an option object
> * it's not unit tested.
> * its localizability is not very good (it does not use data-l10n-id and so
> does not switch the language if the user changes the language) although I
> could live with this since the current approach does the same
> 

Well, those are improvements that could be done in a follow-up patch for that library. 

> My proposal would be to use a simple DOM element here, with the text already
> in data-l10n-id attributes, and simply show/hide, but possibly reuse the CSS
> for this. To me, only the last 40 lines are useful, sorry :/

Comment 13

[Julien Wajsberg [:julienw]]

> Well, those are improvements that could be done in a follow-up patch for that library. 

What I'm saying is that I don't see the point of having this library in the first place.

Comment 14

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] (away until 2nd June) from comment #10)
> Comment on attachment 8430210 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732
> I'm not sure we should use this shared library.
> 
> On one side, it implements the specified animation for this piece of UX, and
> I don't like duplicating things.
> 
> On another side,
> * its code is very tightly coupled to the DOM structure (no use of classes
> to find the various elements, only one ID is used)
> * the "show" method has a lot of parameters without using an option object
> * it's not unit tested.
> * its localizability is not very good (it does not use data-l10n-id and so
> does not switch the language if the user changes the language) although I
> could live with this since the current approach does the same

I tend to agree with your POV. This ConfirmDialog seems still have room to be improved(and there is no unit test for this module)

> 
> My proposal would be to use a simple DOM element here, with the text already
> in data-l10n-id attributes, and simply show/hide, but possibly reuse the CSS
> for this. To me, only the last 40 lines are useful, sorry :/

How about reusing the Dialog module in sms app?

Comment 15

[Fernando Campo (:fcampo)]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Comments addressed, no more promises, and exchange 'shared/confirm_dialog' for the existing 'sms/dialog' (didn't realize you had that guys).

modifying the tests as you read :p
hope everything is fine now.

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

I still don't get why we so want do use a library while a simple markup would work fine. We don't have any dynamic content in this dialog, so I really don't understand the need for a library.

In the SMS app, we use the "Dialog" library when we have a dynamic content, like different items depending on user inputs. This is definitely not the case here.

So, I'll put feedback-, but if Steve is fine with the code, let's move forward.

Comment 17

[Fernando Campo (:fcampo)]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Comment on attachment 8430210 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732
> 
> I still don't get why we so want do use a library while a simple markup
> would work fine. We don't have any dynamic content in this dialog, so I
> really don't understand the need for a library.
Well, I don't have a strong opinion on what to use, unless there's a good reason (performance, code complexity, etc) for one or another. I simply thought that if the tool is already developed (Dialog), I'd rather use it than create new markup for something that is already done.

Plus, even with a simple markup now, we'll need to take care of showing/hiding, and animations at some point.

I understand your concerns using library shared/confirm.js because it's not localized and lack tests (fixable things), also understand that using sms/dialog we are overkilling using a completely dynamic approach, but honestly I think that creating something specific for this, when we have other tools (that we can improve) is a mistake. I don't want to reinvent the wheel;

> 
> In the SMS app, we use the "Dialog" library when we have a dynamic content,
> like different items depending on user inputs. This is definitely not the
> case here.
> 
> So, I'll put feedback-, but if Steve is fine with the code, let's move
> forward.
Again, my opinion is not strong on this as long as there's  backing it up, and I take feedback seriously. I'd rather get to an agreement instead of moving on over other people's opinions.

My proposal here is using shared/confirm.js, together with animations.js, that takes care og showing/hiding/animate problems.
Adding tests on SMS to check that everything is correctly called/shown/hidden.
Create a bug for fixing the library (adding proper l10n, options parameter)

Not sure about unit testing the library on it's own, outside sms app.

Comment 18

[Fernando Campo (:fcampo)]

Attachment: simpler approach - https://github.com/fcampo/gaia/commit/f7e359d44c302c2ca18bce0927ec4344b9138fcc

Anyway, I created a quick test following a more simple approach.
Markup plus hiding/showing. No animations. Localized.

I can encapsulate it on a separate file 'confirm-dialog.js' if you think is a better approach. But it's basically the same as shared/confirm.js, localised :p

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8435833 [details]
simpler approach - https://github.com/fcampo/gaia/commit/f7e359d44c302c2ca18bce0927ec4344b9138fcc

Yeah, I'd do more something like this, except that I'd add twice the markup (we have 2 different texts), and I'd want to add the event listeners once in "init". 

But let's wait for Steve's feedback. If he's ok with using the Dialog library then I am too. I don't have strong feelings.

Comment 20

[Jose Manuel Cantera]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

As the Dialog object is being used by other functionalities in the SMS app, I don't see any reason why we shouldn't use it here

Comment 21

[Borja Salguero [:borjasalguero]]

Hi! After reading your comments, I think it would be enough to use the 'Dialog' code, as it's well tested easy to plug & remove. Actually, if in the future we move to a common 'confirm' dialog from /shared (probably the current one with the improvements needed, or a new one which fulfill our need), it would be easy to change from one to another.

'Dialog' was build in order to reuse it, so I think that we could go with this solution by now, as Steve suggested.

Comment 22

[Steve Chung [:steveck]]

Sorry for the late reply,
The reason why I didn't prefer having additinal markup for dialog is we might have more dialogs in the future(like adding markAsRead action, that would become 4 additional dialogs for thread/conversation at most), and elements in DOM tree just occupies the memory no matter it's displayed or not(it might be trivial, please correct me if I'm wrong). So I didn't suggest that we create additional dialog for deletion at first because we already have reusable dialog widget here and it might be easier to maintain/migrate to shared confirm dialog like Borja said.

If the confirm & animation in BB is necessary for visual unifying in the long run, please add TODO comment for the deletion dialog.

Comment 23

[Julien Wajsberg [:julienw]]

I really think the markup is negligible here (what matters the most for us is not the fixed markup, but the dynamic markup). But ok :)

Comment 24

[Julien Wajsberg [:julienw]]

Also, remember that when you add/remove elements from the DOM, more work needs to be done than when simply changing styles.

Comment 25

[Fernando Campo (:fcampo)]

PR updated addressing Steve's comments.

TBH, I can go with whatever option, I see good and bad in all of them, but ultimately it is your decision guys, SMS is your domain and you know the code and the performance better. 

Whatever option suits you best, I'll try to code ;)

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Only some nits for the testing related part, thanks!

Comment 27

[Steve Chung [:steveck]]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Still some suggestion for the tests but overall is good for me. r=me and replace deepEqual with equal for assertion, thanks.

Comment 28

[Steve Chung [:steveck]]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Sorry, just saw a failed test on Travis and I left a comment for it.

Comment 29

[Fernando Campo (:fcampo)]

(In reply to Steve Chung [:steveck] from comment #28)
> Comment on attachment 8430210 [details] [review]
> Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732
> 
> Sorry, just saw a failed test on Travis and I left a comment for it.
Fixed (I think), got to wait for travis, as I updated my computer and I'm having problems running local tests.

Comment 30

[Fernando Campo (:fcampo)]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Travis green again :) can I haz my + ?

Comment 31

[Steve Chung [:steveck]]

Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

r=me, thanks for all the efforts!

Comment 32

[Steve Chung [:steveck]]

In master: c79d7bebaebf84f897d1513c7264577bee384ddb

Comment 33

[Fernando Campo (:fcampo)]

Attachment: Request for 2.0 uplift

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 #): no regression, just lack of unified style
[User impact] if declined: different styles on confirmation screens
[Testing completed]: unit testing, manual check on devices
[Risk to taking this patch] (and alternatives if risky): low risk, it's style change
[String changes made]: -

Comment 34

[Loli (:lolimartinezcr)]

Tested and working
2.1
Gecko-a3c4c53
Gaia-5f9f64a

Comment 35

[Loli (:lolimartinezcr)]

Pending resolve in 2.0

Comment 36

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/e12f57da45f0966ab5a0ad2ae29bf6caac12a5d1

Comment 37

[Loli (:lolimartinezcr)]

Tested and working
2.1
Flame
Gecko f5e1264
Gaia bf3fb88

2.0
Flame
Gecko d2e992a
Gaia bbcf53d