1602901 - Single left-click on single *selected* pill should edit address

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

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

Those pills can be stubborn when you don't know them yet and are trying to edit...
Let's make it as easy as possible for users to break that resistance of pills so that they can start to love them... ;-)

STR

1. Compose new msg with recipient pills: (John Doe <[e-mail]>)(Jane Doe <[e-mail]>)
2. Try the most intuitive way of editing an existing pill (John):
   • Click once on pill (oh, that selects it, ok).
   • Single-left-click again on selected pill, trying to break the selection (which is a common use pattern, see below).

Actual result

• Nothing

Expected result

• Single left-click on a single selected pill should edit that pill
• Perhaps change the mouse pointer into a small pencil on hover (morphing :aleca's idea of showing a floating pencil icon as a click target)

Benefits

• intuitive and discoverable
• complementary mouse method of editing pill (in addition to double-click and context menu)
• make "slow" double-clicks succeed
• Note that this must only work on a single selected pill (not for multiple selected pills), and prior focus is NOT required (as it comes with that second click). Cf. Windows Explorer behaviour.

Rationale:

• I think this could significantly ease the transition from the current "editable text" recipients, because it would mimick the currently available click pattern of TB 68 exactly:
  • Since my Bug 1527547, TB helpfully auto-selects recipients when clicked or cursored into.
  • So first click selects (pill-style), and next click edits.
• Clicking on selected things to edit their text is a common click pattern not only in current TB:
  • edit name of selected item in Windows Explorer
  • edit URL in FF
  • collapse selection to edit text in any application

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

Comment 1

[Magnus Melin [:mkmelin]]

OTOH, it's not the normal case that you want to edit.
Maybe we want to instead look ahead: maybe there should be a nice popup showing everything we know about that contact, and there there would be an edit button too.

Comment 2

[Richard Marti (:Paenglab)]

Yes, like the popup we have for the addresses in the message header in main window.

Comment 3

[Thomas D. (:thomas8)]

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

Comment 5

[Thomas D. (:thomas8)]

Attachment: 1602901_pill-left-click-edit.diff

Sweet and simple - edit address made easy!

Motivation: We've had several user reports, the latest being duplicate bug 1691255, where users don't know how to edit a pill. Some reporting users are advanced users. Sometimes we make our users suffer for no reason. This fixes the problem by improving discovery without any side effects.

Now the "delayed double-click" will also work to edit an address:

• First click selects the pill ("oh, but I want to edit! Let me click again to break that selection!");
  another deliberate click on the selected pill to edit.
• Established use pattern for editing selected item text (e.g. rename selected file in Windows Explorer) as well as any selected text (e.g. Firefox location bar, TB 68 recipient input, any text input).
• Greatly improves ux-discovery.
• Also eases the transition for TB 68 muscle memory (same click pattern).

(In reply to comment #1 and Richard Marti (:Paenglab) from comment #2)

Yes, like the popup we have for [clicking] addresses in the message header in main window.

FTR: This patch is not prejudicial to having auto-popup menus in the future:

• Wrt discovery and ease of use, auto-popup menus only make sense on hover or first click, whereas here we're handling the second click on an already selected item.

Fwiw: Note that 3-pane auto-popups are different because currently, we do not allow selecting recipients in message reader. Here in composition we do support selecting recipient items, so having large auto-popups in composition which will cover other recipients and prevent their selection is a no-go.

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9201740 [details] [diff] [review]
1602901_pill-left-click-edit.diff

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

I think this might be good, but I'm worried that making pills editable on a single click might cause some disruption on unintentional clicks.
Edge case:
- Write multiple addresses to make the header area scrollable.
- Select the first pill.
- Click again, which turns the pill editable.
- Hit Esc because maybe the click was accidental when the user was reading the selected pill.
- The focus moves to the input field, which makes the area scroll, and pushes the pill the user was interacting with outside the frame.

Would it be possible to enable editing only on a secondary and prolonged click?
I think that would be more on par with the behavior used in file explorer apps.

::: mail/base/content/mailWidgets.js
@@ +2608,5 @@
> +        if (
> +          event.button == 0 &&
> +          !event.ctrlKey &&
> +          !event.metaKey &&
> +          !event.shiftKey &&

Since we're repeating this modifiers conditions twice, I think we should remove them from both this condition and the one above and add them once as truthy to interrupt the code at once.
E.g.:
if (event.ctrlKey || event.metaKey || event.shiftKey) {
  return;
}

And maybe add a comment explaining why we're doing this.

@@ +2612,5 @@
> +          !event.shiftKey &&
> +          !pill.isEditing &&
> +          pill.hasAttribute("selected") &&
> +          this.getAllSelectedPills().length == 1
> +        ){

little lint error, missing space before opening curly bracket.

Comment 7

[Thomas D. (:thomas8)]

Attachment: 1602901_pill-left-click-edit.diff

Awesome! Alex and I have discussed this on Matrix yesterday and figured it's all good as it is.
Some peripheral improvements tbd in followup bugs.

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

Review of attachment 9201740 [details] [diff] [review] ⧉[details] [diff] [review]:
Edge case:

• Write multiple addresses to make the header area scrollable.
• Select the first pill.
• Click again, which turns the pill editable.

I think it's pretty unlikely to "accidentally" click again on an already selected pill. It's the declared goal of this RFE to make it easier to get into edit-pill mode.

• Hit Esc because maybe the click was accidental when the user was reading
  the selected pill.
• The focus moves to the input field,

As discussed on Matrix, we should change that in a separate bug: Let the pill keep focus after editing for better ux-feedback and control. (Likewise, predictable cursor position after starting to edit pill text should have its own bug - maybe we can actually use the mouse position as if it was a text selection? But for keyboard, we must place cursor at the end).

which makes the area scroll, and
pushes the pill the user was interacting with outside the frame.

This will no longer occur if we keep the focus on the pill, so even for accidental clicks on an already selected pill, the impact will be near-zero as you can just press ESC to stop edit pill mode and even keep your visual context.

Would it be possible to enable editing only on a secondary and prolonged
click?

Long left-click may be required on MAC, but it's an unknown concept on Windows.
Fortunately, both the long click and the normal click will work with this patch, so all users can benefit from their OS-specific muscle-memory. Fwiw, capturing long-click with JS would be non-trivial.

Since we're repeating this modifiers conditions twice, I think we should
remove them from both this condition and the one above and add them once as
truthy to interrupt the code at once.
E.g.:
if (event.ctrlKey || event.metaKey || event.shiftKey) {
return;
}

There's checkSelected() at the end of the function which still needs to run, so early return is not possible. We agreed there's no useful way of rewriting this without affecting readability.

little lint error, missing space before opening curly bracket.

Fixed.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9202572 [details] [diff] [review]
1602901_pill-left-click-edit.diff

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

This is good and I think it works correctly.

Since it's an enhancement that brings some changes in a sensitive area, I'd recommend not uplifting this to 78 but let it run its life on beta, and see how our users will react.

Comment 9

[Magnus Melin [:mkmelin]]

Sent to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aa98213075a2e238dbf9962d22c0bc28cdcb2c06

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/91ec97138cdd
Start editing address for click on single selected pill. r=aleca