1528840 - addressingWidgetOverlay.js, abMailListDialog.js code cleanup

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, task)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1527547 +++

STR

1. Compose message with three recipients: R1, R2, R3. Ensure this scenario before going into any of the alternative steps below.
2. Place cursor on R1 and tab forward to R2
3. Check Error Console

Actual result

TypeError: listBox.listBoxObject is undefined[Learn More] addressingWidgetOverlay.js:707:5

Expected result

No Error.

Code looks strange - why only on forward-tab?

From my tests on windows 10, this code (ensureIndexIsVisible...) is not even doing anything - even without it every row is correctly scrolled into view when tabbing into it, both directions. So I suggest to just rip it out.

While we are here, there seems to be a lot of unused code around here. I ripped it out and could not see any difference. Aceman would have to check for addons if they are using some of the removed functions.

I would not expect this to be any different on other platforms, but someone should check to make sure.

Comment 1

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

I went on a dead-code spree and found plenty. I tried without these code chunks and couldn't find any difference in behaviour on Windows 10.

• Paenglab/Aceman, please test on other OS.
• Jörg/Paenglab/Aceman, please review this carefully as I might be wrong.
• Aceman, please check if any of the removed functions are used by important addons.

Comment 2

[:aceman]

Comment on attachment 9044731 [details] [diff] [review]
Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

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

I am not familiar with that the code here does and why it is there (maybe some obscure cases). Anyway, a safe fix to "TypeError: listBox.listBoxObject is undefined[Learn More] addressingWidgetOverlay.js:707:5" should be removing the '.listBoxObject' part. There were some changes to the richlistbox element and ensureIndexIsVisible is now directly on the listbox element.
See https://searchfox.org/comm-central/search?q=ensureIndexIsVisible&redirect=false

So try 'listBox.ensureIndexIsVisible(listBoxRow + 1);' etc.

Comment 3

[Thomas D. (:thomas8)]

Comment on attachment 9044731 [details] [diff] [review]
Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

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

(In reply to :aceman from comment #2)
> Comment on attachment 9044731 [details] [diff] [review]
> Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers
> 
> Review of attachment 9044731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not familiar with that the code here does and why it is there (maybe
> some obscure cases).

Well, I haven't been familiar with this code either until I started working on it, but I noticed that in spite of the error which stops explicit ensureIndexIsVisible from running, everything was correctly made visible when tabbing into hidden rows. We can know what this code does from reading it, isn't it? Then it turns out there's lot of code whose only purpose is to end up in this hack which doesn't do anything, at least for me on Windows 10 (other OS, maybe other Win flavors should be checked by reviewers who have access to that). After such checks, I don't think there can be any other obscure cases. I'll go through each removed chunk of code and explain why I think it's dispensable. 

> Anyway, a safe fix to "TypeError: listBox.listBoxObject
> is undefined[Learn More] addressingWidgetOverlay.js:707:5" should be
> removing the '.listBoxObject' part. There were some changes to the
> richlistbox element and ensureIndexIsVisible is now directly on the listbox
> element.

Yes, but I don't think we should keep useless code which doesn't do anything.

> See
> https://searchfox.org/comm-central/
> search?q=ensureIndexIsVisible&redirect=false
> 
> So try 'listBox.ensureIndexIsVisible(listBoxRow + 1);' etc.

Please try it without this function and see if you're missing anything when it comes to hidden rows becoming visible again when tabbed into. Reducing code complexity is an honorable goal, I believe...

::: mail/components/compose/content/MsgComposeCommands.js
@@ -2567,5 @@
>    dictionaryRemovalObserver.addObserver();
>  
>    var identityList = document.getElementById("msgIdentity");
>  
> -  document.addEventListener("keypress", awDocumentKeyPress, true);

awDocumentKeyPress only checks for 2 keys, and the same keys are already checked on another level (the text input), so the event can never be reached here.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -527,5 @@
> -  if (lastInput && lastInput.value && !top.doNotCreateANewRow)
> -    awAppendNewRow(false);
> -  top.doNotCreateANewRow = false;
> -}
> -

https://dxr.mozilla.org/comm-central/search?q=awInputChanged&=comm-central

This function is unused - why should we keep it then? Maybe to check if addons are using it - unlikely.

@@ -699,5 @@
>  
>  function awTabFromRecipient(element, event) {
> -  // If we are the last element in the listbox, we don't want to create a new row.
> -  if (element == awGetInputElement(top.MAX_RECIPIENTS))
> -    top.doNotCreateANewRow = true;

It's dangerous/error-prone to set a one-time variable which never get's reset because the function which resets it is no longer called anywhere (see above).

@@ -706,5 @@
> -  if (!event.shiftKey && row < top.MAX_RECIPIENTS) {
> -    var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based.
> -    var listBox = document.getElementById("addressingWidget");
> -    listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1);
> -  }

Yes, we could fix the error, but what's the point of keeping this code when it doesn't change the behaviour at all? The net result of this code is ensureIndexIsVisible - with or without this code it works just fine, when you tab in or out currently invisible rows, they are scrolled into view. I guess someone was tired of doing this manually for every list, so they just coded it into the list behaviour that whatever has focus must be visible - invisible focus is useless and self-contradictory. When tabbing through 50 recipients, such useless code may well have performance impact, too. We only need to make sure that it works on all platforms, maybe on all versions of platforms which we support. It's hard to imagine though that the widget would auto-scroll on Windows 10, but not auto-scroll on Windows 8 or other OS.

@@ -708,5 @@
> -    var listBox = document.getElementById("addressingWidget");
> -    listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1);
> -  }
> -
> -  // be sure to add the user add recipient to our ignore list

I corrected the grammar of this comment to make it readable.

@@ -721,5 @@
> -    var listBox = document.getElementById("addressingWidget");
> -    listBox.listBoxObject.ensureIndexIsVisible(listBoxRow - 1);
> -  }
> -}
> -

This covers tabbing backwards, same story as above - not changing the behaviour on Win 10 in any way, things auto-scroll into view when tabbed into automatically, even without this code. So why keep it?

@@ -831,5 @@
> -        awDeleteHit(inputs[0]);
> -    }
> -    break;
> -  }
> -}

From my understanding and tests, this never gets called for the only two keys which it is checking, because we catch those keys on the recipient input already, and then explicitly stop them from getting here via event.stopPropagation();, and there's even a comment in code saying that we don't want to get here. Code which never gets called should go away. I'd think even addons can't use this if it's already bypassed in code, so they would have to overlay the other part - well then, why not use the other part to do what you want?

