870320 - [Email]Attachment size and attachments are not aligned properly in email composer

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : Attachment size and attachments are not aligned properly in email composer 
2. Precondition : Email should be working and an IMAP account is configured
3. Tester's Action:  Launch Email -> compose -> Attach -> Image -> again attach one more image -> again attach one more image 
4. Detailed Symptom (ENG.) : Alignment of attachments and size is not proper.
5. Expected :All email attachments and their sizes must be aligned properly.
6.Reproducibility: Y
           1)Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8.Gaia Revision: 3c199e889364a0cc20f1b13a37eb309364a73de9
9.Personal email id:  [e-mail]

Comment 1

[psingapati]

Attachment: Wrongalignment

Adding attachment for wrong alignment of email attachments

Comment 2

[psingapati]

Attachment: Pull Request

Pull request for the issue.
Added padding top to cmp-attachments-size  and cmp-attachment-item to make total attachment size and attachments aligning properly.

Comment 3

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

Comment on attachment 748390 [details]
Pull Request

This is a good improvement, but:
- There is no whitespace between the "N attachments:" label in gray and the total size; it's floated directly against the label.

- The baselines of the "N attachments:" label and the total size don't actually line up.  ".cmp-attachment-label" is a table-cell span with a 3rem line-height, while ".cmp-attachments-size" is floated with padding-top: 0.5rem.  It sorta lines up, but not really.

- This isn't actually touched by this patch, but ".cmp-attachment-filename" does not line up with ".cmp-attachment-filesize" because ".cmp-attachment-filesize" has a freaky combination of vertical-align and a negative margin-top like .cmp-attachment-icon, apparently to deal with something freaky happen inside .cmp-attachment-filename because of its use of "overflow: hidden" to enable ellipsis.

Eric, can you help out with this patch, or if we have an existing bug elsewhere with a plan, accelerate that one?

Comment 4

[Alex Keybl [:akeybl]]

Minor polish bug so not blocking, but would accept an r+ low risk patch though

Comment 5

[Leo]

As discussed in San Diego work week marking this as leo+.

Comment 6

[Andreas Gal :gal]

leo team, are you supplying a new patch?

Comment 7

[psingapati]

Working on this issue.
As per Comment #3
Improvements 1 and 2 are done.
Checking the 3rd one, will upload the patch soon.

Comment 8

[psingapati]

Attachment: Pointer to pull request

1) Used Span element to provide fix ".cmp-attachments-size" white space and alignment with the ".cmp-attachment-label"
2) Adjusted margin top ".cmp-attachment-filesize" to make alignment with ".cmp-attachment-filename"
3) Provide margin for “.cmp-attachment-item” to make sure envelope line does not touch with the remove button.
Please review

Comment 9

[Dominic Kuo [:dkuo]]

Comment on attachment 755870 [details]
Pointer to pull request

I am able to review this so stealing from Andrew.

Comment 10

[Dominic Kuo [:dkuo]]

Attachment: Email screenshot after applied patch

I think there are still some noticeable issues on this patch, such as:

1. The attachments text and the size text seems proper aligned, probably we have to use vertical-align: middle or vertical-align: bottom to fix this.
2. The bottom line is not aligned to the other bottom lines.
3. There is indent before the attachment, currently email doesn't have this.

Comment 11

[Dominic Kuo [:dkuo]]

Comment on attachment 755870 [details]
Pointer to pull request

I feel this patch is not there yet so I will cancel the review request first and needinfo Eric to comment on this.

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note: bug 856226  had already been filed.

Comment 14

[Wayne Chang [:wchang]]

Hi Leo, are you still working on this further?

Comment 15

[Leo]

Currently not working on this.

Comment 16

[Leo]

Sorry 
by mistakenly removed epang needinfo

Comment 17

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

I didn't get this fixed this evening and I'm out tomorrow.  This was partially build troubles and partially complications from the unusual table-cell/float/relative-block DOM/CSS magic in place.

If anyone has time to steal this on Weds, they should.  It does seem sane to make more extensive cleanups if we can avoid regressions.

Comment 18

[Dominic Kuo [:dkuo]]

Andrew, I can take this and probably I will get rid of the table-cell things because it seems not the must have styles on the message cards.

Comment 19

[Eric Pang [:epang] UX]

Attachment: Attachment Mock Ups

Hi, I've attached mock ups and specs for email attachments.  All image assets are available in the 'assets' folder

I've created 3 mock ups for compose with the following proposed updates (see pngs in 'compose' folder:

1. No Attachments - I updated the header to 'Compose' so that an attachment icon can be added to the header (removing the add attachment section above the subject line).

2. 1 Attachment - Updated layout and font size, added a new remove attachment icons 

3. 2+ Attachments - Additional attachments share the same space (no div lines) 


I've also included mock ups for reading an email with attachments.
They are formatted the same way as the above with a "download" icon (instead of the remove icon).

Let me know if you have any questions!
Thanks!

Comment 20

[Eric Pang [:epang] UX]

Link to specs incase something happens to the attachments:
https://mozilla.box.com/s/klcpjqfgvb6lh27w54eg

Comment 21

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

