Bug 1528840 Comment 82 Edit History

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

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

Addressing/answering review comments. Unfortunately, I'm failing to see the benefit of most proposed changes.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2540,4 @@
>    document.addEventListener("paste", onPasteOrDrop);
>    document.addEventListener("drop", onPasteOrDrop);
>  
> +  let addressingWidget = GetMsgAddressingWidgetTreeElement();

Composition has such wrappers for almost every element, so that would need a new bug to be changed. The wrappers will generally retrieve their element from the respective global variable (e.g. gMsgAddressingWidgetTreeElement), and only resort to document.getElementById() if the element is not available from the variable yet. That looks like a distinct design decision, probably intended to improve microperformance and avoid hardcoding ID's all over, which you also don't like.

@@ +3719,2 @@
>    gContentChanged = true;
> +  awSetAutoComplete(row);

Readability and expressiveness / respecting levels of abstraction, as seen in a lot of similar code where we always retrieve row numbers into a variable first before consuming it:

´´´´
let row = ...;
doSomethingWith(row);
´´´´

It's not obvious at all that the following code just retrieves the row number from the ID of the recipient type selector:

´´´´aAddressWidgetId.slice(aAddressWidgetId.lastIndexOf("#") + 1);´´´´

> "In many cases of obscure code, lower-level code is in the middle of a higher-level layer of the stack — levels of abstraction have been disrespected." [1]

[1]: https://simpleprogrammer.com/respecting-abstraction/

Getting the row number from a given recipient type selector should probably have its own dedicated function similar to awGetRowByInputElement(), and I doubt that saving row numbers in ID's is the best way of doing this (apart from wondering how this is handled when rows are deleted, which then requires changing all IDs).
Splitting out the row retrieval makes this easier to understand and change/improve in the future.

´´´´
let row = aAddressWidgetId.slice(aAddressWidgetId.lastIndexOf("#") + 1);
awSetAutoComplete(row);
´´´´

That said, the naming patterns of this existing function and the IDs are very confusing and error-prone:

