Closed Bug 1627166 Opened 5 years ago Closed 5 years ago

Addressing widget not disabled for user input during send

Categories

(Thunderbird :: Message Compose Window, defect)

Product:
Thunderbird
Component:
Message Compose Window
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 77.0

People

(Reporter: jorgk-bmo, Assigned: aleca)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

1627166-disable-onsend.diff
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review

STR:
Compose a message, add a few recipients
Add some heavy attachments so sending will take some time to complete
Press send.

Observe that the addressing widget is not disabled, so you can change the recipients while sending is in progress. I don't know which effect that will have once sending completes and the sent message is stored.

In any case, this is a regression. The addressing area was always locked from change after the Send button was pressed.

Note that all other actions in the compose window (except the new recipients area) are using commands, where things like auto-disabling/enabling are very easy to handle in the central command controller, which is observed by all the command consumers. I'm aware that commands logic is part of XUL, but imo it's well thought-out and highly convenient once fully understood, reducing the burden on coders to handle everything directly.

Assignee: nobody → alessandro
Status: NEW → ASSIGNED

Thanks Jorg for the report.
This is very strange as I remember specifically tackling this aspect when implementing the new addressing widget.
It has been regressed along the way buy other updates, uff.

All right, I'll take care of it right away!

Also, shouldn't be a test covering this scenario?
If not, I will add it because this is quite important.

I found the problem, this is mostly a styling issue.
The input fields get properly disabled but the UI doesn't update to reflect it since the addressing row is a div container and not a real input.
The only real issue is that the msgIdentity menulist doesn't get properly disabled since that's a XUL element and not pure HTML.
A fix is coming.

Attached patch 1627166-disable-onsend.diff (obsolete) (deleted) — — Splinter Review
Attachment #9138147 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9138147 [details] [diff] [review] 1627166-disable-onsend.diff Review of attachment 9138147 [details] [diff] [review]: ----------------------------------------------------------------- Does this patch disable all of the following? - pill context menu commands (ideally that menu shouldn't even show) - selecting pills via mouse - keyboard access to selected pills - removing rows with pills via [x] button (mouse/keyboard) Currently (without this patch) I am able to do any of the following during sending: - select pills with mouse - remove selected recipients via pill context, DEL, Backspace - start pill editing via pill context / Enter (though it mostly won't edit) - move-to-to/cc/bcc via pill context (successful) - cut/copy pills via context menu, Ctrl+C/Ctrl+X (only paste fails) - remove entire recipient rows via [x], including their pills

On visual inspection, a bunch more elements receive disableonsend="true", so yes, they should all be disabled now.

Comment on attachment 9138147 [details] [diff] [review] 1627166-disable-onsend.diff Review of attachment 9138147 [details] [diff] [review]: ----------------------------------------------------------------- Would indeed be good to also disable the context menu of the pills and removal of addressing rows.
Attachment #9138147 - Flags: review?(mkmelin+mozilla)

On it.

Attached patch 1627166-disable-onsend.diff (deleted) — — Splinter Review

Patch updated to disable all the input fields, labels, and pills.
When a pill is disabled all these actions don't trigger:

  • pill context menu doesn't show
  • unable to select pills via mouse
  • unable to select pills via keyboard
  • unable to remove rows or pills
  • unable to create rows or pills
Attachment #9138147 - Attachment is obsolete: true
Attachment #9138598 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9138598 [details] [diff] [review] 1627166-disable-onsend.diff Review of attachment 9138598 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thx! r=mkmelin
Attachment #9138598 - Flags: review?(mkmelin+mozilla) → review+

The compose tests all pass locally, but I launched try-run just to be sure, even tho we're not changing the behaviour of any section.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=065d9a6d73d5f550407d873e8cc8cda25d6998ae

Keywords: checkin-needed-tb

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2075434642ad
Disable addressing widget rows when sending a message. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 77.0
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: