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