(In reply to Eric Pang [:epang] from comment #20)
> Link to specs incase something happens to the attachments:
> https://mozilla.box.com/s/klcpjqfgvb6lh27w54eg

This link requires credentials to access.

Comment 22

[Dominic Kuo [:dkuo]]

Eric, for the download icon(actionicon_email_downloadattachment_20x20.png), we probably need two more icons because the icon has three states, which are downloadable, downloading and downloaded, but that's in the message page so we can file a new bug to fix it as a follow up.

Comment 23

[Dominic Kuo [:dkuo]]

Attachment: Adjust layout of the attachments on compose page

Andrew, this patch adjusted compose page base on attachment 759089 [details] which Eric gave to us. Mostly there are CSS and HTML changes and I didn't remove the logic/elements of calculating the total size of attachments(updateAttachmentsSize()), instead I just hide them with .collapsed so that we will be safe and not break anything, besides, I do remember there is a user story said we need to display the total size, so if we want to keep the total size, we can just revert it with removing the .collapsed on the UI elements.

Comment 24

[Dominic Kuo [:dkuo]]

Eric,

please pull/test my patch and see if I am doing right, while Andrew is reviewing, as I mentioned in comment 23, we have a leo user story asked for total attachments size in bug 838015 and fixed in bug 838006, and with your latest mockups, the total size is removed and so does my patch, we probably need to make sure it's okay for leo that we are going to remove it, or we want to keep it but it's not mentioned in the mockups?

Comment 25

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

Comment on attachment 759654 [details]
Adjust layout of the attachments on compose page

This looks great, conditional r=asuth:

- You need to change the l10n id since we are both changing the english string and the space available to translations.

- I believe that displaying the total attachment size is an explicit v1.1 feature.  As such, I think we need to re-display the "Attachments:" line with the size on it.  I don't think there's any way around that unless we get the v1.1 requirement struck down.  :robmac is the UX person for e-mail now, so we'll need to leave it to him to resolve this and to *update the wire-frames* :)

If there are only minor changes required to get that resolved, you don't need another review pass.

If we do stick with the string change, please add the late-l10n keyword and cc :l10n once we're sure.  (It's not clear to me if these changes will stick given the total size issue.)

Comment 26

[Rob MacDonald [:robmac]]

Attachment: showing file size for multiple attachments

(In reply to Andrew Sutherland (:asuth) from comment #25)
> - I believe that displaying the total attachment size is an explicit v1.1
> feature.  As such, I think we need to re-display the "Attachments:" line
> with the size on it.  I don't think there's any way around that unless we
> get the v1.1 requirement struck down.  :robmac is the UX person for e-mail
> now, so we'll need to leave it to him to resolve this and to *update the
> wire-frames* :)

There are indeed a couple of updates that need to be made to the spec and visuals. So thanks for your patience. The visual designs are accurate with the one exception that Andrew has pointed out. In bug 838015, the total size of all attachments is indeed a requirement from 1.1.

So this means that, if there is more than one attachment, we need to include the size. I attached a quick PDF to demonstrate. Sorry it's rough but hopefully it tells the story.

Please flag me if you need more information.

Comment 27

[Rob MacDonald [:robmac]]

Comment on attachment 759654 [details]
Adjust layout of the attachments on compose page

I'm still unable to view pull requests on my hardware but I was able to view it in aurora. Although Aurora isn't a substitute for seeing it on the phone, the layout looked very clean and appeared to be to spec. So that's very much appreciated.

The only missing element was the total file size as noted in my last comment and as flagged by Andrew. Because this was a 1.1 requirement, I'm going to flag it "-" for now. I apologize for any confusion if this was related to the spec. But hopefully this is something we can add soon.

Comment 28

[Eric Pang [:epang] UX]

(In reply to Dominic Kuo [:dkuo] from comment #11)
> Comment on attachment 755870 [details]
> Pointer to pull request
> 
> I feel this patch is not there yet so I will cancel the review request first
> and needinfo Eric to comment on this.

Thanks for working on this Dominic :).  Looks like you've received comments from Rob and Andrew already.

Comment 29

[Dominic Kuo [:dkuo]]

Thanks everyone, I will revise my patch to fit attachment 760017 [details].

Comment 30

[Eric Pang [:epang] UX]

(In reply to Rob MacDonald [:robmac] from comment #27)
> Comment on attachment 759654 [details]
> Adjust layout of the attachments on compose page
> 
> I'm still unable to view pull requests on my hardware but I was able to view
> it in aurora. Although Aurora isn't a substitute for seeing it on the phone,
> the layout looked very clean and appeared to be to spec. So that's very much
> appreciated.
> 
> The only missing element was the total file size as noted in my last comment
> and as flagged by Andrew. Because this was a 1.1 requirement, I'm going to
> flag it "-" for now. I apologize for any confusion if this was related to
> the spec. But hopefully this is something we can add soon.

Sorry Rob, I wasn't aware of this requirement!

Comment 31

[Dominic Kuo [:dkuo]]

Comment on attachment 759654 [details]
Adjust layout of the attachments on compose page

Rob, I have revised my patch and I would like to get your feedback+ before I land this, would you please take a look on it again? thanks!

Comment 32

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

Comment on attachment 759654 [details]
Adjust layout of the attachments on compose page

I'm going to provide confirmation of the goodness of this approach.  This conforms to the amended PDF wire-frame provided.  Let's land this now and if :robmac finds any problems we can do a follow-up bug.

Comment 33

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

Fixing this bug should also fix bug 856826, bug 860008, bug 860021.

Comment 34

[Dominic Kuo [:dkuo]]

Okay, sounds good and I am going to land this, then we can fix them all!

Comment 35

[Dominic Kuo [:dkuo]]

Landed on master: ceb11c3bd0b3c7a022f87c1adb80610c4d9d4275

Comment 36

[Dominic Kuo [:dkuo]]

Uplifted commit ceb11c3bd0b3c7a022f87c1adb80610c4d9d4275 as:
v1-train: 0c836be7f24693b74ad7fd21c8153824f1c7f548

Comment 37

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: 0c836be7f24693b74ad7fd21c8153824f1c7f548