Nice, looks good to me. In NVDA changelog entries though, it's typical to link to the issue that it solves (if applicable).

Thanks @TheQuinbox, I just updated the initial description with that, as well as the checkbox for the testing part, since the manual check checks out nicely.

Typo to fix in the change log:
#13321 -> #13221

Have you considered adding Chrome system test demonstrating the behavior based on the code snippet in #13221?

Thanks @CyrilleB79, corrected the typo.

@lukaszgo1 I'll try that system test thing, but this is all still very confusing, so may take a while. And it's made difficult by the fact that I can't currently even run local builds, and that includes tests. See my message in the nvda-devel mailing list. But I'll try nevertheless.

Thanks Marco, this is a nice improvement! One question: Isn't it just enough that a list item has focus? Why check the focusable state on the parent list as well??

@LeonarddeR I was going with @michaelDCurran's statement from #13221 (comment):

We will switch to focus mode if a listItem gets focus, but only if the list itself is also focusable. I.e. the list has at least tabindex of -1.

This is how MS Edge's Download Manager list works, and it means that @kloots will have to make a slight adjustment in Slack to put tabindex="-1" on the actual list, which Slack currently doesn't do yet.

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/m5ua9hmooat6kdvm/artifacts/output/nvda_snapshot_pr13357-24717,d2aff996.exe

See test results for failed build of commit d2aff99624

For reference, the reasoning behind requiring focusable on the list is to limit where this code will apply.

1. Making ul (or role="list) and (li or role="listitem") interactive like this does not quite fit standards as really these are presentational read-only lists. Interactive lists should be role="listbox" and role="option".
2. There may already exist some web pages that deliberately set focus to a presentational list item when the page is scrolled to an arbitrary location. In this case switching to focus mode would be annoying and inappropriate.

But, it has been made clear that the standard does not really fit some particular use cases, and therefore this change is needed. Therefore it is being added, but with the limitation that the list must also be focusable. This idea was proposed by Slack themselves.

I think the title of this pr should refer to Focus mode, not Browse mode.

I think the title of this pr should refer to Focus mode, not Browse mode.

Done.

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/k45p4syf89iwxxxp/artifacts/output/nvda_snapshot_pr13357-24733,c59b1d15.exe

See test results for failed build of commit c59b1d1579

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/6t7i5waajl9yq9o8/artifacts/output/nvda_snapshot_pr13357-24735,a6844f4d.exe

See test results for failed build of commit a6844f4d79

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/dcuu9reda0bd27wl/artifacts/output/nvda_snapshot_pr13357-24736,59e227bd.exe

See test results for failed build of commit 59e227bd53

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/pmsb4byw4l8un70j/artifacts/output/nvda_snapshot_pr13357-24737,f441d32e.exe

See test results for failed build of commit f441d32e50

Sorry this is going so slowly. I cannot run the tests on my local machine because it is not an English language Windows, and therefore all NVDA tests fall on their face. So I have to peel this like an onion.

• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests. See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/93vsqfmuo5q5vr7e/artifacts/output/nvda_snapshot_pr13357-24739,91fb4e4a.exe

See test results for failed build of commit 91fb4e4aae

@michaelDCurran I don't understand this error message. The previous test failures were clear, but this one confuses me. It says:

Error message:
focus mode - focus link in document and turn off focus mode
speech Actual != Expected: message
Todd Kloots link != message
Todd Kloots link
Browse mode

Any help is appreciated.

Looks like we are not automatically switching back to browse mode when tabbing into the document and on to the first link. However, tabbing to the second link does switch.
This is definitely a bug in NVDA, no doubt to do with the fact that the list item and the start of the inner document are at exactly the same offset.
See issue #12896.
We do hope to put significant time into that and related focus issues in the very near future.
For now perhaps simplify the testcase to only handle the listitem and not tabbing into the document. Though you may want to instead add a link after the list and tab to that, and ensure that browse mode turns back on.

I just extended the test case to tab to the second link, which does switch to browse mode, and then shift-tab twice. However, if that fails, I'll go with your suggestion and just do the very simple one, which now passes. Thanks!

@michaelDCurran I went with your suggestion of simplifying the test case after all, and it now passes. I can't clear the "changes requested" flag from the issue even though I applied your suggested code change, and also updated the PR title. So, tossing this back for review in hopes that things look good so far. I'd also be happy to help out with #12896 if that's desired.

Thanks for your work on this @MarcoZehe.
I shall review this sometime in the next couple of days.

This is great! Do you think you can think of a way to fix #12266? It's a similar list focus problem. Thanks!

I just commented on the issue: It is a different bug, which will not be fixed by this pull request. The bug is in the Chromium engine, not NVDA in that case. The markup is also radically different from what is being used here.

@michaelDCurran I see this has been added to the 2022.1 release milestone. Does the team have a release date spec'd for 2022.1?

@kloots perhaps end of March / early April.

@michaelDCurran curious about the release date for this fix: any update as to when this will go out to production?

Great. Thanks @michaelDCurran. Dumb question: how do I get the beta?

Thanks @michaelDCurran. One last question, do you have a sense for how quickly NVDA users upgrade?