Closed Bug 1695521 Opened 3 years ago Closed 3 years ago

New UI for adding attachments: "Add as Attachment" | "Append inline" remains if image not dropped


(Thunderbird :: Message Compose Window, defect, P2)

Thunderbird 87


(thunderbird_esr78 unaffected, thunderbird87+ fixed)

88 Branch
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird87 + fixed


(Reporter: klaus.bartosch, Assigned: aleca)




(Keywords: regression, Whiteboard: [workaround: drag an image in and out again slowly until landing zone disappears])


(2 files, 1 obsolete file)

Attached image overlay-permanent.png

Open a new compose window.
Drag an image over it, "Add as Attachment" | "Append inline" overlay appears.
Drag image off, overlay disappears.
Repeat a few times until a the end, the overlay is permanent although the image has been dragged off.

Seen on Windows 10 with TB 87 beta 1.

Confirming on 88.0a1 (2021-02-28) (64-bit), thank you Klaus!

This is easily reproducible by dragging the image in and out of the drop area several times swiftly.
Needs an additional/different strategy for hiding the landing zone. Maybe mouseover check: if it's not drag, remove the landing zone. And probably something for keyboard, too. Or prevent this in the first place. Maybe caused by async race conditions, or logic flaw.

Assignee: nobody → alessandro
Severity: -- → S3
Priority: -- → P3

This is pretty inconvenient when it happens, as the landing zone overlay prevents any mouse interaction with the composition (keyboard works, good way of training yourself for keyboard-only... ;-).

The workaround is simple, but perhaps not easy to discover - just drag an image in and out again slowly until landing zone disappears.

Whiteboard: [workaround: drag an image in and out again slowly until landing zone disappears]

I don't understand your priority/severity system, but this bug renders the Write window pretty much useless, at least visually. Imagine that happens after writing a long email and then trying to add attachments.

Trying to add a new attachment over the blocked message should remove the overlay again and you can work again with your message.

The offered workaround of starting another drag action isn't discoverable. Usually overlays or "full screen mode" can be dismissed by hitting escape, but that doesn't work here either.

I'm increasing the priority/severity level since indeed it's pretty disruptive.
The initial level is mostly due to the fact that this feature is only present in the beta.
I'll take care of this right away.

Severity: S3 → S2
Priority: P3 → P2

Very strange, it seems that I can't reproduce this on Linux.

Nevermind, I lied, I reproduced it.

Attached patch 1695521-attachment-overlay.diff (obsolete) — Splinter Review

This should fix everything.

  • I removed the dragenter and dragexit from the overlay child elements.
  • I'm handling the highlight of the drop bucket (attach/inline) from the parent's dragover event.
  • I'm using a timeout to avoid the jarring on/off effect of the overlay when dragging over child elements (is this okay or there's a better method?).

I was planning to add a test to cover this and be sure the overlay is always hidden, but I'm not sure mochitest supports what I need to do (dragging a file from outside Thundebird).
Is this comment still relevant?

I'm asking a ui-review to Richard since I changed some styling and to be sure the usage of pointer-events: none; doesn't create any issue.

Attachment #9206194 - Flags: ui-review?(richard.marti)
Attachment #9206194 - Flags: review?(mkmelin+mozilla)
Blocks: 1695579
Blocks: 1695641

Comment on attachment 9206194 [details] [diff] [review]

Looks good and couldn't make the overlay stay when not dragging the file over it (before with multiple times dragging the file over and out it stayed sometimes).

Attachment #9206194 - Flags: ui-review?(richard.marti) → ui-review+

The code comment seems obsolete. You should be able to simulate a drag in mochitest, something like this:

Comment on attachment 9206194 [details] [diff] [review]

Review of attachment 9206194 [details] [diff] [review]:

::: mail/components/compose/content/MsgComposeCommands.js
@@ +149,5 @@
>  var kAttachmentHeight;
> +// Boolean variable to keep track of the dragging action of files above the
> +// compose window.
> +var kIsDragginAttachments;

We use g for global and k for constant (kAttachmentHeight is also wrongly named here)

Also, missing a g ;)

@@ +7989,5 @@
> +
> +    // We use a timeout since a drag leave event might occur also when the drag
> +    // motion passes above a child element and doesn't actually leave the
> +    // compose window.
> +    setTimeout(() => {

I guess this could just use a css transition?
Attachment #9206194 - Flags: review?(mkmelin+mozilla) → feedback+
Target Milestone: --- → 88 Branch

Unfortunately I don't think I can remove the setTimeout as the dragenter and dragleave events fire non stop when passing over a child element of the overlay. Super annoying.

I implemented some subtle fade in and out transitions for the whole area and also added support for the reduced motion media query.

Attachment #9206194 - Attachment is obsolete: true
Attachment #9206775 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9206775 [details] [diff] [review]

Review of attachment 9206775 [details] [diff] [review]:

Thx! r=mkmelin
Attachment #9206775 - Flags: review?(mkmelin+mozilla) → review+

Pushed by
Fix attachment overlay remaining visible after image is dragged out. r=mkmelin

Closed: 3 years ago
Resolution: --- → FIXED
Blocks: 1696489

Comment on attachment 9206775 [details] [diff] [review]

[Approval Request Comment]
Regression caused by (bug #): bug 1640760
User impact if declined: the new attachment overlay remains if the drag and drop action is too fast, preventing the usage of the message compose.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low.

Attachment #9206775 - Flags: approval-comm-beta?

Comment on attachment 9206775 [details] [diff] [review]

[Triage Comment]
Approved for beta

Attachment #9206775 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.