Open Bug 1602456 Opened 5 years ago Updated 2 years ago

Pasting of pills should paste as pills, not comma-separated, red recipients text

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: thomas8, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

I am looking at the latest available iteration of bug 440377 (try build 73.0a1 (2019-12-06) (64-bit).*

STR

  1. To-field, select pill1 and pill2 (both known in your AB)
  2. Copy or Cut
  3. 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

*: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=674bb73bd2b093deb8170dc8ecd7d67b5d1239a9

Summary: Pasting of pills should paste pills, not comma-separated, red recipients text → Pasting of pills should paste as pills, not comma-separated, red recipients text
Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Version: unspecified → 73
Attached patch 1602456-pills-paste.patch (obsolete) (deleted) — Splinter Review

This patch will automatically create pills on keyup if multiple addresses separated by a comma have been pasted.

Attachment #9119596 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9119596 [details] [diff] [review] 1602456-pills-paste.patch Review of attachment 9119596 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWidgets.js @@ +3,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > /* import-globals-from ../../components/compose/content/addressingWidgetOverlay.js */ > +/* import-globals-from ../../components/compose/content/MsgComposeCommands.js */ don't think you need this? If you do please don't wholesale include the whole file @@ +2159,5 @@ > input.setAttribute("recipienttype", recipient.type); > input.setAttribute("size", 1); > > setupAutocompleteInput(input, this.highlightNonMatches); > + this._setupEventListeners(input); seems inconsistent and verbose to put this in a separate function. input.addEventListener("keyup", event => { if (event.key == "V" && (event.ctrlKey || event.metaKey) && event.target.value.includes(",")) { recipientAddPill(event.target); } });
Attachment #9119596 - Flags: review?(mkmelin+mozilla)
Attached patch 1602456-pills-paste.patch (obsolete) (deleted) — Splinter Review

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.

Attachment #9119596 - Attachment is obsolete: true
Attachment #9119971 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9119971 [details] [diff] [review] 1602456-pills-paste.patch Review of attachment 9119971 [details] [diff] [review]: ----------------------------------------------------------------- How about moving it to MailAddressPill _setupEventListeners() then?
Attachment #9119971 - Flags: review?(mkmelin+mozilla)

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 on attachment 9119971 [details] [diff] [review] 1602456-pills-paste.patch Review of attachment 9119971 [details] [diff] [review]: ----------------------------------------------------------------- Actually, shouldn't this listening be on the area, and then just check where the event came from?

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.

Attached patch 1602456-pills-paste.patch (deleted) — Splinter Review
Attachment #9119971 - Attachment is obsolete: true
Attachment #9120115 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9120115 [details] [diff] [review] 1602456-pills-paste.patch Review of attachment 9120115 [details] [diff] [review]: ----------------------------------------------------------------- If I press ctrl+z to undo the paste, the pasted pills shows in addition to the textual format.
Attachment #9120115 - Flags: review?(mkmelin+mozilla) → feedback+

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?

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.

Comment on attachment 9120115 [details] [diff] [review] 1602456-pills-paste.patch Review of attachment 9120115 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for looking into this. From my reading, current patch covers some basic cases, but is a bit coarse otherwise: - not covering single pills - significant potential for false positives from plain text in clipboard which doesn't contain valid addresses As a caveat for validity checks, we might be pasting a mix of valid and invalid pills (which should probably just work). I haven't given this any deep thoughts yet, so I can't offer full specs wrt requirements or solutions, just pointing out potential pitfalls. ::: mail/components/compose/content/addressingWidgetOverlay.js @@ +458,5 @@ > +function recipientKeyUp(event) { > + if ( > + event.key == "v" && > + (event.ctrlKey || event.metaKey) && > + event.target.value.includes(",") Requiring comma causes that a single pasted pill will remain text. Not ideal. Don't we also need some basic or even more elaborate checks for false positives? Ideally, only pasted valid email addresses |John Doe <john@foo.bar>, jane@foo.baz| should auto-convert into pills. Random texts like "Johnson, Paul" or "John" shouldn't. Perhaps we already have some syntax-checking function which might be re-used here? As per the above-mentioned caveat, syntax-checking may become more delicate for mix of valid and invalid pills. As a minimum requirement for this bug, we want to ensure that copied pills remain pills after pasting. Maybe that could be achieved by creating a special clipboard flavor for our own pills upon cut/copy and recognizing that upon paste. *Maybe* it might be acceptable when non-pill text from anywhere gets pasted as text first and requires Enter to convert to pills. Or maybe we want to be more helpful and dynamic...
Attachment #9120115 - Flags: feedback-

Some of the behaviour of this bug will also be required for bug 1602461, so let's keep that in mind.

(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... ;-)

(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).

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?

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.

So, here's the plan of action for this bug:

  1. Listen to the paste even on the CE recipient area and not the input field.
  2. Prevent the default behaviour of the paste action if the copied text has valid email addresses.
  3. 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?

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

So, here's the plan of action for this bug:

  1. Listen to the paste even on the CE recipient area and not the input field.
  2. Prevent the default behaviour of the paste action if the copied text has valid email addresses.
  3. 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 and redo actions with this approach?

I think that's ok. Undo/Redo pills is surely complex enough for another bug on another day.

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

Severity: minor → N/A
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
Version: 73 → Trunk
Assignee: alessandro → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: