1640760 - Refactor and improve the UI of the attachment pane in the message compose window, incl. drag and drop

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement, P3)

Description

[Alessandro Castellani [:aleca]]

Attachment: attachments 1.png

Relocate the Attachment Pane

Let's relocate the Attachment Pane at the bottom of the compose area and remove the Attach toolbar button. With these changes we will:

• Maintain visual consistency with how we list attachments when reading a message.
• Enabling us to include the Attach button in the main focus ring.
• Remove the issue of visible/collapsed panel option as we don't need to save space in the header area.
• The pane is always there, easily discoverable, it doesn't interfere with any major spacing as we have plenty of real estate in the compose body.

Collapsed always shows important Info.

This change will help the user to track how many attachments he has in the message, and the size of the message.
Even if the pane is "collapsed" (the user doesn't see which files he attached), he can always quickly glance at the most important info he needs to know, without the necessity of revealing/opening the panel because it's in the way.

We also can get rid of that pretty unflattering gigantic attachment icon we currently show when the attachment pane is collapsed with files in it.

Attachment list

The user will be able to open/close the attachment pane by focusing on the Twisty icon via a regular focus ring, or maybe with a dedicated shortcut we can provide, limiting the Alt+M to only trigger the File chooser dialog.

The opened pane will show individual files in a similar way we currently do when reading a message.
In this screenshot I applied a more refined UI to improve readability and visual separation between elements.
Reordering, editing, detaching, and so on, will work as expected since this is still a richlist element.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: attachments 2.png

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: attachments 3.png

Comment 3

[Richard Leger]

Excellent idea/initiative... totally make sense ;-)

If I may suggest perhaps to:

• move the Attach button to the left instead the right it would make more sense as all the rest is aligned to the left... and people tends to read from left to right... top to bottom... making it easier to find/access especially on wide screen... a bit the same way you moved Cc/Bcc from the right originally to the left... The Attach button could appear first on the line or above the number of attachments on its own line (but maybe a waste of space). Of course for those reading left to right it could be aligned the other way around...
• explore the possibility to have the attachments panel below Subject... before fomatting bar of body content... end-users tends to like it to have attachments towards the top... to make them easily and quickly accessible...
• if there is only few attachments they could appear directly and if there are more there is a 'more' chevron to unfold to display them... it is indeed annoying to always have to unfold first to access attachments... especially if only few are attached... the number of displayed attchment by default could depends on the space available... on a wide screen they would probably all show fine by default ;-)

For your consideration...

Comment 4

[WaltS48 [:walts48]]

Where is the "Save" button that contains Open, Save As, Detach and Delete?

Comment 5

[Richard Marti (:Paenglab)]

Walt, it's the compose window, not the read window.

Comment 6

[Richard Leger]

