802944 - Reply button should popup select menu for deciding reply mode instead of listing all the reply mode button in title

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

Based on Casey's comment and wireframe spec p.19 : https://www.dropbox.com/s/vaf9d6ppbw58zi1/mail-main.pdf , we should use value selector for choosing the reply mode and make additional space for the subject line.

Comment 1

[ayman maat :maat]

RFI to vicky to confirm if the way reply and reply all is currently implement is as intended.

Comment 2

[Victoria Gerchinhoren [:vicky]]

Right now you have two buttons in the header
1. reply to sender
2. reply all.

The behaviour Josh mentions is not implemented.

Comment 3

[Casey Yee]

This still hasn't been implemented to spec.

If this is an easy change I would like to see it in place for V1.

Comment 4

[Dylan Oliver [:doliver]]

triage: koi+ for now -- subject to change if this turns out to be more complex than we think.

Comment 5

[[e-mail], [e-mail]]

(In reply to Steve Chung [:steveck] from comment #0)
> Based on Casey's comment and wireframe spec p.19 :
> https://www.dropbox.com/s/vaf9d6ppbw58zi1/mail-main.pdf , we should use
> value selector for choosing the reply mode and make additional space for the
> subject line.

Looks like the link isn't existed. NeedInfo UX team for the latest spec. Thanks.

Comment 6

[Stephany Wilkes]

Here is a link to the latest email spec, which is for 1.2. It is Open Access on Box, so no one should need a Box login in order to see this.
https://mozilla.box.com/s/bgrftq1jr38rwzb173zj

Comment 7

[Stephany Wilkes]

Annnnd that spec is only 1.2 deltas, which has not been combined/trued up with the original spec, which will probably be more helpful to you. That one is here and is also set to Open Access. Apologies for any confusion!

Comment 8

[Stephany Wilkes]

Annnnd that spec is only 1.2 deltas, which has not been combined/trued up with the original spec, which will probably be more helpful to you. That one is here and is also set to Open Access. Apologies for any confusion!
https://mozilla.box.com/s/bgrftq1jr38rwzb173zj

Comment 9

[[e-mail], [e-mail]]

Hi Stephany,

I don't see the design for the replay bottom in spec. v1.2. There is no replaying page on it. Maybe we need to reference previous spec?

Comment 10

[Dylan Oliver [:doliver]]

Triage: This bug has been added to the v1.2 backlog but is not blocking release

Comment 11

[Marcus Cavanaugh [:mcav]]

Stephany, it looks like you provided the same link twice by mistake, and we're missing the full 1.2 spec.

Comment 12

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

The spec in question is on page 22 (21 is old and superseded by page 21) of mail-main.pdf, available directly from https://mozilla.app.box.com/applications/1/864506434/7994755152/1

The idea is to just pop-up our standard action menu with the listed buttons.  Note that the back-end does not currently support "Reply to List" because of potentially complicated header issues (ActiveSync! Aaaaaargh!) so that shouldn't get implemented yet.

Comment 13

[Marcus Cavanaugh [:mcav]]

Attachment: pull-request.html

Comment 14

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

Comment on attachment 796211 [details]
pull-request.html

This looks good!  Comments:

- canForward/canReplyAll: We should move these into MailHeader in mailapi.js.  The rule-of-thumb I have for things like this is that if it's e-mail domain logic, another UI built using GELAM would likely want to implement the same logic, and the logic can be cleanly separated from the UI code then we should implement it in GELAM.  We can handle the unit test coverage for canReplyAll when we fix the reply-all implementation to properly filter out stuff (which has its own bug).

- canForward: The existing !body check is a weird band-aid hack for something whose origins I don't remember without code spelunking, but which we should revisit.  Currently, the part of the back-end that does any of the reply/forward logic will asynchronously make sure it gets the body parts before we generate the reply/forward body.  So that's great for correctness, but potentially dangerous for the UI since we eatEventsUntilNextCard() until that compose-setup process completes and there's not actually any guarantees that it will do so in a timely fashion.  At that point the UI will be completely unresponsive to the user.

And this happens for all types of replies.  We could do something fancy with showing a cancelable progress spinner if it takes the back-end more than 200ms or something to let us show the compose UI, but I think that's overkill.  I think the easiest thing is for us to mark the reply button initially disabled (I think building blocks uses aria-disabled for the visual side of things?) and then only turn it on when the body gets fetched, probably in buildBodyDom since downloadBodyReps in our call getBody just initiates the download, it does not cause getBody to wait.

So to summarize for this bit, we don't need canForward, we want to guard against all replies, we do want canReplyAll in GELAM (sorry for the extra hoop).

- Integration tests.  We're on the cusp of having integration tests in general from Evan's patch.  I'd like simple integration tests for reply and forward.  These don't need to be super complex; it might be okay to just bring up the compose UI, verify that a unique string of the original message shows up in the compose window, then discard the draft.  indexOf() on the textContent should be fine to verify the presence of the original message, plus a quick check on 'wrote' for the reply and 'Original Message' for the forward.  (GELAM is the place the more extensive tests go; I really just want to make sure that we're seeing evidence that our plumbing is hooked up to the GELAM logic correctly.)

Comment 15

[Marcus Cavanaugh [:mcav]]

Comment on attachment 796211 [details]
pull-request.html

PR updated to add integration tests and grey out the "Reply" menu until the body has been loaded. I added a comment about someday moving canReplyAll() to GELAM per our IRC discussion.

Comment 16

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

I just realized that I meant to ask whether there's a way for us to show our pretty icons in the action menu or not.  The rationale would be that the distinct sillhouettes can be visually pre-filtered.  Although I'm now thinking I don't know if that fits with our visual design or not.  If you could determine whether showing icons in action menus is something we do elsewhere (or want to do, per visual design) and if we can easily do it, that'd be great.

Otherwise, this seems fantastic review-wise.  But I need to run with it on a device and run the marionette tests locally and maybe do another quick pass once I've gotten some sleep.

Comment 17

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

Comment on attachment 796211 [details]
pull-request.html

Looks great, works great!  Thanks!  r=asuth

Comment 18

[Marcus Cavanaugh [:mcav]]

Landed: https://github.com/mozilla-b2g/gaia/commit/216961d4f3705ebc6e49a6a8d20fae82534cfeb2