1064065 - Trailing strings when removing account or deleting messages

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Théo Chevalier]

Attachment: Screenshots, 2.2

When removing an account or deleting messages, the strings "message-edit-delete-confirm" and "compose-discard-message" are displayed while they shouldn't.

Let's remove them, that's confusing.
At first glance it seems there are not used anywhere, so I'd nuke them completely.

For 2.1, since we are late, I propose to only remove the reference in the HTML and remove the strings from .properties file only on master (we have already enough late-l10n bugs). Will provide 2 PR.

Of course, please shout if I missed something obvious and the strings are still used, or if some work about those strings is ongoing somewhere else.

Comment 1

[Théo Chevalier]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23788

Comment 2

[Théo Chevalier]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23789

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

I think the intent was actually for those strings to be in the h1 where "Confirmation" is right now.  My understanding from way back when was that best practices was to have a terse summary of what the action would be there instead of the arguably useless "Confirmation" string.  I think that was also one of the reasons we wanted to move away from window.confirm...

Unfortunately I'm not sure where to get the current state-of-the-art on this, http://buildingfirefoxos.com/building-blocks/confirm.html just has a "confirm" string but I think that's never been canonical or focused on issues like this.  https://www.mozilla.org/en-US/styleguide/products/firefox-os/ is outdated and doesn't seem to show confirm UIs.  https://developer.mozilla.org/en-US/Apps/Design that links to the styleguide doesn't seem to have specifics.  And I think the spot at https://wiki.mozilla.org/Gaia#UX_Specifications_.28Wireframes.29 where I sorta remember there previously having been direction is now "TEMPORARILY DELETED"?

Our Taipei UX people are out on Monday, so needinfo-ing :swilkes.

Comment 4

[Stephany Wilkes]

Sorry - what is the UX question? Is this about what the text should say?

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Stephany Wilkes from comment #4)
> Sorry - what is the UX question? Is this about what the text should say?

For the confirmation dialog, is it preferred that the text above the horizontal-rule-style line actually be something terse like "Discard Email?" or do we always want it to just say "Confirmation".

If you look at the screenshot attachment, right now the terse strings are coming after the full description of the requested action.  That's clearly wrong, but my suggestion is we move those strings up to replace "Confirmation" as I think was our intent.

Comment 7

[Stephany Wilkes]

This is one for Rob.

Comment 8

[Rob MacDonald [:robmac]]

Going forward we will be eliminating the headers in the confirmation dialogs and just going with the single string. So I would propose retaining only the body of the message. Further, in the case of the deletion, the strings are redundant. And, in the case of removing the email account, the string in the attachment appears to be the wrong string in any case.

I'll NI Juwei as the owner of email as I believe she's in today and able to confirm.

Thanks!
Rob

Comment 9

[Juwei Huang[:juwei]]

Yes as Rob said, we plan to not have header on the pop-out window in the future. 
My suggestion would be solely remove the grey text on confirmation window.
Thank you!

Comment 10

[James Burke [:jrburke]]

Comment on attachment 8485520 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23788

Stealing review to help distribute some of the load :asuth is carrying:

This is close, but there are two cases where the delete message confirmation is shown -- one in the bulk edit on message list and one in the the message reader view.

So this patch needs to be updated to use the message-edit-delete-confirm message to be used in the message reader case. I did this sort of change locally:

