Bugzilla

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.

> - 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 pills 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.

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.

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

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.