@@ -836,5 @@
> -
> -function awMenulistKeyPress(event, element) {
> -  switch (event.keyCode) {
> -  case KeyEvent.DOM_VK_TAB:
> -    awTabFromMenulist(element, event);

This is code which only calls the other code (ensureIndexVisible...) which doesn't change anything. Let it go then!

@@ -947,5 @@
> -function awDocumentKeyPress(event) {
> -  try {
> -    var id = event.target.id;
> -    if (id.startsWith("addressCol1"))
> -      awMenulistKeyPress(event, event.target);

It's very strange why this would need so many nested calls to work correctly, but ultimately it doesn't matter because the only thing which it really does is ... drumroll ... ensureIndexIsVisible - which, per my observation on Win10, does not do anything which the widget is not already doing automatically somehow. Keeping complex nested code for nothing does not sound right to me, unless if there are other versions of Windows or other OS which need this.

::: mail/components/compose/content/messengercompose.xul
@@ -1929,5 @@
>                <menupopup id="msgIdentityPopup"/>
>              </menulist>
>            </hbox>
>            <richlistbox id="addressingWidget" flex="1" seltype="multiple"
> -                       onkeydown="awKeyDown(event, this)"

The target function only checks for DEL/Backspace, but it'll never actually do anything as we capture those keys elsewhere, see above.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 9044731 [details] [diff] [review]
Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

Before we embark on new changes, would it be possible to address the accessibility regressions the last lot caused? I'm referring to bug 1519346 and bug 1519328.

Comment 5

[Richard Marti (:Paenglab)]

Comment on attachment 9044731 [details] [diff] [review]
Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

I saw no issue on Mac.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #4)

Comment on attachment 9044731 [details] [diff] [review]
Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

Before we embark on new changes, would it be possible to address the
accessibility regressions the last lot caused? I'm referring to bug 1519346
and bug 1519328.

Hmmm. I've already commented on both bugs stating that

• I don't see how my feature/UX work on attachment pane could have adversely affected accessibility, so whatever you refer to by "the last lot" probably isn't mine. E.g., if "cursoring-to-select" was still working after my changes, why should the ARIA screenreader announcements of selected items break? So even if it did break after one of my patches, that doesn't necessarily make it my fault.
• Bug 1519346 and friends are probably caused by de-XUL and de-XBL (trunk doesn't even cursor down in the list any more; definitely "wasn't me")
• Bug 1519328 (attachment pane gets focus after adding attachments) is by design and should be wontfixed
• Unfortunately I'm not able to assist with fixing accessibility bugs like ARIA-related etc.

That said, I think those access issues in an unrelated UI part shouldn't stop us from a simple little cleanup exercise here, which is easy to test and very limited in scope, even easy to revert if we notice anything odd.

Comment 7

[Thomas D. (:thomas8)]

Comment on attachment 9044731 [details] [diff] [review]
Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

(In reply to Richard Marti (:Paenglab) from comment #5)
> Comment on attachment 9044731 [details] [diff] [review]
> Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers
> I saw no issue on Mac.

Richard has cancelled ui-r because there's no change in UI/UX, but he has tested my patch on MAC and Linux and did not find any problem with it, i.e. temporarily not visible recipient items are still correctly scrolled into view when tabbed into (forwards/backwards). Please set f+ instead.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 9044731 [details] [diff] [review]
Patch V.1: Remove unused code from addressingWidgetOverlay.js and callers

f+ for not seeing a difference to before the patch.

Comment 9

[Thomas D. (:thomas8)]

Attachment: Patch Part 1, V.1: replace awSetFocus(row, element) with awSetFocusTo(element)

Ok, sorry we're still reshuffling to do things in the right order.

This patch replaces all instances of awSetFocus(row, element) with the new awSetFocusTo(element). As confirmed by Richard, Aceman and myself in tests, the ensureElementIsVisible() did nothing, so it's now much simpler and cleaner.

Aceman, I think you were tending to use the element instead of the row anyway, and I finally figured we're better off to use the element as an argument for awSetFocusTo(). It's more generic (now works to focus any element), it's safer (rows can change before we focus), the element is mostly available anyway, and it's more natural because that's what we normally focus. We're only using the helper function to do the focusing asynchronously.

I preserved the legacy function awSetFocus(row, element) as we don't know if Addons are still using it, but it's now just a one-liner to call awSetFocusTo(element).

I also cleaned up the reciprocal focus stuff in abMailListDialog.js.

Comment 10

[Thomas D. (:thomas8)]

Attachment: Patch Part 1, V.1.1: (nitfixes) replace awSetFocus(row, element) with awSetFocusTo(element)

Now with some nits fixed, otherwise same as before. See comment 9.

Comment 11

[Thomas D. (:thomas8)]

Attachment: Patch Part 2, V.2: Cleanup unused/no-op code in addressingWidgetOverlay.js, abMailListDialog.js and callers

I think we should do all the cleanup stuff here before doing bug 1527547 (but we can reverse the order if we get stuck here).

Again, I've added the reciprocal cleanup in abMailListDialog.js.

Comment 3 explains the details.

Comment 12

[Ian Neal]

Comment on attachment 9047020 [details] [diff] [review]
Patch Part 1, V.1.1: (nitfixes) replace awSetFocus(row, element) with awSetFocusTo(element)

Just as a note, if you are changing under mailnews/ as well as under mail/ need to check there are no changes needed under suite/mailnews

Comment 13

[Thomas D. (:thomas8)]

Not changed by my patch, but we DO need ensureElementIsVisible() when focus is on recipient type selector, user hides focused element by scrolling the list, then presses keys on the focused but hidden selector - it should come back into view.

Comment 14

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere) from comment #13)

Not changed by my patch, but we DO need ensureElementIsVisible() when focus is on recipient type selector, user hides focused element by scrolling the list, then presses keys on the focused but hidden selector - it should come back into view.

Which should probably look similar to this one which we are removing here (probably better performance when it's done on the document, not to hinder autocomplete):

function awDocumentKeyPress(event) {	
  try {	
    var id = event.target.id;	
    if (id.startsWith("addressCol1"))	
      awMenulistKeyPress(event, event.target);	
  } catch (e) { }	
}

Comment 15

[Thomas D. (:thomas8)]

(In reply to Ian Neal from comment #12)

Just as a note, if you are changing under mailnews/ as well as under mail/
need to check there are no changes needed under suite/mailnews

Noted.

Comment 16

[Thomas D. (:thomas8)]

Attachment: Patch #1.1 V.2 (TB only): replace awSetFocus(row, element) with awSetFocusTo(element)

I removed abMailListDialog.js so that we don't depend on the Suite-relevant changes.

Following Ace's review comment on the other bug, I also restored top.awInputToFocus so that we play safe for the asynchronous focusing.

The unaltered TB copy of abMailListDialog.js should still work because I've also kept the legacy function awSetFocus().

Comment 17

[Thomas D. (:thomas8)]

Attachment: Patch #1.2 V.2 (Suite): replace awSetFocus(row, element) with awSetFocusTo(element)

Here's the Suite equivalent (untested) - abMailListDialog.js is shared between TB and suite.

Comment 18

[:aceman]

Comment on attachment 9047141 [details] [diff] [review]
Patch #1.1 V.2 (TB only): replace awSetFocus(row, element) with awSetFocusTo(element)

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

Thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4041,2 @@
>    if (element.value == "") {
> +    // Focus last row of addressing widget

Dots after end of sentences.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -515,4 @@
>    else
> -    awSetFocus(1, awGetInputElement(2));  /* We have to cheat a little bit because the focus will */
> -                                          /* be set asynchronusly after we delete the current row, */
> -                                          /* therefore the row number still the same! */

If you remove it here, don't forget to restore the comment (reformatted) in bug 1527547.

@@ +683,5 @@
>  }
>  
> +/**
> + * Set focus to the specified element, typically a recipient input element.
> + * We do this asynchronusly to allow other processes like adding or removing rows

asynchronously

Comment 19

[:aceman]

Comment on attachment 9047142 [details] [diff] [review]
Patch #1.2 V.2 (Suite): replace awSetFocus(row, element) with awSetFocusTo(element)

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

OK for the abMailListDialog.js part.

But when tabbing in the maillist dialog, I still get this error occasionally:
ReferenceError: reference to undefined property "listBoxObject" addressingWidgetOverlay.js:734:5

This is inside the function awTabFromMenulist() which I'm not sure why we call inside this dialog as there is no menulist. Can you find out why that is?

::: mailnews/addrbook/content/abMailListDialog.js
@@ -407,5 @@
> -    listbox.ensureElementIsVisible(theNewRow);
> -    top.awInputElement.focus();
> -  } catch (ex) {
> -    top.awFocusRetry++;
> -    if (top.awFocusRetry < 8) {

Interesting how hard this tried to set the focus... Hopefully that's not needed anymore.

Comment 20

[:aceman]

Comment on attachment 9047024 [details] [diff] [review]
Patch Part 2, V.2: Cleanup unused/no-op code in addressingWidgetOverlay.js, abMailListDialog.js and callers

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -824,5 @@
> -    break;
> -  }
> -}
> -
> -function awKeyDown(event, listboxElement) {

You are also touching this same function in bug 1527547.

@@ -949,5 @@
>  function awSizerMouseUp() {
>    document.removeEventListener("mousemove", awSizerMouseMove, true);
>  }
>  
> -function awDocumentKeyPress(event) {

Seems you forgot to remove the calls of this.

Comment 21

[Thomas D. (:thomas8)]

Attachment: [Landed in comment 33] Patch #1.1 V.2.1 (TB only): replace awSetFocus(row, element) with awSetFocusTo(element)

r+ aceman from comment 18, now ready to land.

Nits fixed.

Comment 22

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #21)

Created attachment 9047329 [details] [diff] [review]
Patch #1.1 V.2.1 (TB only): replace awSetFocus(row, element) with awSetFocusTo(element)
r+ aceman from comment 18, now ready to land.

Jörg, can you please land this part so that we stabilize the code to proceed with other parts here and bug 1527547. Tia.

Comment 23

[Thomas D. (:thomas8)]

Comment on attachment 9047024 [details] [diff] [review]
Patch Part 2, V.2: Cleanup unused/no-op code in addressingWidgetOverlay.js, abMailListDialog.js and callers

Removing r? for now; needs updating, nitfixes, and looking into the suite parts.
Preliminary review by Aceman in Comment 20.

Comment 24

[Ian Neal]

Comment on attachment 9047142 [details] [diff] [review]
Patch #1.2 V.2 (Suite): replace awSetFocus(row, element) with awSetFocusTo(element)

>+++ b/suite/mailnews/components/compose/content/addressingWidgetOverlay.js

>   {
>     nextInput.select();
>-    awSetFocus(row+1, nextInput);
>+    awSetFocus(nextInput);
Nit: shouldn't this be awSetFocusTo(nextInput);
>   }

>-    // focus on new input widget
>+    // Focus the new input widget
Nit: missing full stop at end of the comment

r=me with those fixed

Comment 25

[Thomas D. (:thomas8)]

Attachment: [Landed in comment 33] Patch #1.2 V.2.1 (Suite/TB): replace awSetFocus(row, element) with awSetFocusTo(element)

(In reply to Ian Neal from comment #24)

Comment on attachment 9047142 [details] [diff] [review]
...
r=me with those fixed

Thanks for rapid review!
Nits fixed.

r=aceman (comment 19)
r=IanN (comment 24)

Another part ready to land.

---

(In reply to :aceman from comment #19)

Review of attachment 9047142 [details] [diff] [review]:
OK for the abMailListDialog.js part.

But when tabbing in the maillist dialog, I still get this error occasionally:
ReferenceError: reference to undefined property "listBoxObject"
addressingWidgetOverlay.js:734:5

This is inside the function awTabFromMenulist() which I'm not sure why we
call inside this dialog as there is no menulist. Can you find out why that
is?

Removing unused/no-op code is planned for the next patches in the series here. This patch was about replacing the focus function only.

Comment 26

[:aceman]

(In reply to Thomas D. from comment #25)

Removing unused/no-op code is planned for the next patches in the series here. This patch was about replacing the focus function only.

Yes, I understand that, but just removing the function may not fix the underlying problem that some caller thinks there is a menulist (probably the recipient type picker) in this dialog. I'd like to find this caller before you remove the function ad hide the problem. Of course you can do the searching in the other bug.

Comment 27

[Jorg K (CEST = GMT+2)]

We're pretty busted right now, so I'll land that when I can.

Comment 28

[Jorg K (CEST = GMT+2)]

Please to a (limited) try run before requestion check-in again.

Comment 29

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #28)

Please to a (limited) try run before requestion check-in again.

Aceman, can you please do the try run for the first two patches here so that we can protect that against bitrot.

Comment 30

[:aceman]

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

Comment 31

[:aceman]

Looks successful :)

Comment 32

[Jorg K (CEST = GMT+2)]

There's more to come, right?

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b4b9a40aa811
Part 1.1, TB: Replace awSetFocus(row, element) with awSetFocusTo(element); addressingWidgetOverlay.js code cleanup. r=aceman
https://hg.mozilla.org/comm-central/rev/03043d153f89
Part 1.2, TB/SM: Replace awSetFocus(row, element) with awSetFocusTo(element); addressingWidgetOverlay.js, abMailListDialog.js code cleanup. r=aceman,IanN

Comment 34

[Thomas D. (:thomas8)]

Attachment: 1528840_2.1_TB_aw-unused-code-cleanup.patch (V.3) - WIP, updated, untested

Updated WIP patch (TB-only) to see what we are trying to do. Need to check for orphaned callers, especially in the TB/SM shared file abMailListDialog.js.

Comment 35

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #19)

Review of attachment 9047142 [details] [diff] [review]:

OK for the abMailListDialog.js part.

But when tabbing in the maillist dialog, I still get this error occasionally:
ReferenceError: reference to undefined property "listBoxObject"
addressingWidgetOverlay.js:734:5

This is inside the function awTabFromMenulist() which I'm not sure why we
call inside this dialog as there is no menulist. Can you find out why that
is?

Grrrr. I found it. Spaghetti code with SeaMonkey flavor. As Aceman hinted, something wrong here!

https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress

TB has mail/.../MsgComposeCommands.js where we listens for keypress on the address widget of composition:

document.addEventListener("keypress", awDocumentKeyPress, true);

TB/SM share mailnews/.../abMailListDialog.js and listen for keypress on the address widget of mailing list:

document.addEventListener("keypress", awDocumentKeyPress, true);

Unfortunately, TB and SM do very different things with that listener in their product-specific addressingWidgetOverlay.js, then it gets messy.

1. SM stack:
   awDocumentKeyPress > if (addressCol1) > awRecipientKeyPress:

• handle cursor/up down (which we have just added to TB in the other bug)
• handle recipient deletion
• So iiuc, SM uses this correctly, but only for mailing list dialog, not used for composition.

2. TB stack:
   awDocumentKeyPress > if (addressCol1) > awMenulistKeyPress > if (TAB) > awTabFromMenulist > if (Shift+TAB):

• listBox.listBoxObject.ensureIndexIsVisible (causes minor error because of listBoxObject)
• For TB composition, this is correct stack, but not doing anything. That's why I want to remove it.
• For TB mailing list dialogs, this is incorrect stack: We go into awMenuListKeyPress but the mailing list does not have recipient type selector menulist, as Aceman said. TB does not need this for mailing lists, either.

So we have the listener/caller in shared code, still used by SM, but TB does not need that listener/caller at all.
I think short of reinventing the wheel, an easy way out could be this:
At compile time, in abMailListDialog.js, only add the listener/caller for SM (using #ifdef MOZ_SUITE).

Aceman, can you comment?

Comment 36

[Thomas D. (:thomas8)]

ni?aceman for comment 35 ^^

Comment 37

[Thomas D. (:thomas8)]

Comment on attachment 9049117 [details] [diff] [review]
1528840_2.1_TB_aw-unused-code-cleanup.patch (V.3) - WIP, updated, untested

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

Aceman, here's annotations to assist your review of this patch.
Patch has one flaw pertaining to the spaghetti code with Seamonkey mix-ins, see comment 35.

::: mail/components/compose/content/MsgComposeCommands.js
@@ -2567,5 @@
>    dictionaryRemovalObserver.addObserver();
>  
>    var identityList = document.getElementById("msgIdentity");
>  
> -  document.addEventListener("keypress", awDocumentKeyPress, true);

This ultimately ends up in ensureIndexIsVisible(), which doesn't do anything, so it can go away. This is TB only for composition.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -547,5 @@
> -  var lastInput = awGetInputElement(top.MAX_RECIPIENTS);
> -  if (lastInput && lastInput.value && !top.doNotCreateANewRow)
> -    awAppendNewRow(false);
> -  top.doNotCreateANewRow = false;
> -}

https://dxr.mozilla.org/comm-central/search?q=awInputChanged

no callers: awInputChanged() is not used at all in TB, neither in SM.

@@ -732,5 @@
>  
>  function awTabFromRecipient(element, event) {
> -  // If we are the last element in the listbox, we don't want to create a new row.
> -  if (element == awGetInputElement(top.MAX_RECIPIENTS))
> -    top.doNotCreateANewRow = true;

https://dxr.mozilla.org/comm-central/search?q=doNotCreateANewRow

top.doNotCreateANewRow is only reset to false in awInputChanged() which is never called, so that's not functional and can go away. Currently we never create new rows with tab, which seems ok.

@@ -746,2 @@
>    // when the user tabs out of an autocomplete line...
>    addRecipientsToIgnoreList(element.value);

ToDo: In another bug, this should probably be tied to some other event, e.g. blur. We have no way of telling when the recipient address is ready or not; just clicking elsewhere is as good to exit recipient input as any other method.

OT: I also wonder about the data/privacy of this: where does the spellcheck ignore list live? Is it temporary or permanent? Is it accessible to the user? Which part(s) of the recipient input are we adding there? Emails, too? Is this known to the user?

@@ -751,5 @@
> -  var row = awGetRowByInputElement(element);
> -  if (event.shiftKey && row > 1) {
> -    var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based.
> -    var listBox = document.getElementById("addressingWidget");
> -    listBox.listBoxObject.ensureIndexIsVisible(listBoxRow - 1);

https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist

awTabFromMenulist() is only used to ensure visibility of previous row for Shift+Tab from recipient type selector - no longer needed.
only caller: in awMenulistKeyPress() below - dispensible, too.

@@ -898,5 @@
>        break;
>    }
>  }
>  
> -function awMenulistKeyPress(event, element) {

https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist

This function currently only checks for tab key, then calls awTabFromMenulist(), which does nothing useful (see above), so let's eliminate. Btw, keypress event and keyCode are also deprecated.

The only TB caller is function awDocumentKeyPress(), which does nothing else, so also dispensable in principle (see below; caveats: see comment 35).

ToDo: Here or in another bug, we may want to capture other key presses on the menu list (using keydown), namely those that do NOT shift focus to the recipient input, and ensure that the input becomes visible again in case the user scrolled it away whilst it had focus. Edge case but relevant because currently you can change the recipient type without seeing it, which is error-prone.

@@ -1008,5 @@
>  function awSizerMouseUp() {
>    document.removeEventListener("mousemove", awSizerMouseMove, true);
>  }
>  
> -function awDocumentKeyPress(event) {

https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress

In TB, this function only calls awMenuListKeyPress(), which is useless (see above), so this could go too.

However, the function is also called from mailing lists which is shared between TB and SM. In TB's mailing lists, it shouldn't even be called, that's a bug (no recipient type selector menulist in mailing lists). In SeaMonkey's mailing lists, it calls awRecipientKeyPress(), which handles cursor down/up and deletes for mailing lists.

ToDo: Sooo, we can't remove awDocumentKeyPress() for TB without also removing TB's callers (which makes this patch incomplete); we can't just remove the mailing list caller as it's currently shared with SM; and we can't remove the SM caller as it's still in use afasics. (Although I wonder how SM avoids the composition address widget functions from applying in the mailing list, because in TB it seems shared functionality.) What we could do is to only keep the callers for SM at compile time, using #ifdef MOZ_SUITE.

See comment 35 for details.

Comment 38

[Thomas D. (:thomas8)]

Attachment: 1528840_2.1_TB_aw-unused-code-cleanup.patch (V.4): fix callers in abMailListDialog.js

This patch addresses the problem of comment 35.

Comment 39

[Thomas D. (:thomas8)]

Comment on attachment 9051492 [details] [diff] [review]
1528840_2.1_TB_aw-unused-code-cleanup.patch (V.4): fix callers in abMailListDialog.js

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

Aceman, this should now be simple to review.
Now with shorter annotations. For details, see comment 35 ff.

::: mail/components/compose/content/MsgComposeCommands.js
@@ -2567,5 @@
>    dictionaryRemovalObserver.addObserver();
>  
>    var identityList = document.getElementById("msgIdentity");
>  
> -  document.addEventListener("keypress", awDocumentKeyPress, true);

https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress

TB call stack (before this patch):

(1) awDocumentKeyPress: if (addressCol1) >
(2) awMenulistKeyPress: if (TAB) >
(3) awTabFromMenulist:  if (Shift+TAB):
      listBox.listBoxObject.ensureIndexIsVisible 

Which does nothing. So the entire call stack can be removed for TB.

(1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress)
(2) [awMenulistKeyPress](https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress)
(3) [awTabFromMenulist](https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -547,5 @@
> -  var lastInput = awGetInputElement(top.MAX_RECIPIENTS);
> -  if (lastInput && lastInput.value && !top.doNotCreateANewRow)
> -    awAppendNewRow(false);
> -  top.doNotCreateANewRow = false;
> -}

https://dxr.mozilla.org/comm-central/search?q=awInputChanged&=comm-central

No callers - no use.

@@ -732,5 @@
>  
>  function awTabFromRecipient(element, event) {
> -  // If we are the last element in the listbox, we don't want to create a new row.
> -  if (element == awGetInputElement(top.MAX_RECIPIENTS))
> -    top.doNotCreateANewRow = true;

https://dxr.mozilla.org/comm-central/search?q=awTabFromRecipient

no-op: top.doNotCreateANewRow is only set to false in awInputChanged(), which is never called. With or without this, we never create a new row when pressing TAB on recipient input, which seems ok.

@@ -738,5 @@
> -  var row = awGetRowByInputElement(element);
> -  if (!event.shiftKey && row < top.MAX_RECIPIENTS) {
> -    var listBoxRow = row - 1; // listbox row indices are 0-based, ours are 1-based.
> -    var listBox = document.getElementById("addressingWidget");
> -    listBox.listBoxObject.ensureIndexIsVisible(listBoxRow + 1);

no-op: this was used for ensureIndexIsVisible upon Shift+TAB, which is no longer needed, as changing focus auto-ensures visibility.

@@ -746,2 @@
>    // when the user tabs out of an autocomplete line...
>    addRecipientsToIgnoreList(element.value);

So this is the lonely only line which remains, which could probably be realized in a more generic way, maybe onblur.

@@ -746,5 @@
>    // when the user tabs out of an autocomplete line...
>    addRecipientsToIgnoreList(element.value);
>  }
>  
> -function awTabFromMenulist(element, event) {

https://dxr.mozilla.org/comm-central/search?q=awTabFromMenulist

no-op: This is number (3) of the dispensable TB callstack (see above, MsgComposeCommands.js). ensureIndexIsVisible has no effect, already inbuilt with focus.

> TB call stack (before this patch):
>  
> (1) [awDocumentKeyPress](https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress): if (addressCol1) >
> (2) awMenulistKeyPress: if (TAB) >
> (3) awTabFromMenulist:  if (Shift+TAB):
>      listBox.listBoxObject.ensureIndexIsVisible

@@ -898,5 @@
>        break;
>    }
>  }
>  
> -function awMenulistKeyPress(event, element) {

https://dxr.mozilla.org/comm-central/search?q=awMenuListKeyPress

no-op: Number (2) of the dispensable TB stack.

@@ -1008,5 @@
>  function awSizerMouseUp() {
>    document.removeEventListener("mousemove", awSizerMouseMove, true);
>  }
>  
> -function awDocumentKeyPress(event) {

https://dxr.mozilla.org/comm-central/search?q=awDocumentKeyPress

no-op: Number (1) of the dispensable TB callstack. SM has its own (only for mailing lists), with different functionality, so we preserve their listener which calls that in shared abMailListDialog.js.

::: mailnews/addrbook/content/abMailListDialog.js
@@ +173,5 @@
>  
>    AppendNewRowAndSetFocus();
>    awFitDummyRows(1);
>  
> +#ifdef MOZ_SUITE

SM is still using this.
No-op and erroneous for TB: this triggers the dispensable callstack involving awMenulistKeyPress, but recipient type selector menulist doesn't even exist in the mailing list dialog. That's because addressCol1 is the recipient type column in composition, but the recipient column in the mailing list dialog. Error-prone design, now removed.

@@ +257,5 @@
>      // completely.
>      document.getElementById("addressingWidget").disabled = true;
>    }
>  
> +#ifdef MOZ_SUITE

SM is still using this, but no-op for TB.

Comment 40

[Magnus Melin [:mkmelin]]

No new preprocessing requirements in js files please, that makes them hard to work with.
You can check AppConstants.MOZ_BUILD_APP == "comm/mail" I think.

But, not a huge point pretending files like these are shared...

Comment 41

[Richard Marti (:Paenglab)]

Maybe AppConstants.MOZ_APP_NAME == "seamonkey" would be clearer.

Comment 42

[Thomas D. (:thomas8)]

Attachment: 1528840_2.1_TB_aw-unused-code-cleanup.patch (V.4.1): remove #ifdef

Annotations see comment 39.

Magnus, could you review this?

Comment 43

[Thomas D. (:thomas8)]

Attachment: 1528840_3_menulist-visibility.patch (V.1): Ensure visibility of focused recipient type menulist when keys are pressed

Part 3 is a peripheral mini bug fix for the recipient type selector.

STR

1. composition, add 5 recipients (sic) R1, R2 etc.
2. focus recipient type menulist of R1 (click menulist or shift-tab from R1)
3. use addressing widget scrollbar to scroll down to the last recipient (R5).
4. press "C" because you think you're in msg body and want to write "Conny"; or press cursor down (e.g. with an intention of navigating recipients list with cursor keys after we've enabled that in bug 1527547)

Actual result

• user has unintentionally changed recipient type, but the focused menulist where the change occured is still hidden; that's error-prone (and can be pretty consequential, imagine unintentionally changing bcc to cc)

Expected result

• when pressing keys on a focused element (which may change its value), the visibility of that element must be ensured to avoid user errors (unintentional change of value)
• here: any potentially value-changing keypress on the menulist should scroll it back into view. (Ideally, unrelated keypresses like alt+s to focus subject should be ignored).

Comment 44

[Thomas D. (:thomas8)]

Comment on attachment 9051650 [details] [diff] [review]
1528840_3_menulist-visibility.patch (V.1): Ensure visibility of focused recipient type menulist when keys are pressed

Geoff, any idea why addEventListener() fails on the addressing widget, and only works on the document? According to documentation [1], this should work on <richlistbox>.
It's not nice having to listen on the entire composition window when we only want to listen on the addressing widget...

````
  let addressingWidget = GetMsgAddressingWidgetTreeElement();
  console.log(addressingWidget);
  // addressingWidget.addEventListener("keyup", awOnKeyUp);
  document.addEventListener("keyup", awOnKeyUp);
````

Btw, how hard would it be make ensuring visibility of menulist when receiving (relevant) keypresses an **inbuilt** functionality of the menulist? See comment 43 for details.
We also have the same problem for pressing Alt+Cursor down on a focused, but hidden menulist: the menu will appear at some random location in the UI, which is confusing and error-prone.
For comparison, scroll a focused textbox out of view and start typing text: it automatically comes back into view.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/richlistbox

Comment 45

[Geoff Lankow (:darktrojan)]

(In reply to Thomas D. from comment #44)

Geoff, any idea why addEventListener() fails on the addressing widget, and only works on the document?

No idea. It works for me.

Btw, how hard would it be make ensuring visibility of menulist when
receiving (relevant) keypresses an inbuilt functionality of the
menulist? See comment 43 for details.

I don't know how hard it would be, but it seems like something that should be addressed. I suggest filing a bug in Toolkit::XUL Widgets if there isn't already one.

Comment 46

[Magnus Melin [:mkmelin]]

Comment on attachment 9051529 [details] [diff] [review]
1528840_2.1_TB_aw-unused-code-cleanup.patch (V.4.1): remove #ifdef

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

Looks ok r=mkmelin. You realize dxr is out of date for comm-central, right? use searchfox.org

Comment 47

[Thomas D. (:thomas8)]

Attachment: 1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5): more cleanup.

Comment 48

[Richard Marti (:Paenglab)]

Comment on attachment 9051650 [details] [diff] [review]
1528840_3_menulist-visibility.patch (V.1): Ensure visibility of focused recipient type menulist when keys are pressed

It could be surprising for the user that the widget jumps to the line the action happens but this is better than working in the hidden.

Comment 49

[Thomas D. (:thomas8)]

Attachment: 1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5.1): fix AppConstants

Comment 50

[Thomas D. (:thomas8)]

Comment on attachment 9052052 [details] [diff] [review]
1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5.1): fix AppConstants

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

Magnus, thanks a lot for reviewing the patch (comment #46 on attachment 9051529 [details] [diff] [review]).

I have done a bit more cleanup in the same patch which you have already reviewed, and fixed the missing AppConstants which had slipped the review and my tests.
I'll comment on the changes. Could you please review them. Tia.

Iann, from my reading, this patch should not affect Seamonkey in any way, even though abMailListDialog.js is shared.
But to be on the safe side, please have a look.

(In reply to Magnus Melin [:mkmelin] from comment #46)
> You realize dxr is out of date for comm-central, right? use searchfox.org

Thanks, noted and applied :-)

::: mail/components/compose/content/MsgComposeCommands.js
@@ -2567,5 @@
>    dictionaryRemovalObserver.addObserver();
>  
>    var identityList = document.getElementById("msgIdentity");
>  
> -  document.addEventListener("keypress", awDocumentKeyPress, true);

The removal of this no-op listener and the entire subsequent call stack (see below) has been reviewed in the previous patch:

awDocumentKeyPress > awMenulistKeyPress >  awTabFromMenulist >
     listBox.listBoxObject.ensureIndexIsVisible(listBoxRow - 1);

Not doing anything as ensuring visibility is apparently now inbuilt with focus change.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ -743,5 @@
> -  }
> -
> -  // be sure to add the user add recipient to our ignore list
> -  // when the user tabs out of an autocomplete line...
> -  addRecipientsToIgnoreList(element.value);

In previous patch, the spellcheck thingy was the only line left in this function, awTabFromRecipient(). So I moved it into the only TB caller of that function, awRecipientKeyPress(), and did a little cleanup there, too.

https://searchfox.org/comm-central/search?q=awtabfromrecipient

@@ +793,5 @@
>  
>  function awRecipientKeyPress(event, element) {
> +  switch (event.key) {
> +  case "Enter":
> +  case "Tab":

More readable now using event.key (and event.keycode is deprecated)!

@@ +795,5 @@
> +  switch (event.key) {
> +  case "Enter":
> +  case "Tab":
> +    // If the recipient input text contains a comma (we also convert pasted line
> +    // feeds into commas), check if multiple recipients and add them accordingly.

Corrected comment.

@@ +804,5 @@
> +    } else if (event.key="Tab") {
> +      // Single recipient added via Tab key:
> +      // Add the recipient to our spellcheck ignore list.
> +      // For Enter key, this is done in awReturnHit().
> +      addRecipientsToIgnoreList(element.value);

Added comment. This is the spot where we've integrated the single remaining line from awTabFromRecipient().

::: mailnews/addrbook/content/abMailListDialog.js
@@ +6,5 @@
>  /* import-globals-from ../../../mail/components/compose/content/addressingWidgetOverlay.js */
>  
>  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var {MailServices} = ChromeUtils.import("resource:///modules/MailServices.jsm");
> +var {AppConstants} = ChromeUtils.import("resource://gre/modules/AppConstants.jsm");

This was missing in the last patch.

@@ -397,5 @@
>    return document.getElementById("addressCol1#" + row);
>  }
>  
> -function awTabFromRecipient(element, event) {
> -  // If we are the last element in the listbox, we don't want to create a new row.

Seamonkey still have their own awTabFromRecipient(), so I think removing this function shouldn't affect them. SM's mailing list dialog should be tested to double-check.

@@ -399,5 @@
>  
> -function awTabFromRecipient(element, event) {
> -  // If we are the last element in the listbox, we don't want to create a new row.
> -  if (element == awGetInputElement(top.MAX_RECIPIENTS))
> -    top.doNotCreateANewRow = true;

As explained on previous patches, this was a no-op, buggy, and we don't really want tab to create new rows, which is irritating for tab navigation.

Comment 51

[Ian Neal]

Comment on attachment 9052052 [details] [diff] [review]
1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5.1): fix AppConstants

>+++ b/mail/components/compose/content/addressingWidgetOverlay.js

> function awRecipientKeyPress(event, element) {
>-  switch (event.keyCode) {
>-  case KeyEvent.DOM_VK_RETURN:
>-  case KeyEvent.DOM_VK_TAB:
>-    // if the user text contains a comma or a line return, ignore
>+  switch (event.key) {
>+  case "Enter":
>+  case "Tab":
>+    // If the recipient input text contains a comma (we also convert pasted line
>+    // feeds into commas), check if multiple recipients and add them accordingly.
>     if (element.value.includes(",")) {
>-      var addresses = element.value;
>+      let addresses = element.value;
>       element.value = ""; // clear out the current line so we don't try to autocomplete it..
>       parseAndAddAddresses(addresses, awGetPopupElement(awGetRowByInputElement(element)).value);
>-    } else if (event.keyCode == KeyEvent.DOM_VK_TAB) {
>-      awTabFromRecipient(element, event);
>+    } else if (event.key="Tab") {
Drive-by: shouldn't this be (event.key == "Tab")
>+      // Single recipient added via Tab key:
>+      // Add the recipient to our spellcheck ignore list.
>+      // For Enter key, this is done in awReturnHit().
>+      addRecipientsToIgnoreList(element.value);
>     }
> 
The shared code seems to leave what SM does intact, so f+ from me.

Comment 52

[Thomas D. (:thomas8)]

(In reply to Geoff Lankow (:darktrojan) from comment #45)

(In reply to Thomas D. from comment #44)

Geoff, any idea why addEventListener() fails on the addressing widget, and only works on the document?

No idea. It works for me.

Thanks for checking, but I think there's a misunderstanding, could you please double-check:

Patch has this line activated (which works):

document.addEventListener("keyup", awOnKeyUp);

awOnKeyUp will show on console as expected.
But if I use the following line instead, the listener doesn't complain when it's registered, but the function of the listener never gets called:

addressingWidget.addEventListener("keyup", awOnKeyUp);

Iow, listener on document works as expected, but same listener on addressingWidget does NOT work. But according to documentation, it should work imo.

Geoff, can you please try this again:
Clear error console, outcomment the document listener from patch, and activate the addressingWidget listener.
Do you then see this message on error console?

console.log("awOnKeyUp");

Myself I'm not seeing that message, meaning the listener directly on addressingWidget is failing.
It should be the same for you...

Comment 53

[Geoff Lankow (:darktrojan)]

Okay, I see why that's happening and why what I tried worked. In this odd piece of code the listbox is removed and replaced with a clone of itself. So the event listener you added is no longer useful. Why does this happen? I have no idea.

I'll post a patch that replaces the odd piece of code.

Comment 54

[Geoff Lankow (:darktrojan)]

Attachment: 1528840-addressing-widget-clone-1.diff

This should fix your problem with events not fired. I'll leave it to you to get it reviewed and landed.

Comment 55

[Magnus Melin [:mkmelin]]

Comment on attachment 9052052 [details] [diff] [review]
1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5.1): fix AppConstants

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +802,3 @@
>        element.value = ""; // clear out the current line so we don't try to autocomplete it..
>        parseAndAddAddresses(addresses, awGetPopupElement(awGetRowByInputElement(element)).value);
> +    } else if (event.key="Tab") {

Should be ==

While you're heere, please indent the case clauses. It should be 

switch (event.key) {
  case "Enter":
  case "Tab":
......
}

Comment 56

[Thomas D. (:thomas8)]

(In reply to Geoff Lankow (:darktrojan) from comment #54)

Created attachment 9053027 [details] [diff] [review]
1528840-addressing-widget-clone-1.diff

This should fix your problem with events not fired. I'll leave it to you to get it reviewed and landed.

Thanks a lot, you're a genius!
But why did they clone it in the first place? Without the clone, are we not breaking something else?

Aceman?

Comment 57

[Thomas D. (:thomas8)]

Attachment: 1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5.2): Nitfix comparison operator, indent

Nits fixed, thanks. Anything else? I'm asking review again bc you didn't indicate "f+ with these nits fixed..." and also I prefer original flags from reviewer on my patches.

Comment 58

[Magnus Melin [:mkmelin]]

Comment on attachment 9053058 [details] [diff] [review]
1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5.2): Nitfix comparison operator, indent

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +800,5 @@
> +      if (element.value.includes(",")) {
> +        let addresses = element.value;
> +        element.value = ""; // clear out the current line so we don't try to autocomplete it..
> +        parseAndAddAddresses(addresses, awGetPopupElement(awGetRowByInputElement(element)).value);
> +      } else if (event.key=="Tab") {

nit: should have space around ==

::: mailnews/addrbook/content/abMailListDialog.js
@@ +174,5 @@
>  
>    AppendNewRowAndSetFocus();
>    awFitDummyRows(1);
>  
> +if (AppConstants.MOZ_APP_NAME == "seamonkey") {

this isn't preprocessed code, so the if indention should be like a normal if, not at start of the line

sorry I didn't see that earlier

Comment 59

[Thomas D. (:thomas8)]

Attachment: 1528840_2.1_TB_aw-unused-code-cleanup.patch (V.5.3): Nitfix indents

(In reply to Magnus Melin [:mkmelin] from comment #58)

Comment on attachment 9053058 [details] [diff] [review]
...
sorry I didn't see that earlier

No problem, I appreciate the fast turnaround time!

Comment 60

[Thomas D. (:thomas8)]

Jörg, could you please land attachment 9053069 [details] [diff] [review], then leave open. Thank you.

Comment 61

[Thomas D. (:thomas8)]

Attachment: [Landed in comment 76] 1528840_3a_TB_addressing-widget-no-clone.patch

Magnus, Geoff discovered that we were cloning the addressingWidget before populating it and it's not clear why. Any ideas? Cloning breaks element references like my aw keyup listener. So Geoff removed the cloning part and it seems to work just like before.

Comment 62

[Thomas D. (:thomas8)]

Attachment: 1528840_3b_TB_menulist-visibility.patch (V.2): listen for keyup on addressing widget

Geoff, from experience we can wait for changes to XUL widgets forever, so I suggest that be fix the visibility issue in TB first - we can always back that out when the widget behaviour improves.

Geoff, Aceman, Magnus: For the address widget, there's always a choice between having dedicated onevent="eventhandler" attributes on elements of every recipient row, which may get duplicated 1000 times, causing considerable document bloat in the process.
If we add the listener on a higher level like the addressing widget, code bloat is avoided, but it will trigger also on unrelated events, e.g. here we'll trigger for keyup on recipient input field and then bailout as we only want act on the menulist.
I wonder between code bloat and false positives, which one is overall better wrt performance/memory/code etc?

We should look into this and maybe get performance data for all the event listeners because composition with many recipients (say 500+) is always very slow.

Comment 63

[Jorg K (CEST = GMT+2)]

You really need to run the linter and do a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=18bc2be722cd47128abdf1d6cebb05895b3d5011
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/addrbook/content/abMailListDialog.js:179:43 | 'awDocumentKeyPress' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/addrbook/content/abMailListDialog.js:263:43 | 'awDocumentKeyPress' is not defined. (no-undef)

I'm not landing anything without it any more.

Comment 64

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #63)

You really need to run the linter and do a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=18bc2be722cd47128abdf1d6cebb05895b3d5011
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/addrbook/content/abMailListDialog.js:179:43 | 'awDocumentKeyPress' is not defined. (no-undef)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mailnews/addrbook/content/abMailListDialog.js:263:43 | 'awDocumentKeyPress' is not defined. (no-undef)

I'm not landing anything without it any more.

Except maybe when the try run is lying to you because it's not smart enough to understand this application-specific conditional:

  if (AppConstants.MOZ_APP_NAME == "seamonkey") {
    document.addEventListener("keypress", awDocumentKeyPress, true);
  }

We only add the listener for SeaMonkey, and they still have that function.
The function is undefined only for TB, where it never runs. Looks right to me?

https://searchfox.org/comm-central/search?q=awdocumentkeypress

Comment 65

[Magnus Melin [:mkmelin]]

You need to add the global statement so the linter would know. Like (I think)
/* global awDocumentKeyPress */

Comment 66

[Jorg K (CEST = GMT+2)]

Yep.

Comment 67

[Thomas D. (:thomas8)]

Attachment: [Landed in comment 70] 1528840_2.1_TB_unused-code-cleanup.patch (V.5.4): Shut up the linter

So let's try this in a trybuild. Aceman?

Comment 68

[:aceman]

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

Comment 69

[:aceman]

Comment on attachment 9053719 [details] [diff] [review]
[Landed in comment 70] 1528840_2.1_TB_unused-code-cleanup.patch (V.5.4): Shut up the linter

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

I have played with this and also haven't found problems compared to before the patch.

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/475b48fdc301
Part 2.1, TB: addressingWidgetOverlay.js, abMailListDialog.js unused/no-op code cleanup. r=mkmelin DONTBUILD

Comment 71

[Jorg K (CEST = GMT+2)]

I played with it too and found bug 1539295 :-( - That's why I delayed landing, hence the DONTBUILD.

Comment 72

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #71)

I played with it too and found bug 1539295 :-( - That's why I delayed landing, hence the DONTBUILD.

For the avoidance of doubt (which clarification could have been part of your comment), from the findings in the other bug, this bug has NOT caused bug 1539295.

Comment 73

[Magnus Melin [:mkmelin]]

Comment on attachment 9053199 [details] [diff] [review]
[Landed in comment 76] 1528840_3a_TB_addressing-widget-no-clone.patch

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

Seems the cloning is redundant. r=mkmelin

Comment 74

[Magnus Melin [:mkmelin]]

Comment on attachment 9053202 [details] [diff] [review]
1528840_3b_TB_menulist-visibility.patch (V.2): listen for keyup on addressing widget

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +991,5 @@
> +      let listbox = document.getElementById("addressingWidget");
> +      listbox.ensureElementIsVisible(eventTarget);
> +    }
> +  } catch (e) {
> +  }

wrapping it all in a try-catch is really bad style. this shouldn't be needed.

Comment 75

[Jorg K (CEST = GMT+2)]

Let's check in attachment 9053199 [details] [diff] [review].

Comment 76

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9dda6f78d5ad
Part 3a, TB: Stop replacing addressingWidget with a clone of itself. r=mkmelin

Comment 77

[Jorg K (CEST = GMT+2)]

Part 3b is the last part here? Can we wrap up this bug before TB 68 goes to beta in 10 days?

Comment 78

[Thomas D. (:thomas8)]

Attachment: 1528840_3b_TB_menulist-visibility.patch (V.2.1): remove try{}

(In reply to Jorg K (GMT+2) from comment #77)

Part 3b is the last part here? Can we wrap up this bug before TB 68 goes to beta in 10 days?

Yes to both.

(In reply to Magnus Melin [:mkmelin] from comment #74)

Review of attachment 9053202 [details] [diff] [review]
wrapping it all in a try-catch is really bad style. this shouldn't be needed.

I don't recall why the try-catch was added in the first place, Aceman, any ideas?
Aceman, may you please do a try run. Tia.

Comment 79

[Thomas D. (:thomas8)]

Comment on attachment 9064335 [details] [diff] [review]
1528840_3b_TB_menulist-visibility.patch (V.2.1): remove try{}

(In reply to Magnus Melin [:mkmelin] from comment #74)

> Review of attachment 9053202 [details] [diff] [review] 
> wrapping it all in a try-catch is really bad style. this shouldn't be needed.

This addresses the review.

Comment 80

[:aceman]

Comment on attachment 9064335 [details] [diff] [review]
1528840_3b_TB_menulist-visibility.patch (V.2.1): remove try{}

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

I don't remember why the try {} block was there. Let's try without. Unless event.target can be null at times.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=245713611&revision=48481cb18f1153d33e2dc38efef8490a32267062

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3719,2 @@
>    gContentChanged = true;
> +  awSetAutoComplete(row);

Why is it better with the temp variable?

Comment 81

[Magnus Melin [:mkmelin]]

Comment on attachment 9064335 [details] [diff] [review]
1528840_3b_TB_menulist-visibility.patch (V.2.1): remove try{}

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

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

GetMsgAddressingWidgetTreeElement should be removed really... it's a wrapper for document.getElementById

::: 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) {

there's no reason to add this here instead of inline where it's used the one time

@@ +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")) {

can we instead check event.target.classList.contains("textbox-addressingWidget")

@@ +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");

ugly to hardcode the id here. especially as what you're looking for is "this" ;) (if you use => syntax for setting the caller)

Comment 82

[Thomas D. (:thomas8)]

Comment on attachment 9064335 [details] [diff] [review]
1528840_3b_TB_menulist-visibility.patch (V.2.1): remove try{}

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.

Comment 83

[Magnus Melin [:mkmelin]]

Comment on attachment 9064335 [details] [diff] [review]
1528840_3b_TB_menulist-visibility.patch (V.2.1): remove try{}

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

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

I'm not suggesting moving anything to xul. You would simply have here instead

addressingWidget.addEventListener("keyup", (event) => {
  if (event.target.classList.contains("textbox-addressingWidget")) {
    this.ensureElementIsVisible(event.target);
  }
});


I think that is all. Very short, no need to read a lot of documentation.

Comment 84

[Thomas D. (:thomas8)]

Attachment: [Landed in comment 86 for TB 68] 1528840_3b_TB_menulist-visibility.patch (V.3: inline event listener with => per Magnus request)

(In reply to Magnus Melin [:mkmelin] from comment #83)

Comment on attachment 9064335 [details] [diff] [review] ...
You would simply have here instead

addressingWidget.addEventListener("keyup", (event) => {
  if (event.target.classList.contains("textbox-addressingWidget")) {
    this.ensureElementIsVisible(event.target);
  }
});

I think that is all. Very short, no need to read a lot of documentation.

Aaalright... Elegant! :-) I corrected the class and replaced 'this' with 'addressingWidget' to make it work (your 'this' refers to Chrome Window).

I have reduced comments to the minimum; some documentation is required because this is just a quick and dirty hack for ux-error-prevention wrt accidental, covert recipient type changes. Someone should file a bug to make a more refined variant of this inbuilt with <menulist>.

Comment 85

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/780e49e2d1a9
Part 3b, TB: Ensure visibility of focused recipient type menulist when keys are pressed. r=mkmelin, ui-r=Paenglab

Comment 86

[Jorg K (CEST = GMT+2)]

Last part landed in TB 68.

Comment 87

[Thomas D. (:thomas8)]

Comment on attachment 9066026 [details] [diff] [review]
[Landed in comment 86 for TB 68] 1528840_3b_TB_menulist-visibility.patch (V.3: inline event listener with => per Magnus request)

Thank you!