991396 - [camera][madai] delete dialogue button should be red and say delete

Status: RESOLVED FIXED

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

Description

[Tiffanie Shakespeare [:tif]]

Attachment: Incorrect format of delete dialogue

Step to Repro:
1. Take a photo or video
2. Tap preview icon
3. Tap delete

Expected
Delete dialogue shows cancel and red delete button (see gallery)

Actual
Delete dialogue shows cancel and blue ok button

This doesn't sound like a big deal, but should be high priority as it breaks consistency with our other delete dialogues that follow this pattern. Since the action is so destructive, we want the button to be red and confirm they are deleting.

Comment 1

[Hema Koka [:hema]]

wilsonpage: can you take this?

Comment 2

[hyuna.cho]

Attachment: PR-991396.html

I changed the dialog type to custom-dialog.
Please review it.

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8401710 [details]
PR-991396.html

r-, but just a nit on github.  I'd like to see a screenshot before giving final r+.

Also: we have another bug about the fact that this dialog always appears in portrait mode. It might be worth fixing that here in this bug. I don't know if we can do that and still use shared/js/CustomDialog.js and shared/js/confirm.css or if it is necessary to make modified local copies of those files.

If you can do it easily in this bug, I'd suggest you go ahead and do it so we can close two bugs with one patch.  But if not, I'm okay landing this patch with just the one nit addressed.

Comment 4

[hyuna.cho]

Attachment: screenshot_991396.png

It's a screenshot about the previous patch.

Comment 5

[hyuna.cho]

If we change some codes in view/overlay.js and controller/overlay.js, I think we can display all dialog in camera app.
Now overlay.js is for one button. I will upload the patch after fixing and checking.

Comment 6

[hyuna.cho]

I made a patch except landscape mode issue.
If I use 'transform' to display landscape dialog, I need position information.
How to get information of title/body position? I can't fine this in confirm.css.
If you have any idea except using transform, please let me know.

Comment 7

[David Flanagan [:djf]]

Hyuna,

As part of the patch I'm working on in 987569, I'm going to "unlock" screen orientation in the preview gallery so that orientation can rotate. This will automatically mean that the delete dialog and the share dialog will appear in their correct orientation. So assuming that the screenshot above looks good to Amy, let's just go with the simple CustomDialog solution you already have, rather than modifying overlay.js

I'll set the UI review flag on the screenshot.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8403106 [details]
screenshot_991396.png

Tif and Amy: I'm not sure which of you is the correct one to ask UI review on. I'd guess Amy, but Tif filed the bug, so I'm adding her to the review request, too.

The color and button text has obviously changed.  The dialog is designed to have a title (above the line, I think) and a message.  We're using the message field here which is closer to how it looked before. If you want the question above the line, we could use the title, though I think that actually shrinks the font size.  Or if you want some other text above the line, we can do that, too. (Though that would be a late-l10n string, probably)

Comment 9

[Rob MacDonald [:robmac]]