- In composition, ID="addressCol1#XX" refers to the recipient type, but in mailing lists, the same ID refers to the email address. Imo it would be far better/clearer/safer to have IDs like recipientType#XX and recipientAddress#XX.
- onAddressColCommand() only applies to the recipient type column but the name suggests otherwise.
- aAddressWidgetId parameter also looks very generic but in reality this can only take aRecipientTypeMenuListId.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +970,5 @@
> + * Note: This function must only be called from composition, not mailing lists.
> + *
> + * @param {Event} event  a keyup event on composition's addressing widget
> + */
> +function awOnKeyUp(event) {

I understand you are suggesting to add this functionality into the onKeyUp attribute in the XUL file? I wouldn't do that:
- Strict separation of layout and code layers does look like an accepted and useful design principle for our code which should always apply even for a single line of code.
- You lose all the comments which are pretty important in this case to understand and correctly apply this workaround function until visibility check functionality will be inbuilt into <menulist> element.
- We are using the keyup event of the high-level addressing widget to avoid having event listeners on every single recipient type selector. This should live in the code so that if we ever want to use aw keyup for other purposes we don't get confused between inline and in-code functionality.

@@ +972,5 @@
> + * @param {Event} event  a keyup event on composition's addressing widget
> + */
> +function awOnKeyUp(event) {
> +  let eventTarget = event.target;
> +  if (eventTarget.id.startsWith("addressCol1")) {

That would fail because it retrieves the recipient address text input, but we could use "aw-menulist" aw-menulist. The ID is badly named as I explained above. What's the advantage of using class name over ID? Class names might change, which is less likely for IDs imo.

@@ +979,5 @@
> +    // User pressed a key with focus on the menulist; ensure that it's visible.
> +    // Note: Unrelated key presses (e.g. access keys for other UI elements)
> +    // typically do not fire keyup on the menulist as focus will have shifted.
> +    // Some false positives like function keys might occur and we accept that.
> +    let listbox = document.getElementById("addressingWidget");

Hardcoding is bad indeed, I should use the wrapper GetMsgAddressingWidgetTreeElement() instead ;-)
Otoh, we are using hardcoded ID's all over the place, so I'm not the only one...

Please explain how you'd want to use => syntax for the caller, but as per above, I don't think maintaining code in the layout layer is a good idea.
Review of attachment 9064335 [details] [diff] [review]:
-----------------------------------------------------------------

Addressing/answering review comments. Unfortunately, I'm failing to see the benefit of most proposed changes.

**Please use splinter REVIEW mode to read this, because I'm answering to the existing reviews and my answers can only be understood in the splinter review context.**

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2540,4 @@
>    document.addEventListener("paste", onPasteOrDrop);
>    document.addEventListener("drop", onPasteOrDrop);
>  
> +  let addressingWidget = GetMsgAddressingWidgetTreeElement();

Composition has such wrappers for almost every element, so that would need a new bug to be changed. The wrappers will generally retrieve their element from the respective global variable (e.g. gMsgAddressingWidgetTreeElement), and only resort to document.getElementById() if the element is not available from the variable yet. That looks like a distinct design decision, probably intended to improve microperformance and avoid hardcoding ID's all over, which you also don't like.

@@ +3719,2 @@
>    gContentChanged = true;
> +  awSetAutoComplete(row);

Readability and expressiveness / respecting levels of abstraction, as seen in a lot of similar code where we always retrieve row numbers into a variable first before consuming it:

´´´´
let row = ...;
doSomethingWith(row);
´´´´

It's not obvious at all that the following code just retrieves the row number from the ID of the recipient type selector:

´´´´aAddressWidgetId.slice(aAddressWidgetId.lastIndexOf("#") + 1);´´´´

> "In many cases of obscure code, lower-level code is in the middle of a higher-level layer of the stack — levels of abstraction have been disrespected." [1]

[1]: https://simpleprogrammer.com/respecting-abstraction/

Getting the row number from a given recipient type selector should probably have its own dedicated function similar to awGetRowByInputElement(), and I doubt that saving row numbers in ID's is the best way of doing this (apart from wondering how this is handled when rows are deleted, which then requires changing all IDs).
Splitting out the row retrieval makes this easier to understand and change/improve in the future.

´´´´
let row = aAddressWidgetId.slice(aAddressWidgetId.lastIndexOf("#") + 1);
awSetAutoComplete(row);
´´´´

That said, the naming patterns of this existing function and the IDs are very confusing and error-prone:

- In composition, ID="addressCol1#XX" refers to the recipient type, but in mailing lists, the same ID refers to the email address. Imo it would be far better/clearer/safer to have IDs like recipientType#XX and recipientAddress#XX.
- onAddressColCommand() only applies to the recipient type column but the name suggests otherwise.
- aAddressWidgetId parameter also looks very generic but in reality this can only take aRecipientTypeMenuListId.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +970,5 @@
> + * Note: This function must only be called from composition, not mailing lists.
> + *
> + * @param {Event} event  a keyup event on composition's addressing widget
> + */
> +function awOnKeyUp(event) {

I understand you are suggesting to add this functionality into the onKeyUp attribute in the XUL file? I wouldn't do that:
- Strict separation of layout and code layers does look like an accepted and useful design principle for our code which should always apply even for a single line of code.
- You lose all the comments which are pretty important in this case to understand and correctly apply this workaround function until visibility check functionality will be inbuilt into <menulist> element.
- We are using the keyup event of the high-level addressing widget to avoid having event listeners on every single recipient type selector. This should live in the code so that if we ever want to use aw keyup for other purposes we don't get confused between inline and in-code functionality.

@@ +972,5 @@
> + * @param {Event} event  a keyup event on composition's addressing widget
> + */
> +function awOnKeyUp(event) {
> +  let eventTarget = event.target;
> +  if (eventTarget.id.startsWith("addressCol1")) {

That would fail because it retrieves the recipient address text input, but we could use "aw-menulist" aw-menulist. The ID is badly named as I explained above. What's the advantage of using class name over ID? Class names might change, which is less likely for IDs imo.

@@ +979,5 @@
> +    // User pressed a key with focus on the menulist; ensure that it's visible.
> +    // Note: Unrelated key presses (e.g. access keys for other UI elements)
> +    // typically do not fire keyup on the menulist as focus will have shifted.
> +    // Some false positives like function keys might occur and we accept that.
> +    let listbox = document.getElementById("addressingWidget");

Hardcoding is bad indeed, I should use the wrapper GetMsgAddressingWidgetTreeElement() instead ;-)
Otoh, we are using hardcoded ID's all over the place, so I'm not the only one...

Please explain how you'd want to use => syntax for the caller, but as per above, I don't think maintaining code in the layout layer is a good idea.
Review of attachment 9064335 [details] [diff] [review]:
-----------------------------------------------------------------

Addressing/answering review comments. Unfortunately, I'm failing to see the benefit of most proposed changes.

**Please use splinter REVIEW mode to read this, because I'm answering to the existing reviews and my answers can only be understood in the splinter review context.**

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2540,4 @@
>    document.addEventListener("paste", onPasteOrDrop);
>    document.addEventListener("drop", onPasteOrDrop);
>  
> +  let addressingWidget = GetMsgAddressingWidgetTreeElement();

Composition has such wrappers for almost every element, so that would need a new bug to be changed. The wrappers will generally retrieve their element from the respective global variable (e.g. gMsgAddressingWidgetTreeElement), and only resort to document.getElementById() if the element is not available from the variable yet. That looks like a distinct design decision, probably intended to improve microperformance and avoid hardcoding ID's all over, which you also don't like.

@@ +3719,2 @@
>    gContentChanged = true;
> +  awSetAutoComplete(row);

Readability and expressiveness / respecting levels of abstraction, as seen in a lot of similar code where we always retrieve row numbers into a variable first before consuming it:

´´´´
let row = ...;
doSomethingWith(row);
´´´´

It's not obvious at all that the following code just retrieves the row number from the ID of the recipient type selector:

´´´´aAddressWidgetId.slice(aAddressWidgetId.lastIndexOf("#") + 1);´´´´

> "In many cases of obscure code, lower-level code is in the middle of a higher-level layer of the stack — levels of abstraction have been disrespected." [1]

[1]: https://simpleprogrammer.com/respecting-abstraction/

Getting the row number from a given recipient type selector should probably have its own dedicated function similar to awGetRowByInputElement(), and I doubt that saving row numbers in ID's is the best way of doing this (apart from wondering how this is handled when rows are deleted, which then requires changing all IDs).
Splitting out the row retrieval makes this easier to understand and change/improve in the future.

´´´´
let row = aAddressWidgetId.slice(aAddressWidgetId.lastIndexOf("#") + 1);
awSetAutoComplete(row);
´´´´

That said, the naming patterns of this existing function and the IDs are very confusing and error-prone:

- In composition, ID="addressCol1#XX" refers to the recipient type, but in mailing lists, the same ID refers to the email address. Imo it would be far better/clearer/safer to have IDs like recipientType#XX and recipientAddress#XX.
- onAddressColCommand() only applies to the recipient type column but the name suggests otherwise.
- aAddressWidgetId parameter also looks very generic but in reality this can only take aRecipientTypeMenuListId.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +970,5 @@
> + * Note: This function must only be called from composition, not mailing lists.
> + *
> + * @param {Event} event  a keyup event on composition's addressing widget
> + */
> +function awOnKeyUp(event) {

I understand you are suggesting to add this functionality into the onKeyUp attribute in the XUL file? I wouldn't do that:
- Strict separation of layout and code layers does look like an accepted and useful design principle for our code which should always apply even for a single line of code.
- You lose all the comments which are pretty important in this case to understand and correctly apply this workaround function until visibility check functionality will be inbuilt into <menulist> element.
- We are using the keyup event of the high-level addressing widget to avoid having event listeners on every single recipient type selector. This should live in the code so that if we ever want to use aw keyup for other purposes we don't get confused between inline and in-code functionality.

@@ +972,5 @@
> + * @param {Event} event  a keyup event on composition's addressing widget
> + */
> +function awOnKeyUp(event) {
> +  let eventTarget = event.target;
> +  if (eventTarget.id.startsWith("addressCol1")) {

That would fail because it retrieves the recipient address text input, but we could use "aw-menulist" class. The ID is badly named as I explained above. What's the advantage of using class name over ID? Class names might change, which is less likely for IDs imo.

@@ +979,5 @@
> +    // User pressed a key with focus on the menulist; ensure that it's visible.
> +    // Note: Unrelated key presses (e.g. access keys for other UI elements)
> +    // typically do not fire keyup on the menulist as focus will have shifted.
> +    // Some false positives like function keys might occur and we accept that.
> +    let listbox = document.getElementById("addressingWidget");

Hardcoding is bad indeed, I should use the wrapper GetMsgAddressingWidgetTreeElement() instead ;-)
Otoh, we are using hardcoded ID's all over the place, so I'm not the only one...

Please explain how you'd want to use => syntax for the caller, but as per above, I don't think maintaining code in the layout layer is a good idea.

Back to Bug 1528840 Comment 82