Multi-account-containers: Using keyboard to open a container automatically sorts tabs

[anbenson]

• Multi-Account Containers Version: Various - see below
• Operating System + Version: Linux, kernel 5.5.4
• Firefox Version: 73.0.1
• Other installed Add-ons + Version + Enabled/Disabled-Status: not relevant - see details

Actual behavior

When selecting a container using the keyboard, tabs are automatically sorted. (It's really frustrating :/ )

Expected behavior

When selecting a container using the keyboard, tabs are _not_ automatically sorted.

Steps to reproduce

1. Ctrl + .
2. Tab to a container.
3. Press enter.

Notes

I have done a bunch of debugging to try to help the diagnosis. Here's what I've learned:

• This behavior does not occur if using the mouse, which makes me more confident that it's a bug.
• I've tried cloned the repo and done a git bisect. The regression was introduced in f159e42, which merges a relevant PR (#1539). (tagging @dnahol the author of the PR who probably has the most context here)
• If you look at the code, it listens for an enter key (which is done in the repro steps) and responds by clicking on the first link it can find in the panel. It looks like this was added for onboarding, but it breaks the normal panel, since the first link it finds is the "Sort Tabs" link.
• I have confirmed that removing the code introduced by the PR fixes the issue.
• I don't know much about this Onboarding code, but I assume it's the panels that show up on first run (which I activated by deleting the 'onboarding-stage' key in extension storage. It looks like this is still a11y-friendly - I can use the keyboard to navigate it as long as I tab once per screen (it'd be nice to autofocus the button though?). So maybe this code isn't necessary, and removing it is a sufficient fix? In my experience, the best solution to a11y issues is appropriate use of tabIndex when possible, not JS.

[kendallcorner]

Looks like the cause of #1637

[nikoladev]

Great detective work!

So maybe this code isn't necessary, and removing it is a sufficient fix? In my experience, the best solution to a11y issues is appropriate use of tabIndex when possible, not JS.

If removing the code isn't viable and appropriate use of tabIndex is too difficult to implement quickly, maybe it would be possible to do a quick check in the offending case to see if we are in the onboarding phase? If we're not we can just break out of the case before any of the code is run.

[kendallcorner]

This fix is merged and sent to the addon's team. You should see an update in 1-2 business days.