Bug 1700278 Comment 6 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

2nd iteration, more cleanup!

- Remove focused `pill` argument from removeSelectedPills(), as we can always retrieve the focused pill from inside this function. Typically as long as any pills are selected, the focus will also be on a pill (which may or may not be selected). Even when using context or main menus, the pill focus is not lost. 
- If there's no selected pill, there's nothing to remove, so let's bail out.
- I'm now using the first selected pill as a fallback for the unlikely event that there's no focused pill.
- Declutter callers: removeSelectedPills(), moveSelectedPills().
- Shrinked dropAddressPill() to a one-liner with a single argument, the label where the drop occurs. This function was calling moveSelectedPills(mailRecipientsArea.getAllSelectedPills()[0],...) - not ideal, to iterate an unlimited number of pills just to get the first one... In fact, 5 out of 6 lines in that function were just clutter, as well as 2/3 of its arguments. I'm still not happy with the general design around dropAddressPill(), but at least it's now de-cluttered.

Complete revised summary of this patch:

- Simplify pills' contextual cut/copy/delete/move-to-* call chain by eliminating
  no-op focus pill argument, which will make them context-independent.
- Make pills' cut/copy/delete actions internal functions of <mail-recipients-area>.
- Hook up pills' actions in the layout layer to generic onCommand handlers in
  MsgComposeCommands.js.
- Retrieve the focused pill via querySelector at the only place where we need it,
  removeSelectedPills(), and handle lost focus gracefully.
- Cleanup dropAddressPills() and remove unneeded arguments; more to be done.
2nd iteration, more cleanup!

- Remove focused `pill` argument from removeSelectedPills(), as we can always retrieve the focused pill from inside this function. Typically as long as any pills are selected, the focus will also be on a pill (which may or may not be selected). Even when using context or main menus, the pill focus is not lost. 
- If there's no selected pill, there's nothing to remove, so let's bail out.
- I'm now using the first selected pill as a fallback for the unlikely event that there's no focused pill.
- Declutter callers: `removeSelectedPills()`, `moveSelectedPills()`.
- Shrinked `dropAddressPill()` to a one-liner with a single argument, the label where the drop occurs. This function was calling `moveSelectedPills(mailRecipientsArea.getAllSelectedPills()[0],...)` - not ideal, to iterate an unlimited number of pills just to get the first one... In fact, 5 out of 6 lines in that function were just clutter, as well as 2/3 of its arguments. I'm still not happy with the general design around `dropAddressPill()`, but at least it's now de-cluttered.

Complete revised summary of this patch:

- Simplify pills' contextual `cut/copy/delete/move-to-*` call chain by eliminating
  no-op focus pill argument, which will make them context-independent.
- Make pills' cut/copy/delete actions internal functions of `<mail-recipients-area>`.
- Hook up pills' actions in the layout layer to generic onCommand handlers in
  MsgComposeCommands.js.
- Retrieve the focused pill via querySelector at the only place where we need it,
  `removeSelectedPills()`, and handle lost focus gracefully.
- Cleanup `dropAddressPills()` and remove unneeded arguments; more to be done.
2nd iteration, more cleanup!

- Remove focused `pill` argument from `removeSelectedPills()`, as we can always retrieve the focused pill from inside this function. Typically as long as any pills are selected, the focus will also be on a pill (which may or may not be selected). Even when using context or main menus, the pill focus is not lost. 
- If there's no selected pill, there's nothing to remove, so let's bail out.
- I'm now using the first selected pill as a fallback for the unlikely event that there's no focused pill.
- Declutter callers: `removeSelectedPills()`, `moveSelectedPills()`.
- Shrinked `dropAddressPill()` to a one-liner with a single argument, the label where the drop occurs. This function was calling `moveSelectedPills(mailRecipientsArea.getAllSelectedPills()[0],...)` - not ideal, to iterate an unlimited number of pills just to get the first one... In fact, 5 out of 6 lines in that function were just clutter, as well as 2/3 of its arguments. I'm still not happy with the general design around `dropAddressPill()`, but at least it's now de-cluttered.

Complete revised summary of this patch:

- Simplify pills' contextual `cut/copy/delete/move-to-*` call chain by eliminating
  no-op focus pill argument, which will make them context-independent.
- Make pills' cut/copy/delete actions internal functions of `<mail-recipients-area>`.
- Hook up pills' actions in the layout layer to generic onCommand handlers in
  MsgComposeCommands.js.
- Retrieve the focused pill via querySelector at the only place where we need it,
  `removeSelectedPills()`, and handle lost focus gracefully.
- Cleanup `dropAddressPills()` and remove unneeded arguments; more to be done.

Back to Bug 1700278 Comment 6