(In reply to WaltS48 [:walts48] from comment #4)

Where is the "Save" button that contains Open, Save As, Detach and Delete?

When receiving message it should remain possible to save all attachment at once or individually... with a link to the folder where they were saved appearing in TB after saving allowing quick access to the folder if needed... in a similar way you can download in Firefox and be able to open folder where files were saved...

Comment 7

[Alessandro Castellani [:aleca]]

When receiving message it should remain possible to save all attachment at once or individually...

This bug is only to track the work on the compose window, not the read window, like Richard pointed out.
Let's stay in topic, please :D

Comment 8

[Alessandro Castellani [:aleca]]

move the Attach button to the left instead the right it would make more sense as all the rest is aligned to the left

I'll explore this, but I'm not sure as I'd like to keep the recap of how many attachments there are and the total weight in that position.
I'm worried it might feel a little bit cramped.

explore the possibility to have the attachments panel below Subject... before fromatting bar of body content... end-users tends to like it to have attachments towards the top... to make them easily and quickly accessible...

Indeed, I explored this idea during early stages of mockups as I noticed other email clients do that.
I have a couple of doubts about that location. First, it removes the consistency I'm looking to implement between compose and read pane, with the attachments always at the bottom.
Second, and I think this is the most important, it creates many more spacing boundaries since we're inside the header container. We wouldn't have the freedom and ease to resize and enlarge the attachment container and make it a scrollable area if we put it in there.

if there is only few attachments they could appear directly and if there are more there is a 'more' chevron to unfold to display them... it is indeed annoying to always have to unfold first to access attachments...

I was thinking to always have the pane opened with 1 row of attachments visible, like in the mock-up. If there are more, that area turns into a scrllable and resizable container. The user will have the option to collapse it by clicking on the chevron, or leave it open since it won't expand independently.

Thanks for the suggestions, I'll explore some of those, and since we're not in a rush for the next ESR (hopefully), we will be able to release a beta and gather feedback, with plenty of time to change it and improve it.

Comment 9

[Richard Leger]

(In reply to Alessandro Castellani (:aleca) from comment #8)

move the Attach button to the left instead the right it would make more sense as all the rest is aligned to the left
I'll explore this, but I'm not sure as I'd like to keep the recap of how many attachments there are and the total weight in that position.
I'm worried it might feel a little bit cramped.

You can always add space after the button (and before eventually) so to bring "air" ;-)

So instead of:

[Attach] 4 Attachments [+]

You can do:

[Attach] 4 Attachments [+]

or

[Attach] 4 Attachments [+]

to detach the attach button from the rest... but you would see (and know) best via iteration...

explore the possibility to have the attachments panel below Subject... before fromatting bar of body content... end-users tends to like it to have attachments towards the top... to make them easily and quickly accessible...
Indeed, I explored this idea during early stages of mockups as I noticed other email clients do that.
I have a couple of doubts about that location. First, it removes the consistency I'm looking to implement between compose and read pane, with the attachments always at the bottom.

Sorry, like Walt I had understood those changes would affect both compose and read Windows...
So my suggestion implied that the location under Subject would also apply to the read Window... not just the compose. What ever is chosen, both should be inline... agreed! Consistency oblige...

Second, and I think this is the most important, it creates many more spacing boundaries since we're inside the header container. We wouldn't have the freedom and ease to resize and enlarge the attachment container and make it a scrollable area if we put it in there.

Isn't the attachmemt area a container in itself? That can be separate from header container?

I am assuming here that msg body and formatting toolbar are themself a separate container... (which may not be the case I don't really know)... so the attachments one could go in between... for both read/compose Windows...

if there is only few attachments they could appear directly and if there are more there is a 'more' chevron to unfold to display them... it is indeed annoying to always have to unfold first to access attachments...
I was thinking to always have the pane opened with 1 row of attachments visible, like in the mock-up. If there are more, that area turns into a scrllable and resizable container. The user will have the option to collapse it by clicking on the chevron, or leave it open since it won't expand independently.

Scrolling on one raw height might be a bit "weird" (or "crancked") not sure how to describe it... Wouldn't a "more" button that unfold down the entire list of remaining attachments that can be scroll down via the window scrolling be more appropriate? It might be faster to reach an attachment you know is already there... faster access = better
One could even imagine that all attachments appear by default... why to limit if window space and msg content size allow it?

With the new design, is that really necessary to be able to collapse the entire attachments panel via a chevron (if I understood you correctly)?

Comment 10

[Alessandro Castellani [:aleca]]

I am assuming here that msg body and formatting toolbar are themself a separate container... (which may not be the case I don't really know)... so the attachments one could go in between... for both read/compose Windows...

They're separate containers, but try to visualize what it might happen when the user has many attachments if we put that section in between the subject line and the formatting toolbar.
What do we do? We let the attachment area grow indefinitely? Do we add a height limit and then scrolling? Do we allow the user to resize only the attachment area or that's also controlled by the header height which affects also the height of the recipient area? And what if the user has many recipients and many attachments and wants to see both all the pills and all the attachments? Which container has an expanding priority?
Too many visual edge cases we should really avoid.

With the new design, is that really necessary to be able to collapse the entire attachments panel via a chevron (if I understood you correctly)?

Indeed it is.
We can't assume how the user wants to see the content, or if there's enough vertical space, or how many attachments might fit in there. Having this panel at the bottom, allowing the user to resize it to see as many attachments as he/she wants, or collapse it to have it out of the way, ensures enough flexibility to cover almost any edge case (we will definitely stumble upon some random edge cases we didn't consider, but that's life).

Showing only one row it's just the default first view, as that, potentially, will be enough to handle the majority of the situations, where the user uploads <5 attachments, which is more common than always uploading 6 or more attachments on every email.
But the "1 row" is not a fixed or forced height as the user will be able to resize the panel to fit his/her needs.

The "more" button is not a paradigm we currently use and it wouldn't make sense introducing it here.

The things that you propose are interesting approaches, but they deviate a bit from the scope of this implementation. When we design an interface we always need to keep in mind the purpose of these changes and the scope of the section we're updating.
What's the purpose of the attachment area in the compose window? Allowing users to see which files were attached. That's it.
We don't need a full screen interactive area always visible during composition, and after the user confirmed that the files are correct, the focus should go back to the body of the message, therefore collapsing is extremely important.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

I started working on this and here's a WIP patch that kinda works.

I'd like to ask some early feedback because I think the keyboard and focus interaction of this area should be rethought and improved.

The current keyboard implementation is very strange.

• Alt+M toggles the attachment pane on and off, even if there are not attachments, which is useless with the updated UI since we will always show the number of attachments if present.
• If no attachment is present, Alt+M moves the focus to the richlistbox element and if the user hits Enter it triggers the file uploader.
• If attachments are present, Alt+M moves the focus to the first attachment element and if the user hits Enter it triggers the opening of the focused file (very strange and inconsistent).
• Ctrl+Shift+A triggers the file uploader, but it doesn't work if the focus is on the message body.

I think we should rethink and unify these mechanisms and implement something more consistent and expected.
These are the things I'd like to consider for this implementation:

• A keyboard shortcut that works anywhere to trigger the file uploader.
• A keyboard shortcut that opens and closes the attachment pane, only if NOT empty, and always moves the focus on the first item.
• We should add the "Attach" button to the default focus ring.
• We should add the visual toggle to open and close the attachment pane to the default focus ring, only if the attachment bucket is NOT empty.

We should also use the occasion to handle all these things via eventListeners and step away for the XUL key implementation.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9196504 [details] [diff] [review]
1640760-attachment-pane.diff

About the keyboard shortcut I'll let Thomas reply as it's his beloved area. ;-)

What me bothers the most is that there is always a big toolbar on the bottom also when I usually don't send attachments. Together with the status bar it takes a lot of space on bottom. The actual implementation is good because when no attachments are used it takes no space. Maybe leave the attachment button where it is now and show the attachment area when there are attachments.

When dropping attachments with the mouse only the header bar and the attachment bar are targets but not the attachment list nor the text area. Especially with HTML mails attaching inline would be great.

What's also a bit weird is that the attachment list shows/hides when you left or right click on the attachment bar.

Also when it looks similar to the reader attachment list it doesn't follow the mailnews.attachments.display.view pref. I know, this isn't well known and the reader view has some issues with showing too much space below the attachmentItems.

Comment 13

[Alessandro Castellani [:aleca]]

(In reply to Richard Marti (:Paenglab) from comment #12)

What me bothers the most is that there is always a big toolbar on the bottom also when I usually don't send attachments. Together with the status bar it takes a lot of space on bottom. The actual implementation is good because when no attachments are used it takes no space. Maybe leave the attachment button where it is now and show the attachment area when there are attachments.

Sure, I'll investigate this solution.
I'm not sure if it would look weird having the attachment button completely detached from the location of the attachment bucket, but we'll see.

When dropping attachments with the mouse only the header bar and the attachment bar are targets but not the attachment list nor the text area. Especially with HTML mails attaching inline would be great.

Yeah, I didn't work on any of this yet so most things are broken.

What's also a bit weird is that the attachment list shows/hides when you left or right click on the attachment bar.

This emulates the behaviour of the attachment bar when reading a message (maybe I should prevent right click interaction).
I personally don't mind that the entire bar is clickable as it keep the UX consistency with the message reader, but we can easily change that if we consider it not useful.

Also when it looks similar to the reader attachment list it doesn't follow the mailnews.attachments.display.view pref. I know, this isn't well known and the reader view has some issues with showing too much space below the attachmentItems.

Indeed, I wasn't aware of this, thanks.

Comment 14

[Richard Leger]

(In reply to Alessandro Castellani (:aleca) from comment #13)

(In reply to Richard Marti (:Paenglab) from comment #12)

What me bothers the most is that there is always a big toolbar on the bottom...
I'm not sure if it would look weird having the attachment button completely detached from the location of the attachment bucket, but we'll see.

Why not hide attachment toolbar when it is empty and show only a square attachment button (Paperclip icon) slightly detached from the bottom left corner in the message body?

When dropping attachments with the mouse only the header bar and the attachment bar are targets but not the attachment list nor the text area. Especially with HTML mails attaching inline would be great.

+1

What's also a bit weird is that the attachment list shows/hides when you left or right click on the attachment bar.

This emulates the behaviour of the attachment bar when reading a message (maybe I should prevent right click interaction).
I personally don't mind that the entire bar is clickable as it keep the UX consistency with the message reader, but we can easily change that if we consider it not useful.

Would left click allow select if attachment(s) fir further interaction?

Would right click on an attachment (or selection) be available to interact with it? E.g Save As, Delete etc...

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9196504 [details] [diff] [review]
1640760-attachment-pane.diff

Review of attachment 9196504 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I like that the bar is visible all the time at the bottom of the compose window. It's wasting space. So I'm not sure overall. I guess it's ok when the attachments are added.

The patch as such doesn't work in dark mode.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +141,5 @@
>  var gEncryptedURIService = Cc[
>    "@mozilla.org/messenger-smime/smime-encrypted-uris-service;1"
>  ].getService(Ci.nsIEncryptedSMIMEURIsService);
>  
> +// Temporarily store the height of the attachment contair allowing users to keep

container?

Comment 16

[Alessandro Castellani [:aleca]]

Would left click allow select if attachment(s) fir further interaction?
Would right click on an attachment (or selection) be available to interact with it? E.g Save As, Delete etc...

Yes to both.
The interaction is only on the grey bar, not the whole bucket.

Comment 17

[Alessandro Castellani [:aleca]]

I don't think I like that the bar is visible all the time at the bottom of the compose window. It's wasting space. So I'm not sure overall. I guess it's ok when the attachments are added.

Indeed, it takes up a little bit too much space.
I'll see if it makes sense leaving the toolbarbutton in the toolbar at the top right, or having it in some other locations.

The patch as such doesn't work in dark mode.

Yeah, I didn't work much on styling but I wanted to gather some early feedback.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Patch updated with the following paradigms:

• The Attach button stays in the top right toolbar as it looks like a good location.
• The Attachment Bucket is at the bottom as it turns visible only if there's at least 1 attachment present. If no attachments are uploaded, the entire bucket disappears to save space.
• Once the Attachment Bucket is visible, it can be toggled with a left click on the entire grey bar, or by pressing Alt+M.
• The entire bucket area is a drop area for files, as well as the Attach button like we currently have.
• I updated the F6 focus ring to match the new layout.

I updated a bit the shortcuts:

• Alt+M toggles the Attachment Bucket visibility only if we have any attachment. If the bucket is empty, nothing happens since it's not necessary to show an empty bucket.
• Alt+A triggers the file picker, and it's represented and discoverable in various locations like the File > Attach menu, the accesskey of the Attach button, and in the button's tooltip.
• I removed the Ctrl+Shift+A shortcut to trigger the file picker because it didn't consistently worked, and it wasn't possible to trigger it when the focus was on the message body.

I think this combination of shortcuts and accesskeys is sensible as it removes the strange inconsistencies we currently have, makes these combinations more discoverable, and it works regardless where the user is currently focused.

P.S.: I launched a try-run which I'm sure will have a lot of failures due to the layout changes, so please ignore it as I will fix everything.
I'm just adding it here for the record: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a00b6dc39ea050daf571b52d4aa38705180607a6

Comment 19

[Richard Marti (:Paenglab)]

Comment on attachment 9197239 [details] [diff] [review]
1640760-attachment-pane.diff

Review of attachment 9197239 [details] [diff] [review]:
-----------------------------------------------------------------

Much better now. I think we could go this way.

For ui-r not yet ready as the attachmentbar isn't completely dark theme ready and the #attachmentbucket-sizer needs more styling like the main window's #attachment-splitter. At least on Windows it is too fat and always grey. It should only show the top border.

::: mail/themes/shared/mail/messenger.css
@@ +440,5 @@
>    margin-block: 3px;
>  }
>  
> +#palette-box .toolbarbutton-1,
> +#attachmentView  .toolbarbutton-menubutton-button {

Only one space.

::: suite/themes/modern/messenger/messengercompose/messengercompose.css
@@ +24,5 @@
>  }
>  
>  #button-send:hover {
>    -moz-image-region: rect(374px 99px 407px 50px);
> +}

You're a SM developer too? ;-)

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Richard helped with the styling, and I fixed the tests: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c4580aee4dff3d5b9231762dc7ce28e40778f419

Comment 21

[Richard Marti (:Paenglab)]

Comment on attachment 9197485 [details] [diff] [review]
1640760-attachment-pane.diff

Looks good now. Thanks.

Comment 22

[Richard Leger]

(In reply to Alessandro Castellani (:aleca) from comment #18)

• The Attach button stays in the top right toolbar as it looks like a good location.

Is that temporary as part of iteration process?

Top right corner is really not a good location nor the right place for the Attach button in the first place. Not natural or intuitive especially now that there will be an attachment bucket located at the bottom.

Do you plan to re-locate it in a follow-up bug?

1st Proposal:

Like at the bottom left of message body perhaps slightly detached from the corner so it looks like an "embedded" attachment button in body (or underneath body field but making it looked like it is part of the body)?
Putting it where the attachments would be effectively located would make more sense.

Left:
Because reading from left to right, all other fields are left aligned by default as well as body text.
(For right-to-left reader mode it could be align to right as all the rest would be in reverse)

Bottom:
Because brain read/scan top to bottom by education and layout alignement already existing...
Also it would make sense in regard of Attachment Bucket now relocated at the bottom.

Having a button top-right is counter intuitive/productive as it create "brain effort" to find that brain muscle do not adjust to...

2nd Proposal:

A possible alternative location could be within the body formating toolbar that already contains button to add Image so why not have the button to add attachement in a similar location? Though 1st proposal may be best considering the Attach Bucket feature...

Comment 23

[Alessandro Castellani [:aleca]]

The Attach button stays in the top right toolbar as it looks like a good location.

Is that temporary as part of iteration process?

Yes, let's keep it there for now as it's part of the customizable toolbar and users might have it in a different location.
The focus of this bug was the attachment bucket, so once this lands we will do a couple of follow ups to explore the button location, and also the drag&drop of files to attach or add inline.

2nd Proposal:
A possible alternative location could be within the body formating toolbar that already contains button to add Image so why not have the button to add attachement in a similar location?

Yeah, I was thinking about this, rather than having a "floating" button at the bottom left.
We can continue this exploration with some mock-ups in a follow up.
Thanks for the suggestions!

Comment 24

[Thomas D. (:thomas8)]

TL;DR:
While this certainly provides some eyecandy and has been skillfully crafted (that's Alex!), unfortunately I'll have to be the bad cop on the whole idea (I don't like being the bad cop, guess that's why I've delayed this feedback):

• I am mostly failing to see any tangible UX benefits of this new design, especially the relocation.
• I am seeing a number of substantial UX disadvantages and losses of functionality, and I don't think all of them can be addressed within the new design, as some of the biggest concerns are about the horizontal relocation itself and the resulting waste of vertical space.
• I would not be surprised if this receives a similar backlash from users like the new addressing area - only our arguments in favor may be weaker this time, because - in the current iteration at least - this looks like a rather cosmetic change.

(In reply to Alessandro Castellani (:aleca) from comment #0)

Relocate the Attachment Pane
Let's relocate the Attachment Pane at the bottom of the compose area and remove the Attach toolbar button. With these changes we will:

• Maintain visual consistency with how we list attachments when reading a message.

• So far I used to think the visual distinction is actually helpful, more so when composition will live in a tab - that consistency will make it much harder to distinguish between a received message and a composition in a tab.
• I don't see a particular benefit of visual consistency between attachment area of received msgs vs. composition - the functionality is quite different.

• Enabling us to include the Attach button in the main focus ring.

• So far this has not materialized.
• The main focus ring is the tab focus ring (F6 is fast focus ring, not documented in primary UI, hence all but undiscoverable except in keyboard shortcuts documentation).
• You cannot include the Attach button in the main focus ring if it's at the bottom, because msg body hijacks Tab keypresses on the way there.

• Remove the issue of visible/collapsed panel option as we don't need to save space in the header area.

• I'm failing to see the issue with visible vs. collapsed panel in the header. It was working well. We could add a circular indication of the number of attachments on top of the paper clip when collapsed, maybe the size. Otherwise nothing wrong, but a lot of benefits (see below).

• The pane is always there, easily discoverable, it doesn't interfere with any major spacing as we have plenty of real estate in the compose body.

• Disagreed. The new design can cause substantial interference with major spacing, and I don't think we have "plenty of real estate in compose body".
• Depending on scenario, the new design may eat massively into vertical spacing, in addition to our large addressing header (as we lack an option to collapse that). On today's wide screens, there's plenty of horizontal space, so it's the vertical space which is precious.
• With the existing (TB78) implementation, combining the addressing area and the attachment pane into the same vertical space was a perfect vertical space saver, providing well for everyday scenarios like the desire to see and act on attachments whilst writing the message (e.g. to mention/describe/discuss/rename/review/reorder/add/remove attached files as you write along).
• The new design eats up more vertical space for such scenarios, especially when Thunderbird window is smaller, e.g. half-screen or smaller screens. Not everyone works on 27+ inches (think laptops).

Collapsed always shows important Info.

• Yeah, that's nice - but costly.

This change will help the user to track how many attachments he has in the message, and the size of the message.
Even if the pane is "collapsed" (the user doesn't see which files he attached), he can always quickly glance at the most important info he needs to know, without the necessity of revealing/opening the panel because it's in the way.

• As hinted above, it's not too hard to add that to the current implementation.

We also can get rid of that pretty unflattering gigantic attachment icon we currently show when the attachment pane is collapsed with files in it.

• Imho, nothing wrong with that: It's a can't-miss indicator that there are attachments with the message, but user volunteered to hide them. This is about ux-error-prevention.

Attachment list

The user will be able to open/close the attachment pane by focusing on the Twisty icon via a regular focus ring, or maybe with a dedicated shortcut we can provide, limiting the Alt+M to only trigger the File chooser dialog.

• Regular focus ring not possible. I'm seeing Alt+A on en-US now for "Attach files", which may be less attractive than expected as it's no longer an international shortcut (easy to support and document), but now a localized access key: Alt+A works only for en-US, other languages will need to find their own, and it competes with all other localized Alt+* access keys out there, in main menu and elsewhere. This is a moving target due to the undefined and undiscussed partial transition to browser access keys (Alt+Shift+*). At least we should be aware of that. Let's hope translators do their job - otherwise we end up with broken accelerators. Ymmv - from another angle the localized access key might make things easier and it's also highly discoverable.

The opened pane will show individual files in a similar way we currently do when reading a message.
In this screenshot I applied a more refined UI to improve readability and visual separation between elements.
Reordering, editing, detaching, and so on, will work as expected since this is still a richlist element.

• As expected, really? Due to the horizontal re-organisation, I cannot even keyboard-navigate from left to right and back using arrow-right/left... (same problem as in received messages... it's wrong in the widget, not Aleca's fault).
• Reordering is now a lot less charming than before, the naturally intuitive application to the vertical list of attachments is gone. I'm not sure if we should try to fix that, surprisingly imo the swapped direction feels less strange than with plain vanilla navigation.

Bottom line:

• I'm not sure if the perceived/claimed benefits outweigh a number of disadvantages of the new design.
• I don't perceive this as broken, and I'm quite happy with the status quo - space-saving and highly efficient.
• I might add some more details of specific issues with the current implementation below.

So far for first impressions... sorry!
I'll definitely continue playing with this - maybe I'll warm up with it...

Comment 25

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #11)

The current keyboard implementation is very strange.

I have been involved in the creation of the keyboard implementation, and I don't think so.

• Alt+M toggles the attachment pane on and off, even if there are not attachments

• Yes. This was done in response to many user requests asking for a way of showing an empty, open attachment pane, esp. as an easy and obvious target for drag and drop.

which is useless with the updated UI since we will always show the number of attachments if present.

... and you'll always consume more vertical space. I don't think it's useless, see below.

• If no attachment is present, Alt+M moves the focus to the richlistbox element and if the user hits Enter it triggers the file uploader.

• Alt+M is primarily an access key, here with slightly extended functionality - does the right and helpful thing every time you press it.
• Enter on focused, empty attachment pane is a convenience shortcut - highly efficient once discovered, no harm if you don't find it or don't want to use it. Note that you can attach multiple files from the first dialog - there's a decent chance that you don't even need to trigger the dialog again. We're not exactly promoting that as the main way of adding attachments, I guess we have Ctrl+Shift+A international shortcut for that (before this patch removed that and replaced it with localized access key).

• If attachments are present, Alt+M moves the focus to the first attachment element and if the user hits Enter it triggers the opening of the focused file (very strange and inconsistent).

• Imho, nothing strange. Dito: Alt+M is primarily an access key. Access keys trigger buttons, OR they focus inputs. Attachment pane is a file input, so we focus that. If there's content, that content gets focused - nothing special or wrong so far. Again, this is quite convenient, because the chances are much higher that you'll be focusing a full bucket to act on it's content in one way or another.
• There's Ctrl+Shift+A which always works.

• Ctrl+Shift+A triggers the file uploader, but it doesn't work if the focus is on the message body.

Worksforme from everywhere in composition (including body) on Windows 10 - on which OS does this fail for you? Maybe a mixup between Ctrl vs. Cmd on Mac OS.

I think we should rethink and unify these mechanisms and implement something more consistent and expected.
These are the things I'd like to consider for this implementation:

• A keyboard shortcut that works anywhere to trigger the file uploader.

We already have that: Ctrl+Shift+A.

• A keyboard shortcut that opens and closes the attachment pane, only if NOT empty, and always moves the focus on the first item.

• Opening empty attachment pane is a feature, not an accident! Esp. easy drag n drop target. We even have a feature that starts every composition with the attachment pane already open, which has been requested by enterprise users. Unfortunately, this feature has been made inaccessible by this patch (was available via right-click on attachment pane header).
• There's a another advantage of having Alt+M access key to focus/open empty or full attachment pane alike: Easy keyboard access to the attachment pane context menu, which has some attachment commands that are otherwise much harder to reach via File menu (e.g. attach Web page. There's a bug in the widget that when all attachments are deselected (e.g. using Ctrl+Space), the context menu key does not open the pane context, but the file context.

• We should add the "Attach" button to the default focus ring.

• Hasn't happened yet; not possible imo when "attach" button is at the bottom. The new button access key might mitigate this.

• We should add the visual toggle to open and close the attachment pane to the default focus ring, only if the attachment bucket is NOT empty.

We should also use the occasion to handle all these things via eventListeners and step away for the XUL key implementation.

Yeah.

Comment 26

[Thomas D. (:thomas8)]

Comment on attachment 9197485 [details] [diff] [review]
1640760-attachment-pane.diff

Review of attachment 9197485 [details] [diff] [review]:
-----------------------------------------------------------------

As explained with some detail in my Comment 24 (and comment 25), I'm not too thrilled by the whole idea in principle.
Let's look at some more details.

I have a big question for this patch: Can we please stick to agreed procedures and avoid combining a massive code cleanup/rewrite (without functional changes) with new feature/UI work?
- This patch is extremely hard to review because it's entirely not clear which parts actually constitute the new feature, because most changes are totally irrelevant to the feature/UI as they just optimize the existing code by introducing the global variable gMsgAttachmentElement consistently. I suggest to split up this bulk patch into a code cleanup patch and a feature/UI implementation patch. If we ever need to back this out, we'll lose not just the new UI, but also the code optimizations - not ideal!
- That variable is pretty bulky btw, given that we're using it a thousand times. What about using `gAttachmentBucket`/`gAttachmentArea`/`gAttachmentPane` or even just `gAttachmentList`? I think `gAttachmentList` would be much more readable and allow for more condensed code. I understand that `gMsgAttachmentElement` was chosen for consistency, but still... just saying. Even `gMsgAttachmentList` would be a bit better... `gAttachmentPane` or `gMsgAttachmentPane` are also good as they use the official name of this area.

- Are we sure that we want a localized access key for adding attachments instead of the current, international Ctrl+Shift+A? (patch has Alt+A for English, will be something else for other languages). There's pros and cons.
- When I add more attachments than the first row can show, they disappear into nowhere, and there's no scrollbar. See Bug 1428977 for some ideas to handle just-added attachments better and make them stand out as a success feedback. I also think that we should auto-increase the height of full attachment pane as more attachments get added, up to something like 3 rows at least.
- Failing to navigate from left to right and vice versa using the respective cursor keys (right/left) - existing widget bug, now more exposed where it matters.
- Please restore access to "Initially show attachment pane" feature (was available via right-click context menu on attachment header bar, as in message reader).
- Before this patch, when dragging file to addressing area, attachment area would open up to indicate landing zone. Now it's much harder to know where to drop the file - no explicit indication on addressing area, body is a false positive (does nothing when file dropped), and attachment button is a pretty small drop target. I appreciate that there's more plans for this.
- Please restore access or shortcut key to focus attachment pane - that's the swiss knife to access full functionality and/or contents of the pane via keyboard (esp. via pane/file context). Alt+M versatile access key was quite helpful. F6 is pretty undiscoverable so moving focus to the full pane has become much harder, almost impossible. Full pane access is much-needed for actions like renaming, reordering, double-checking attachments, switch on manual attachment reminder etc.
- In the new implementation, the "Attach" button is now disconnected from the attachment pane - not ideal. Again, this was carefully crafted for the existing area, to have the button near where the files are which you add from that button. See my next point.
- Maybe we could have an inline iconic (+) or [Add...] button next to existing attachments. E.g. on the left top inside the pane, so that it'll be always available. Or an "Add..." link on the header bar, after (size). Let's experiment!
- Speaking of functionality on the header, how about having a [≡] button on the right side of the attachment header bar to conveniently access pane context menu via mouse (even keyboard!). Pane context has useful actions different from file context, e.g. remove all, remind me later, attach web page.

Comment 27

[Alessandro Castellani [:aleca]]

I have a big question for this patch: Can we please stick to agreed procedures and avoid combining a massive code cleanup/rewrite (without functional changes) with new feature/UI work?

I'll let Magnus decide on this, no problem on my end in doing the code clean up in a separated bug prior to this.

That variable is pretty bulky btw...

I agree. gAttachmentPane sounds and looks better.

Are we sure that we want a localized access key for adding attachments instead of the current, international Ctrl+Shift+A?

This combination doesn't work on Linux when the message body has focus.
We also can't be 100% sure that the A hasn't been translated in other languages because that key comes from a DTD file, which if I'm not wrong can be translated.

I'd be okay with not using the accesskey on the button and keeping those keys as standalone and not associated to a button string.

When I add more attachments than the first row can show, they disappear into nowhere, and there's no scrollbar...I also think that we should auto-increase the height...

I agree with this. The container should grow up to a max-height to show as many attachments as possible, since we have the space.

Failing to navigate from left to right and vice versa...

I'll fix this, alongside the popup panel for reordering.

Please restore access to "Initially show attachment pane" feature.

I'm against this.
It doesn't make sense having an open empty attachment pane.
The drop area to accept attachment needs to be extended to the entire compose window, with a "smart" option to let the user decide if the file needs to be attached or added inline the body (like I originally proposed in this mock-up: https://bug1625263.bmoattachments.org/attachment.cgi?id=9145662).
I was planning to that in a follow up bug.

Please restore access or shortcut key to focus attachment pane...Alt+M versatile access key was quite helpful...

I find it very cumbersome and not intuitive. An access key or shortcut should do one thing and not change behaviour based on the situation.
Currently, Alt+M opens the attachment pane, if the pane is empty it focuses it and you can hit Enter to prompt the file picker. If the attachment pane is opened and empty, and you hit Alt+M again it prompts the upload, but if there are attachments it toggles off the attachment pane, and if you open it again it focuses on the first attachment so if you hit Enter it prompts to save the attached file.
This is too strange. An accesskey or shortcut shouldn't be a swiss army knife.

With this patch, we establish a consistent and expected behaviour with quicker interactions:

• Alt+A to always prompt the file picker.
• Alt+M to always and only toggle on and off the attachment pane, and focus on it.
  That's it.

In the new implementation, the "Attach" button is now disconnected from the attachment pane - not ideal

I don't see this as a problem.
Having the "Attach" button close to the Attachment Pane doesn't really bring any benefit as the 2 elements don't need to be interacted with at the same time, and the button doesn't change based on the status of the attachment, or vice versa.
The 2 elements as mostly unrelated.

In terms of the vertical space issue

Also this, IMHO, is a non issue.

I find the current location of the attachment pane way worse than the proposed solution.
The bucket is cramped and its sizing is determined by the entire addressing area and expanding the bucket shrinks the addressing fields.
The overall approach of having horizontal files organized in a vertical way is very wrong. File names are cut, when the panel is collapsed the solution was to show a massive clip icon because having text there to represent the amount of attachments and the file size would eat up more precious space.

The new attachment pane, when collapsed, takes a total of 38px in height. That's basically 1 row of text.
It doesn't affect the spacing of the body as much as the current implementation affects the addressing area.
Even for a user using a laptop with 768px height that doesn't create any issue tot he message body area.

Comment 28

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Here's an updated patch with improvements.

• The left and right arrow keys now work in the pane to properly navigate the various attachments.
• The reordering pane has been updated to match the new horizontal layout.
• A simple animation appears when new attachments are added.
• When a new attachment is added, the latest file is selected and bucket scrolls to ensure visibility.

This works great IMHO, the updated shorctuts work consistently anywhere, and it's way easier to manage large amount of attachments.
I'll deal with the improvements to the attachment drop area in a follow up bug.

Comment 29

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #28)

Created attachment 9198055 [details] [diff] [review]
1640760-attachment-pane.diff

Here's an updated patch with improvements.

• A simple animation appears when new attachments are added.

Nice gimmick. Should it also be shown when an attachment is deleted again?

• When a new attachment is added, the latest file is selected and bucket scrolls to ensure visibility.

Good

This works great IMHO, the updated shorctuts work consistently anywhere, and it's way easier to manage large amount of attachments.
I'll deal with the improvements to the attachment drop area in a follow up bug.

The in the reorder panel shown shortcuts ALT-left arrow or the ALT-right arrow don't move the attachments to the right or left. They move the selection. Tried on Linux and Windows. ALT-home and ALT-end are working.

Comment 30

[Alessandro Castellani [:aleca]]

Nice gimmick. Should it also be shown when an attachment is deleted again?

No, I don't think it's necessary since the delete action is done by the user right in the bucket.
The disappearing element that was selected is enough of a visual feedback.

The in the reorder panel shown shortcuts ALT-left arrow or the ALT-right arrow don't move the attachments

Good find, I'll fix it.

Comment 31

[Alessandro Castellani [:aleca]]

Comment on attachment 9198055 [details] [diff] [review]
1640760-attachment-pane.diff

Removing the flags as I found a small JS error.
Sorry for the noise.

Comment 32

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Comment 33

[Richard Marti (:Paenglab)]

Comment on attachment 9198235 [details] [diff] [review]
1640760-attachment-pane.diff

Rearranging works now with the shortcuts.

But there is a problem with adding the selection through keyboard. This works only with SHIFT-up/down and not with SHIFT-left/right.

In the reader view too all navigation goes through up/down and should be left/right. Follow-up bug?

Comment 34

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Good catch, I fixed it.

In the reader view too all navigation goes through up/down and should be left/right. Follow-up bug?

Yes, I'll deal with that in a follow up bug if this gets approved.

Comment 35

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #27)

I have a big question for this patch: Can we please stick to agreed procedures and avoid combining a massive code cleanup/rewrite (without functional changes) with new feature/UI work?

I'll let Magnus decide on this, no problem on my end in doing the code clean up in a separated bug prior to this.

Hey Alessandro, thank you for indicating that it's no problem for you to split out the code cleanup and indenting changes into a separate patch (and yeah, ideally on a separate bug). I know it's boring, but I think that would make reviewer's life here (including my feedback which you've asked for) a LOT easier because with the current patch, it's all but impossible to identify the relevant changes of the actual UI refactoring between loads of unrelated little rewrites and indenting changes. It's easy to overlook flaws in this combined patch. It will also save us from a lot of trouble if either the cleanup or the refactoring happen to break something - if we land them separately, it will be much easier to identify the culprit and to avoid searching the needle in the haystack. I guess there's a reason for one-issue-per-bug, and having a "task" flavor for code cleanup bugs. Could you do that? Pretty-please... Tia!!!

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 9198295 [details] [diff] [review]
1640760-attachment-pane.diff

Review of attachment 9198295 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed it's a bit of a massive patch, so please in the future keep it more incremental. I don't know if it's worth splitting up at this point.

Anyway, I kind of like how this works now. 
There's a bug, that when you add, say 10 attachments, you only still have one row showing (in "expanded view"), and it doesn't even show a scrollbar so it's rather confusing what's going on.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4075,5 @@
>      "obs_documentCreated"
>    );
>    UnloadCommandUpdateHandlers();
>  
> +  gAttachmentCounter.removeEventListener(

no need to remove normal eventlisteners, as they just go away with the unload.

@@ +5959,5 @@
>    if (addedAttachments.length > 0) {
> +    // Trigger a visual feedback to let the user know how many attachments have
> +    // been added.
> +    document.l10n.setAttributes(
> +      gAttachmentCounter,

If it's always just going to be a number, just use element.value = "+" + count

@@ +6596,5 @@
>  
> +    if (shown && !bucketHasFocus) {
> +      // Interrupt and move the focus to the attachment pane if it's already
> +      // visible but not currently focused.
> +      moveFocusToAttachmentPane();

I think in the original code the intention has been to be able to add an attachment, then go on with composition, and such. That would now not work like that

::: mail/components/compose/content/messengercompose.xhtml
@@ +2144,5 @@
> +                 ondragover="envelopeDragObserver.onDragOver(event);"
> +                 ondrop="envelopeDragObserver.onDrop(event);"
> +                 ondragexit="envelopeDragObserver.onDragExit(event);">
> +          <hbox id="msgheaderstoolbar-box" flex="1">
> +            <vbox flex="1" id="addresses-box">

while here, move id first

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +81,5 @@
> +
> +# Reorder Attachment Panel
> +
> +move-attachment-first-panel-button =
> +    .label = Move to First

Move First

@@ +87,5 @@
> +    .label = Move Left
> +move-attachment-right-panel-button =
> +    .label = Move Right
> +move-attachment-last-panel-button =
> +    .label = Move to Last

Move Last

Comment 37

[Magnus Melin [:mkmelin]]

Might also want to do bug 1625263 before landing this, so that that functionality stays available.

Comment 38

[Alessandro Castellani [:aleca]]

Could you do that? Pretty-please... Tia!!!

Agreed it's a bit of a massive patch, so please in the future keep it more incremental. I don't know if it's worth splitting up at this point.

No problem, I'm splitting the patch starting with a simple code clean up before implementing the UI and UX changes.
The code clean up was necessary to me in order to understand and properly update everything as at this state it is a bit confusing.

Might also want to do bug 1625263 before landing this, so that that functionality stays available.

Uh? bug 1625263 landed months ago. Am I missing something?

Comment 39

[Magnus Melin [:mkmelin]]

Sorry I mean the https://bug1625263.bmoattachments.org/attachment.cgi?id=9145662 functionality from that bug's attachment

Comment 40

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #39)

Sorry I mean the https://bug1625263.bmoattachments.org/attachment.cgi?id=9145662 functionality from that bug's attachment

Ah yes, I can definitely do that.

Comment 41

[Jaise]

Is there plan to improve copy / paste from clipboard or mail in attachment pane. It will be very nice, if copy paste option available.

I have faced an issue while working from home. I used remote desktop from mobile. If you want to attach mail, drag & drop is very difficult task.
But in Thunderbird, drag & drop is only option to attach a full mail as attachment.

Comment 42

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #33)

But there is a problem with adding the selection through keyboard. This works only with SHIFT-up/down and not with SHIFT-left/right.

Pls also ensure that Ctrl+cursor works correctly (just saying, haven't tested).

Comment 43

[Alessandro Castellani [:aleca]]

Is there plan to improve copy / paste from clipboard or mail in attachment pane. It will be very nice, if copy paste option available.

I will see if I can tackle that as a follow up, thanks for the request.

Pls also ensure that Ctrl+cursor works correctly (just saying, haven't tested).

Yup, that works as expected.

Comment 44

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

I'm uploading this WIP patch to gather some feedback as this is getting a bit complicated.
Apologies in advance for the wall of text.

What works and what's good
New accelerators have been implemented which now work anywhere and are always consistent:

• Ctrl+Shift+A: Trigger the file picker.
• Ctrl+Shift+M: Toggle the visibility of the attachment pane, only if not empty.

This combination works very well since it removes the problem caused by the accesskey, and the previous shortcuts which worked inconsistently.
One of the many problems was that, if the user had the Contacts sidebar opened and the Attachment notification reminder visible, the Alt+A of the "Add an Attachment" button in the notification was overriding the Alt+A used in the "Add to To:" button.

These shortcuts are well represented in the tooltiptext (which we can convert to title tags after the top level HTML implementation) and the acceltext attribute in the menu items of the main menu bar.
With this combination, the shortcuts are discoverable by both mouse users and keyboard users, as well as being readable by screen readers.

Reordering and moving across attachments in the horizontal pane works well.
A new indicator is now used when dragging attachment to reorder them.

The Attachment Bucket grows when many attachments are added, making it more obvious that the list is growing, with the focus always moved to the latest attached file.

What doesn't work
The drop area on the whole window is nice at first glance, but super broken as it adds some complexity in order to guarantee an optimal user experience for mouse users.

We currently don't check if the dropped file is an image or not, so the "Add inline" option doesn't work yet. I'm planning to have that option visible only if the dragged file is a type that can be displayed inline the content body.

Another problem, which is the biggest one so far, is that if you highlight some text and drag it around the content area (for example to move a paragraph above another one), the code thinks something is getting dropped and it triggers the overlay, which is super annoying.

Also dragging some text and dropping it into the content body doesn't work, but that's a similar situation as described above.
The plan here is to disable the whole drag&drop area if the content dragged comes from the editor (e.g.: the user is moving an inline image around the content), or if simple text is being dragged from outside.

What do you think?
This is a bit complicated but if we can craft it perfectly, it will bring a massive usability improvement. Right now the section is not as dynamic as it needs to be, so more work is necessary.

Apologies, as this is taking me longer than expected.

Comment 45

[Richard Marti (:Paenglab)]

Comment on attachment 9199437 [details] [diff] [review]
1640760-attachment-pane.diff

Looks already very good.

(In reply to Alessandro Castellani (:aleca) from comment #44)

Created attachment 9199437 [details] [diff] [review]
1640760-attachment-pane.diff

New accelerators have been implemented which now work anywhere and are always consistent:

• Ctrl+Shift+A: Trigger the file picker.
• Ctrl+Shift+M: Toggle the visibility of the attachment pane, only if not empty.

Thanks

Reordering and moving across attachments in the horizontal pane works well.
A new indicator is now used when dragging attachment to reorder them.

Nice, maybe you could move the indicator a bit to the left as it is too near to the attachment type icon and isn't good visible then.

The Attachment Bucket grows when many attachments are added, making it more obvious that the list is growing, with the focus always moved to the latest attached file.

On Windows is grows but the first attachment row is then always half hidden by the attachment toolbar. Not tested on other platforms.

What doesn't work
The drop area on the whole window is nice at first glance, but super broken as it adds some complexity in order to guarantee an optimal user experience for mouse users.

We currently don't check if the dropped file is an image or not, so the "Add inline" option doesn't work yet. I'm planning to have that option visible only if the dragged file is a type that can be displayed inline the content body.

The drop overlay looks very good. It gives a good feedback what happens when you drop an attachment over the composer. Maybe you could hide the inline attachment box until this works, when you implement it in a follow-up bug.

Comment 46

[Alessandro Castellani [:aleca]]

I found a solution to prevent the attachment overlay when dragging elements from within the message body.
This includes even images, so inline images reordered around the message body won't trigger the upload.

Things I need to complete:

• Prevent the drop area overlay from appearing if the user is dragging plain text from outside the message compose window.
• Enable the "Append inline" feature only for dragged images.

Slowly getting there...

Comment 47

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

All right, I think this is good as I hopefully handled all the possible scenarios.

When testing this, especially when trying the drag and drop, please be sure to test these scenarios:

• Drag an image and test the 2 dropping areas.
• Drag a non image file and confirm no "add inline" area is visible.
• Confirm that the overlay doesn't appear when dragging simple text, pills, addresses from the contact sidebar, HTML text, and any other thing that is not a file.
• Confirm that the new overlay doesn't interfere with the current features, like dragging and reordering pills, dropping addresses from the contact sidebar, or selecting and dragging content inside the message body to reorder it.

Richard, I moved the drop indicator for the attachments to the left by a couple of pixels. I can't do more otherwise it will disappear outside the bucket for the first item.
Please, let me know how it looks on Windows and with the dark theme variation.

Magnus, please let me know if the implementation of adding images inline is correct. I never interacted with this area before and I tried to look how we do things in the "Insert image" dialog.
I'm not sure all those files manipulations I do are correct and if there's a smarter way.

Overall, I think this works great and I really enjoy using it while testing it.
One thing I'd like to implement is a drop indicator for when you reorder elements inside the message body, as of now there's no hint on where a dropped text or image might end up.

Apologies for another big patch.

Comment 48

[Wayne Mery (:wsmwk)]

(In reply to Alessandro Castellani (:aleca) from comment #27)

In the new implementation, the "Attach" button is now disconnected from the attachment pane - not ideal

I don't see this as a problem.
Having the "Attach" button close to the Attachment Pane doesn't really bring any benefit as the 2 elements don't need to be interacted with at the same time, and the button doesn't change based on the status of the attachment, or vice versa.
The 2 elements as mostly unrelated.

Will the attach button move next to other buttons, or remain right aligned?

Staying right as a transition seems fine, I'm just curious. Also, you could move it during the beta process and see how many users care.

In terms of the vertical space issue

Also this, IMHO, is a non issue.

I find the current location of the attachment pane way worse than the proposed solution.
The bucket is cramped and its sizing is determined by the entire addressing area and expanding the bucket shrinks the addressing fields.
The overall approach of having horizontal files organized in a vertical way is very wrong. File names are cut, when the panel is collapsed the solution was to show a massive clip icon because having text there to represent the amount of attachments and the file size would eat up more precious space.

FWIW, I agree with this rationale.

Comment 49

[Richard Marti (:Paenglab)]

Comment on attachment 9199987 [details] [diff] [review]
1640760-attachment-pane.diff

Tested different things and all looks good except when dragging an image file on a plain text message. It still asks if the file should be attached inline. When I attach it inline, it is shown in composer but when sending it is no more there -> data loss.

Comment 50

[Alessandro Castellani [:aleca]]

It still asks if the file should be attached inline. When I attach it inline, it is shown in composer but when sending it is no more there -> data loss.

Ah, good catch.

Comment 51

[Magnus Melin [:mkmelin]]

Comment on attachment 9199987 [details] [diff] [review]
1640760-attachment-pane.diff

Review of attachment 9199987 [details] [diff] [review]:
-----------------------------------------------------------------

If you have many attachments, and the toggle closed the attachmentBucket, opening it again is just one row, not a few as expected.

Works quite nicely in general.

::: mail/base/content/mailWidgets.js
@@ +1593,5 @@
>        textContainer.appendChild(textName);
>        textContainer.appendChild(spacer);
>        textContainer.appendChild(sizeLabel);
>  
> +      let dropIndicatorBefore = document.createXULElement("image");

can we use an html:img instead?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +7818,5 @@
> +          if (event.target.baseURI != "about:blank?compose") {
> +            DropRecipient(event.target, data);
> +            // Since we are now using ondrop (eDrop) instead of previously using
> +            // ondragdrop (eLegacyDragDrop), we must prevent the default
> +            // which is dropping the address text into the widget.

Please update the comment to not talk about the past anymore.

@@ +7856,5 @@
>  
>    onDragOver(event) {
> +    let dragSession = gDragService.getCurrentSession();
> +
> +    // Check if we're draggin from the attachment bucket onto itselt.

dragging. itself

@@ +7888,5 @@
>  
>      for (let flavour of flavours) {
> +      if (!dragSession.isDataFlavorSupported(flavour)) {
> +        continue;
> +      }

while were here, make the spelling consistenly flavor without an u

@@ +7896,5 @@
>          event.stopPropagation();
>          event.preventDefault();
> +        document.getElementById("dropAttachmentOverlay").classList.add("show");
> +
> +        // Show the #addInline box only if the user is draggin only images.

g missing

@@ +7935,5 @@
> +  /**
> +   * Loop through all the images that have been dropped above the #addInline
> +   * box and create an image element to append to the message body.
> +   *
> +   * @param {Object} dataTransfer - The dataTransfer object from the drop event.

@param {DataTransfer}

@@ +8007,5 @@
> +    }
> +    box.classList.remove("hover");
> +  }
> +
> +  target.classList.add("hover");

Can we avoid the js things for hover and not using css :hover?

::: mail/components/compose/content/messengercompose.xhtml
@@ +2476,5 @@
> +                      ondragenter="addHighlightDropBox(this);"
> +                      ondragexit="removeHighlightDropBox(this);">
> +            <html:label data-l10n-id="drop-file-label-attachment"
> +                        class="add-attachment-label">
> +            </html:label>

Doesn't look like this should be an html:label. 
Probably the content of the aside?

@@ +2482,5 @@
> +          <html:aside id="addInline" class="drop-attachment-box"
> +                      ondragenter="addHighlightDropBox(this);"
> +                      ondragexit="removeHighlightDropBox(this);">
> +            <html:label id="addInlineLabel"
> +                        data-l10n-id="drop-file-label-inline"></html:label>

same here. html labels are for form control associations.

You should also use the single/multiple items fluent way.
Adding one or many could be different wording in many languages

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +48,5 @@
> +# Attachment widget
> +
> +# International shortcut keys. Do not translate!
> +key-toggle-attachment-pane = M
> +key-trigger-attachment-picker = A

The convention is to use .key, like https://searchfox.org/comm-central/source/mozilla/browser/locales/en-US/browser/pageInfo.ftl#9
(the notice above seems unwarranted)

Comment 52

[Alessandro Castellani [:aleca]]

target.classList.add("hover");

Can we avoid the js things for hover and not using css :hover?

I tried, but unfortunately it doesn't seem to work when the dragging is in progress.
I'll see if I can find out why.

You should also use the single/multiple items fluent way.
Adding one or many could be different wording in many languages

Ah, good catch, I'll do that.

key-trigger-attachment-picker = A

The convention is to use .key

If I do that, how then can I get the key in JS in the setKeyboardShortcuts() method?
Using the ID "key-trigger-attachment-picker.key" doesn't seem to work.

Comment 53

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Updated everything, except for the .key fluent variation as we need those keys to be used in JavaScript, and we're not using those fluent strings inside a <key> XUL element.

Comment 54

[Richard Marti (:Paenglab)]

Comment on attachment 9200791 [details] [diff] [review]
1640760-attachment-pane.diff

Review of attachment 9200791 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/themes/shared/mail/attachmentList.css
@@ +41,5 @@
> +#attachmentBucket .attachmentItem .attach-drop-indicator {
> +  position: absolute;
> +  z-index: 3;
> +  display: none;
> +  margin: -6px -6px -4px -6px;

This could be
`margin: -6px -6px -4px;`

::: mail/themes/shared/mail/messengercompose.css
@@ +315,5 @@
> +  justify-content: space-around;
> +}
> +
> +:root[lwt-tree] .drop-attachment-overlay {
> +  background-color: rgba(0, 0, 0, 0.75);

This applies also to light LW-themes.
Have you meant to use `:root[lwt-tree-brighttext]`?

@@ +352,3 @@
>  }
>  
> +:root[lwt-tree] .drop-attachment-box:not(.hover) {

Applies to light LW-themes too.

Comment 55

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Thanks Richard for the ui-r+ and the suggested fixes

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 9200939 [details] [diff] [review]
1640760-attachment-pane.diff

Review of attachment 9200939 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few small things.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4083,5 @@
> +    { id: "toggle-attachment-pane-key" },
> +  ]);
> +
> +  document.addEventListener("keypress", event => {
> +    // Interrupt if we don't have the correct combination of CTRL/CMD + SHIFT.

Maybe "Don't do anything if..."

@@ +6746,5 @@
> + *
> + * @param {Event} event - The DOM Event.
> + */
> +function onToggleAttachmentPane(event) {
> +  // Interrupt if it's not a left click.

Skip if... ?

@@ +7028,1 @@
>  function attachmentBucketMarkEmptyBucket() {

I wonder if we can remove this function altogether, and use css :empty instead here: https://searchfox.org/comm-central/search?q=%5Bempty%5D&path=&case=false&regexp=false

@@ +7939,5 @@
> +   * @return {boolean} True if at least 1 file not an image.
> +   */
> +  isNotDraggingOnlyImages(dataTransfer) {
> +    for (let file of dataTransfer.files) {
> +      return !file.type.includes("image/");

doesn't seem to do as documented. this will return true/false based on the first file only

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +99,2 @@
>  
> +drop-file-label-inline = Append inline

This is not set up for one-vs-many in localizations, like drop-file-label-attachment

Comment 57

[Alessandro Castellani [:aleca]]

function attachmentBucketMarkEmptyBucket() {

I wonder if we can remove this function altogether, and use css :empty instead here: https://searchfox.org/comm-central/search?q=%5Bempty%5D&path=&case=false&regexp=false

Indeed, we can remove this altogether since we're not showing an empty bucket anymore as the dropping area is the entire compose window.

Comment 58

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Patch updated with the requested fixes, and another try run launched to be sure: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ce49e26da65bab5d385e7f759d0076fd69324b2c

Comment 60

[Magnus Melin [:mkmelin]]

comm/mail/test/browser/composition/browser_focus.js (at least is broken)

Comment 61

[Alessandro Castellani [:aleca]]

Yup, I just saw that, fixing it.

Comment 62

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Tests fixed locally.
Let's see if I got all of them: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=dee961eef5c74929f477e300d2ea8ee4a1613f0f

Comment 63

[Alessandro Castellani [:aleca]]

One macOS failure still standing...

Comment 64

[Alessandro Castellani [:aleca]]

There's something sketchy going on with macOS shortcuts.
All of a sudden the CTRL+ combination works, but it shouldn't, and when I hit the Command+ shortcut I get this error:

2021-02-10 20:10:55.638 thunderbird[29991:901490] Unable to find object to handle service <_NSServiceEntry - 0x13815e9e0:
	bundleIdentifier:		com.apple.Terminal
	bundlePath:			/System/Applications/Utilities/Terminal.app
	executablePath:			(null)
	defaultTitle:			Search man Page Index in Terminal
	localizedTitleWithoutSubstitutions:			Search man Page Index in Terminal
	defaultKeyboardShortcut:	<NSKeyboardShortcut: 0x1310fd2e0 (⇧⌘A)>
	keyboardShortcut:		<NSKeyboardShortcut: 0x1310fd2e0 (⇧⌘A)>
	message:			searchManPages
	portName:			Terminal
	sendPasteboardTypes:		[public.plain-text]
	returnTypes:			[(null)]
	userData:			(null)
	languages:			[(null)]
	required context: (
    "<_NSServiceFilter: 0x1363fec40 word limit: 20>"
)>

Comment 65

[Alessandro Castellani [:aleca]]

I figured this out with the help of :arndissler on Matrix.

macOS comes with ⇧⌘A and ⇧⌘M defined as Universal shortcut at the OS level to access the Man Page in terminal.
It seems the the XUL key intercepts these keys on keydown and prevents the propagation to the OS.

I'm reworking this to make it work on macOS as well, and I'm implementing the keydown listener, also because the keypress listener is deprecated.

Comment 66

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

I updated the patch to fix the macOS issues explained in the message above.

I'm asking another round of reviews because I changed from keypress to keydown as listener in order to make everything work properly.
I also updated a couple of event listeners we have attached to the pills and recipients fields that were interfering with the Attachment Bucket (eg. pressing Ctrl+Shift+A on a field with many pills selected all the pills and also opened the file picker).

Another issue I found was the drop indicator being always visible in the attachment bucket in the message pane, since we use the same CE for message and compose, the CSS needed to be updated to account for that.

I'm asking for an extra review from Richard to be sure the behavior is consistent also on macOS and Windows.

Here's another try-run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=17058ee05b49a82f5c3ef6316a24320b9d4eac99

Comment 67

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Another inconsistency between OSs detected.

• Linux and Windows return the uppercase version of a pressed key if the Shift modifier is pressed.
• macOS always returns the lowercase key, even if the Shift modifier is pressed.

Sorry for the noise, here's another try-run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a2f5d02e67500e4e7138e4ed8052ec51bd81d92d

Comment 68

[Alessandro Castellani [:aleca]]

Lots of test failures.
I'll keep the review flags on while I keep fixing those tests.

Comment 69

[Alessandro Castellani [:aleca]]

Attachment: 1640760-attachment-pane.diff

Fixed the focus ring tests.
The major changes from the previous r+ patch are in the setKeyboardShortcuts() method.

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e347790f84e1320a72253216c191bb11f457d117

Comment 70

[Richard Marti (:Paenglab)]

Comment on attachment 9202758 [details] [diff] [review]
1640760-attachment-pane.diff

Tested the on Mac too. :-)

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8bb80263dee6
Refactor and improve the UI of the attachment pane in the message compose window. r=mkmelin, ui-r=Paenglab

Comment 72

[Óvári]

Will this patch enable the compose window to work with small screens (eg Librem 5)?

The Geary email client also has the attachments at the bottom of the compose window.

Thank you

Comment 73

[Alessandro Castellani [:aleca]]

We don't directly target phones as the Thunderbird UI is not fully responsive.
I guess, with the attachment panel at the bottom, it might be easier to use.

Comment 74

[Dave Miller [:justdave]]

I'm going to put this into the release notes as "Message Compose Window: New user interface for adding attachments - see <link>" where link points at a blog post (or a feature section on the website?) that I suggest someone write that has screenshots and whatnot. There's too much going on here for me to make a one-liner out of trying to explain the whole thing. I'll initially post it without the link, we can add the link later if someone writes it up.

Comment 75

[Alessandro Castellani [:aleca]]

That's fair, thanks.

Comment 76

[Thomas D. (:thomas8)]

Attachment: Screenshot 4: The entire composition window as a drag and drop target with drop flavors `Insert inline` vs. `Add as attachment`

So for the benefit of posterity and the annals of history, this is the bug where the awesome new drag and drop interface for attachments was introduced. The screenshot shows how it looks after some more polishing in other bugs.

I've documented this here: New user interface for adding attachments section of the New in Thunderbird 91 SUMO article.

Comment 78

[Thomas D. (:thomas8)]

Drag and drop of attachments onto the compose message body (as implemented here, see comment 76) has been a much-requested feature for 20 years: 8 duplicates on Bug 113435 - Dragging files into text pane of compose window does not add attachment. Thanks Alex!!!

Comment 79

[Alessandro Castellani [:aleca]]

Thank you so much for the documentation!