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

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.

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

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.

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

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.

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.

(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) { }	
}

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

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

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

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

Nits fixed.

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

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

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

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

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

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

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

Looks successful :)

There's more to come, right?

Pushed by mozilla@jorgk.com:
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

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.

(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?

ni?aceman for comment 35 ^^

This patch addresses the problem of comment 35.

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

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

Annotations see comment 39.

Magnus, could you review this?

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

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

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

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.

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

(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?

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.

(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!

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

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.

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.

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.

(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

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

Yep.

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

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

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

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

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

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

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

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

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

Pushed by mozilla@jorgk.com:
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

Last part landed in TB 68.