Pasting of pills should paste as pills, not comma-separated, red recipients text
Categories
(Thunderbird :: Message Compose Window, enhancement)
Tracking
(Not tracked)
People
(Reporter: thomas8, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mkmelin
:
feedback+
thomas8
:
feedback-
|
Details | Diff | Splinter Review |
I am looking at the latest available iteration of bug 440377 (try build 73.0a1 (2019-12-06) (64-bit).*
STR
- To-field, select pill1 and pill2 (both known in your AB)
- Copy or Cut
- Paste into another recipient type field (CC)
Actual result
Expected result
- paste pills, not text, definitely not red text because these are known and correct recipients
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
This patch will automatically create pills on keyup
if multiple addresses separated by a comma have been pasted.
Comment 2•5 years ago
|
||
Comment 3•5 years ago
|
||
Updated.
I need to keep the _setupEventListeners()
method as that event listener need to be set also for potential extra recipients generated from the ComposeLoad()
method.
Comment 4•5 years ago
|
||
Updated•5 years ago
|
Comment 5•5 years ago
|
||
How about moving it to MailAddressPill _setupEventListeners() then?
Mh, no I don't think that's right as those are two separated things.
This is the keyup event listener for the main input fields. The MailAddressPill _setupEventListeners() is for different events happening on the pill.
I guess it's more an issue of naming.
Maybe something like this would make more sense?
input.addEventListener("keyup", event => {
recipientKeyUp(event, input);
});
And moving it in the addressingWidgetOverlay.js
file, which currently holds the other events of the recipient input fields.
Comment 6•5 years ago
|
||
Comment 7•5 years ago
|
||
Mhhh..no, I think you're referring to bug 1602461.
This is tackling only the issue that if the users paste a bunch of addresses in the input field, pills should be generated immediately.
Comment 8•5 years ago
|
||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Mhh, interesting point.
How do we want to handle the undo action?
If we decide to cover for that it might get a bit complicated as we'll need to remember which pill or pills were last created and remove them.
How many history undo do we want to support?
Should we actually handle that? And if we want to, should we maybe deal with that in another bug?
Comment 11•5 years ago
|
||
I think we should handle it here, as without this patch that issue doesn't exist. Don't know off hand how it would be implemented.
Reporter | ||
Comment 12•5 years ago
|
||
Reporter | ||
Comment 13•5 years ago
|
||
Some of the behaviour of this bug will also be required for bug 1602461, so let's keep that in mind.
Reporter | ||
Comment 14•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #11)
I think we should handle it here, as without this patch that issue doesn't exist. Don't know off hand how it would be implemented.
Magnus, is there a way of clearing the undo-buffer of a text field? If yes, what if we did that, i. e. disable undo buffer only for cases where we actually paste pills, so there's no text input to undo? That would avoid the error of "duplication by undo" gracefully. At our current stage of "basic UX" iterations, handling pills-undo would seem to add more complexity than we're able and willing to handle for now. Or maybe the text shouldn't even appear in the originalInput, which would also solve the input-undo problem, see below...
Alessandro, isn't this another instance of "flickering"? From my reading (pls correct me if I'm wrong), Ctrl+V will first paste plain text into the originalInput field, and thereafter (upon Ctrl+V's keyup event), you then convert that already visible text into pills if applicable, which will cause some visual commotion. Shouldn't we catch the Ctrl+V keypress before the text lands in the input and add pills directly if applicable? (Approaching pills-API eg...)
Sorry for playing the devil's advocate and adding complexity, but the devil is really in the detail, and I'm trying to polish the UX whilst we are here so that our new baby will be received with pride and joy... ;-)
Reporter | ||
Comment 15•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) (PTO to 17th Jan 2020, sporadically reading bugmail) from comment #7)
Mhhh..no, I think you're referring to bug 1602461.
This is tackling only the issue that if the users paste a bunch of addresses in the input field, pills should be generated immediately.
(In reply to Magnus Melin [:mkmelin] from comment #6)
Comment on attachment 9119971 [details] [diff] [review]
Actually, shouldn't this listening be on the area, and then just check where
the event came from?
I think Magnus may be right. What's the point of creating small listeners all over when we could just listen for Ctrl+V on the entire recipient field (and perhaps ignore only when we're pasting non-comma-separated text into edit-mode-pills)? The following action (creating new pills from clipboard content if applicable) should be the same for both cases. Can't think of any other cases right now, and we could always check the event source as Magnus points out (e.g. if we have inter-pill insertion points later).
Comment 16•5 years ago
|
||
I'm starting to grow a feeling where we shouldn't touch this, and the only edit we should do is not mark the pasted addresses in red.
The reasons is mostly one and simple: avoid hijacking the normal history.
If we implement this "paste-pills" action, then we'll have to deal with potential ctrl+z
and ctrl+shift+z
actions, and if we go the route of preventing the text to be pasted in the input field on the first place, we're removing the ability to see the pasted text before it gets converted into a pill.
Do we want to do that?
Comment 17•5 years ago
|
||
Possibly yes.
One way to solve the technical problems might be to make the paste go somewhere else instead, somewhere temporary hidden, so that since the filed is removed after the paste completes, there is no issue with an undo either.
Comment 18•5 years ago
|
||
So, here's the plan of action for this bug:
- Listen to the
paste
even on the CE recipient area and not the input field. - Prevent the default behaviour of the
paste
action if the copied text has valid email addresses. - Create and add pills of the valid email addresses without passing through the input field.
Does this sound good?
Can we ignore the undo
and redo
actions with this approach?
Reporter | ||
Comment 19•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #18)
So, here's the plan of action for this bug:
- Listen to the
paste
even on the CE recipient area and not the input field.- Prevent the default behaviour of the
paste
action if the copied text has valid email addresses.- Create and add pills of the valid email addresses without passing through the input field.
Does this sound good?
Sounds good to me. So invalid strings mixed with valid email addresses would come out as error pills. Guess that's ok. Separating the good and the bad (paste good as pills, paste bad as plain input text) would be possible, but also somewhat confusing.
Can we ignore the
undo
andredo
actions with this approach?
I think that's ok. Undo/Redo pills is surely complex enough for another bug on another day.
Reporter | ||
Comment 20•4 years ago
|
||
Mass-changing bugs around the new recipient area (pills) from product/component MailNews Core/Composition
to Thunderbird/Message Compose Window
, because composition frontend code is not shared with SM. Mostly cloned from Bug 440377 which started out in MailNews Core
long back.
20200614002RecipientPillsProductChangeTypeEnhancement
Updated•2 years ago
|
Description
•