diff --git a/apps/email/js/cards/message_reader.js b/apps/email/js/cards/message_reader.js
index 6c59269..1a95670 100644
--- a/apps/email/js/cards/message_reader.js
+++ b/apps/email/js/cards/message_reader.js
@@ -359,6 +359,8 @@ MessageReaderCard.prototype = {
 
   onDelete: function() {
     var dialog = msgDeleteConfirmNode.cloneNode(true);
+    var content = dialog.getElementsByTagName('p')[0];
+    mozL10n.setAttributes(content, 'message-edit-delete-confirm');
     ConfirmDialog.show(dialog,
       { // Confirm
         id: 'msg-delete-ok',

and that fixes the message reader case.

If you can do that change, then update the other pull request to keep the message-edit-delete-confirm string, and just remove the compose-discard-message string, that should be enough to get it landed. Flip the review back to me and let me know if you have the permissions to merge the change or if you want me to do the actual merge.

Comment 11

[James Burke [:jrburke]]

Comment on attachment 8485522 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23789

Flipping review off for now until comments above have been addressed.

Comment 12

[Théo Chevalier]

Comment on attachment 8485520 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23788

Hi James,

Thanks for your help. Tested on Flame 2.2, it looks good

Comment 13

[James Burke [:jrburke]]

Comment on attachment 8485520 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23788

Looks good, thank you! If you need me to do the final merge, just let me know and I can push the buttons.

Comment 14

[Théo Chevalier]

Thanks James
Yes, would like you to merge it for me (don't have repo rights)
Thanks!

Comment 15

[James Burke [:jrburke]]

For the l10n string removal (*should not* ever be backported to 2.1), merged to master:

https://github.com/mozilla-b2g/gaia/commit/72f47e731680e415713d2639b60ce75133fd5c2d

from pull request:
https://github.com/mozilla-b2g/gaia/pull/23789

For the changeset that *can* be uplifted to 2.1 without needing late-l10n, merged to master:

https://github.com/mozilla-b2g/gaia/commit/aff6781cf392d0e0fba534c499e0d0aa7df10a3a

from pull request:
https://github.com/mozilla-b2g/gaia/pull/23788

:tchevalier, if you would like to make the case for 2.1 uplift, please set the blocking-b2g to 2.1? with the reason. My understanding is that they are being fairly strict for 2.1 uplifts, and only for breaking changes. However, if this improves the l10n localization for some cases, perhaps that will be allowed.

If you cannot flip that flag for permissions reasons, then just state the reason why you think 2.1? flag makes sense and ask me to flip it.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8485520 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23788

Per https://wiki.mozilla.org/Release_Management/B2G_Landing#v2.1 we don't need to request blocking, just approval.  I'll request it now since this meets the polish requirements, etc.

[Approval Request Comment]
[Bug caused by]: UI refactoring back for v2.0, probably.  Nothing to back out, it's very complex to look into exactly.
[User impact] if declined: Dumb-looking duplicate strings, see the screenshots on the first attachment for the bad stuff happening now.
[Testing completed]: James Burke looked at things
[Risk to taking this patch]: No risk is expected; :jrburke already caught the weird edge cases.
[String changes made]: No changes to strings in this patch other than ceasing to use them.  The changes to remove dead strings happened in the other patch which should not be uplifted because it would cause the l10n tools to freak out.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/0092e8fd1f4a1f1224489538824acee3d6433b8b

Comment 18

[Juwei Huang[:juwei]]

Hey guys,

I just found that the patch isn't what we want...

On the dialog of removing account, it should keep the body script and erase the gray text.
And for the delete message, the text font should be the same as body text and should be white.

Thanks

Comment 19

[Juwei Huang[:juwei]]

Attachment: screenshots.png

Current screenshots attached

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

I just determined with :juwei on IRC that the uplift is not yet in the build being used.  Manual resolution trace: 

Build ID: 20140924160202
Version 34.0a2

https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-aurora-flame/2014/09/2014-09-24-16-02-02/sources.xml says:
<project name="gecko.git" path="gecko" remote="mozillaorg" revision="c1088b96b248b04460a9709170a7000180ca5cf2"/>

gecko-dev's b2g/config/gaia.json says:
    "revision": "e7f0f380339cfbcdd624987276a978d8e2df160d", 
    "repo_path": "/integration/gaia-2_1"

http://hg.mozilla.org/integration/gaia-2_1 indicates that the build commit predates the fix uplift with hg hash 2d13e802ea3e

Comment 21

[Juwei Huang[:juwei]]

Sorry I checked on the wrong version!
The latest one looks fine! Thanks.

Comment 22

[Edward Chen[:edchen]]

[Environment]
Gaia-Rev        13973ab50760d1e8bb773082163f0dff19d35a44
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/6e317e075d04
Build-ID        20140928160204
Version         34.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20140925.192608
FW-Date         Thu Sep 25 19:26:18 EDT 2014
Bootloader      L1TC10011800


[Result]
Pass

Comment 23

[Alissa（leave from Mozilla）]

Attachment: verify.png

This issue has been successfully verified on Flame 2.1, 2.2
See attachment:verify.png
Reproducing rate: 0/5

Flame 2.1 new versions:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141130001203
Version         34.0

Flame 2.2 new version:
Gaia-Rev        7119da7a86cd803840678ca3a6067e5622adc481
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/df3fc7cb7e80
Build-ID        20141130040207
Version         37.0a1