Closed Bug 1005866 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S4 (20june)
blocking-b2g -
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: oteo, Assigned: fcampo)

Details

Attachments

(4 files, 1 obsolete file)

Attached image 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"
Attached image 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"
Summary: [Messages] Confirmation button when deleting messages and threads should be in Red and says "Delete" → [Messages] Confirmation button when deleting messages and threads should be in Red and say "Delete"
Nominating to 2.0? in order to guarantee the consistency across the different apps.
blocking-b2g: --- → 2.0?
triage: per comment2
blocking-b2g: 2.0? → 2.0+
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?
Assignee: nobody → fernando.campo
Status: NEW → ASSIGNED
Attachment #8430210 - Flags: review?(felash)
Attachment #8430210 - Flags: feedback?(jmcf)
Target Milestone: --- → 2.0 S3 (6june)
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
Attachment #8430210 - Flags: feedback?(jmcf) → feedback-
(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
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.
Flags: needinfo?(jmcf)
This is definitely not a blocker. There's literally no user impact here at all. Polish at best.
blocking-b2g: 2.0+ → 2.0?
(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
Flags: needinfo?(jmcf)
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 :/
Attachment #8430210 - Flags: review?(felash) → review?(schung)
(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.
blocking-b2g: 2.0? → -
(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 :/
> 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.
(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?
Attachment #8430210 - Flags: review?(schung)
QA Contact: lolimartinezcr
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.
Attachment #8430210 - Flags: review?(schung)
Attachment #8430210 - Flags: feedback?(jmcf)
Attachment #8430210 - Flags: feedback?(felash)
Attachment #8430210 - Flags: feedback-
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.
Attachment #8430210 - Flags: feedback?(felash) → feedback-
(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.
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
Attachment #8435833 - Flags: feedback?(felash)
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.
Attachment #8435833 - Flags: feedback?(felash)
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
Attachment #8430210 - Flags: feedback?(jmcf) → feedback+
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.
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
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.
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 :)
Also, remember that when you add/remove elements from the DOM, more work needs to be done than when simply changing styles.
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 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 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.
Attachment #8430210 - Flags: review?(schung) → review+
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.
Attachment #8430210 - Flags: review+
(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 on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

Travis green again :) can I haz my + ?
Attachment #8430210 - Flags: review?(schung)
Comment on attachment 8430210 [details] [review]
Link to PR - https://github.com/mozilla-b2g/gaia/pull/19732

r=me, thanks for all the efforts!
Attachment #8430210 - Flags: review?(schung) → review+
In master: c79d7bebaebf84f897d1513c7264577bee384ddb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attached file 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]: -
Attachment #8435833 - Attachment is obsolete: true
Attachment #8440550 - Flags: approval-gaia-v2.0?
Tested and working
2.1
Gecko-a3c4c53
Gaia-5f9f64a
Status: RESOLVED → VERIFIED
Status: VERIFIED → RESOLVED
Closed: 6 years ago6 years ago
Pending resolve in 2.0
Attachment #8440550 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Tested and working
2.1
Flame
Gecko f5e1264
Gaia bf3fb88

2.0
Flame
Gecko d2e992a
Gaia bbcf53d
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.