(In reply to David Flanagan [:djf] from comment #8)
> Comment on attachment 8403106 [details]
> 
> Tif and Amy: I'm not sure which of you is the correct one to ask UI review
> on. I'd guess Amy, but Tif filed the bug, so I'm adding her to the review
> request, too.

Hi David...

Amy is visual and Tif is interaction but in many instances there's overlap. Due to the urgency of these reviews, I encourage engineering to flag both Tif and Amy. Whoever is responsible will then reply.

Otherwise, if the wrong person is flagged, our response may be unnecessarily delayed.

Rob

Comment 10

[Tiffanie Shakespeare [:tif]]

Message above the line isn't needed. The screenshot looks good, but when I load the patch I see two lines (above and below "Delete photo?") instead of just one.

I also notice that it's not rotating - should that be happening in this patch?

Comment 11

[David Flanagan [:djf]]

Tif: I had asked Hyuna to consider adding rotation in this patch, but now (in comment 7) have asked her not to do that. Rotation will come with my patch in bug 987569, so this one should not rotate for now.

Comment 12

[Amy Lee [:amylee] UX]

(In reply to David Flanagan [:djf] from comment #8)
> Comment on attachment 8403106 [details]
> screenshot_991396.png
> 
> Tif and Amy: I'm not sure which of you is the correct one to ask UI review
> on. I'd guess Amy, but Tif filed the bug, so I'm adding her to the review
> request, too.
> 
> The color and button text has obviously changed.  The dialog is designed to
> have a title (above the line, I think) and a message.  We're using the
> message field here which is closer to how it looked before. If you want the
> question above the line, we could use the title, though I think that
> actually shrinks the font size.  Or if you want some other text above the
> line, we can do that, too. (Though that would be a late-l10n string,
> probably)

I agree with Tiff's comments.

Comment 13

[Tiffanie Shakespeare [:tif]]

Understood, thanks David.

Waiting for Hyuna's response about the double line before I can ui-review +/-.

(In reply to David Flanagan [:djf] from comment #11)
> Tif: I had asked Hyuna to consider adding rotation in this patch, but now
> (in comment 7) have asked her not to do that. Rotation will come with my
> patch in bug 987569, so this one should not rotate for now.

Comment 14

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Rebased patch that fixes the issue with two horizontal lines in the overlay.

Comment 15

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8405683 [details] [review]
pull-request (master)

My only nit is that the tap highlights on delete/cancel don't consistently work, but Justin looked at the CSS and verified that isn't the problem.

Comment 16

[Tiffanie Shakespeare [:tif]]

Opened follow up bug for my about nit
https://bugzilla.mozilla.org/show_bug.cgi?id=995532

Comment 17

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8405683 [details] [review]
pull-request (master)

https://github.com/mozilla-b2g/gaia/pull/18252

Comment 18

[Justin D'Arcangelo [:justindarc]]

Attachment: pull-request (master)

Updated PR. Carrying over ui-review+ from last PR.

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8405732 [details] [review]
pull-request (master)

Justin has finished up Hyuna's original version of the fix, which was simpler and cleaner than her second version. This just replaces the old call to confirm() with the use of shared/js/CustomDialog.js.  That shared file is known to work in other apps, and is a good choice here.

Comment 20

[David Flanagan [:djf]]

Tif: this version of the patch is different than the one you tested. Visually they are the same, but you might want to check the button behavior again because your nit may have disappeared.  The buttons seem to work well (on a 1.4 hamachi) to me.

Comment 21

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/e151331f5688d933fad4b9da067ffdd57653b733

Comment 22

[David Flanagan [:djf]]

Comment on attachment 8405732 [details] [review]
pull-request (master)

Should we uplift this? It is a minor visual change with very minor localization impact ("cancel" and "delete" strings).

Comment 23

[Hema Koka [:hema]]

Axel, is it okay to take this in from localization impact perspective...

Comment 24

[Tiffanie Shakespeare [:tif]]

It seems better, but still not 100%. I must have a light touch :)

(In reply to David Flanagan [:djf] from comment #20)
> Tif: this version of the patch is different than the one you tested.
> Visually they are the same, but you might want to check the button behavior
> again because your nit may have disappeared.  The buttons seem to work well
> (on a 1.4 hamachi) to me.

Comment 25

[Axel Hecht [:Pike]]

I don't think we should take the l10n change here.

Comment 26

[Tiffanie Shakespeare [:tif]]

Does that mean the patch won't make it in? 

This is actually a pretty big deal as it's breaking a UX pattern by confirming the delete action via the word delete - it eliminates any chance that the user may not know which button to tap. Looking at our other destructive action dialogues, we use a red button that says delete to make it super obvious to the user that by tapping this button the are in fact deleting the item.

Axel - is there any way we can get this patch in? Can we use translations from other apps that have this same dialogue? Thanks!

Comment 27

[Axel Hecht [:Pike]]

We can't use translations from other apps, that's kinda how the security stuff between apps works.

I suggest to look at it this way, you can have the consistent UX layout in English, or the current UX localized.

I know it's painful, but the UX of stuff landing this late will have a few rough spots around the edges.

Comment 28

[Tiffanie Shakespeare [:tif]]

Let me ask a follow up question - can we use translations from our own app? If we use delete in the same manner elsewhere, can we reuse that translation?

I'm referring to this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=992393

Which is using "delete" in an action menu.

Never hurts to ask - thanks Axel!

Comment 29

[Francesco Lodolo [:flod]]

As a general rule, string reuse is bad. 

In this specific case, you're asking to to reuse strings in different contexts (action item=lot of space, small font vs button=tiny space, maybe different case).

Last note: I don't see any "delete" or "cancel" string in 1.4, so that bug is in the same situation of this one
https://github.com/mozilla-b2g/gaia/blob/v1.4/apps/camera/locales/camera.en-US.properties

Comment 30

[Tiffanie Shakespeare [:tif]]

I want to reiterate why this is such an important bug to get into the release. 

When the user taps delete, the confirmation is presented to help avoid data loss. We’re doing a final check because there is no going back.

For usability reasons, the destructive button is red and contains the word delete. The red color alerts the user to the destructive nature of this button and the word “delete” provides more information as to what is going to happen. This acts as a warning flag and helps prevent the user from dismissively tapping ok to get close the dialogue. 

You may be asking why is this important? We have messaging asking “Delete photo?” so shouldn’t it be obvious that ok is deleting? It is well known that users don’t read - they skim or skip reading altogether and make snap decisions with little information. 

By retaining the friendly, blue ok button, people are going to tap it to get rid of the dialogue and then realize their error afterwards because they have just lost their favorite photo of grandma...forever. 

By making the seemingly insignificant change of a red button with delete we change the game. The user now pauses a little longer to consider the action. If in doubt, the user will tap cancel. The confident user will go ahead and tap delete and any stomach drop moments occur before data loss. This isn’t just a FFOS pattern, this is a mobile pattern and users have come to expect this for destructive things. 

Remember, there is no undo, because that is the confirmation dialogue’s job - to prevent mistakes. It can only do this job by conveying to the user the seriousness of the situation which is done not through messaging but via the red delete button. Users don’t read.

Comment 31

[Amy Lee [:amylee] UX]

I think Tiff has made some very strong points here. Especially since all of our highlight states are blue we really do need a red button to differentiate it from our less permanent actions. This should be considered a high priority item IMO.

Comment 32

[David Flanagan [:djf]]

Comment on attachment 8405732 [details] [review]
pull-request (master)

Setting approval for uplifting this to 1.4 because it contains strings that are required by the 1.4+ blocker bug 992393.

Comment 33

[Francesco Lodolo [:flod]]

I agree that this improves the user experience, but you're also going to make the experience on localized builds worse if those strings are not translated in time (quality perception of mixed language UI? Just bad).

That's not unlikely, considering that string freeze was supposed to happen weeks ago, and Camera already broke it several times. And considering also that Firefox OS is currently targeting markets where English is not the primary language, I wish we could do better than this.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/b1fba95fcbad1af7ba3aee466bf65429fe49fedf