Setup a11y: using the Tab key to select the language

[mishanthrop]

What does it do?

Improved keyboard navigation during language selection.

Why is it needed?

Because accesibility

How to test

1. goto http://localhost/setup/?action=language
2. use keyboard to select language and switch between popular and other.

Related issue(s)/PR(s)

video by @Ibochkarev
https://user-images.githubusercontent.com/5341208/134926009-e72304cd-7c51-4d64-b250-3ffc78d00df8.mp4

[Mark-H]

Congrats on your first pull request :D

This seems like a step back semantically and (partially) accessibly.

When the language options are radio buttons (like they were), you can tab into the selected value and then use the arrow keys to navigate the different options. Turning them into buttons means they all can get focus, so you're going to have to tab through them all if you just want to get to the button for the next step. That would make it less accessible from an interaction point of view.

Semantically, as you're not performing an action, buttons isn't right either.

In the old situation there's no clear focus indicator, so it definitely makes sense to improve that to clarify where the user is currently, but definitely not with <button>.

[mishanthrop]

@Mark-H thanks for review, I didn't take this moment into account.
Anyway, buttons "All languages" and "Only popular" are not focusable, so, I will be back with corrections :-)
Also I just tested this screen with a VoiceOver and it turned out be very verbose (VoiceOver reads all button text, eg "English English EN").

[opengeek]

See notes from @Mark-H

[mishanthrop]

@Mark-H, now this seems like a step forward semantically and (partially) accessibly :-)

Also fixed a bug for the case when the number of languages is a multiple of 4 (look at the right bottom).

[Mark-H]

Great - I only have one comment from a code review for now, which is that the javascript seems be lacking the ; at the end of each line. Maybe I'm old and out of date on what the cool kids are doing these days, but... isn't the ; still required for vanilla (non-compiled) javascript?

Next step before getting a thumbs up is to test it which may lead to some further issues. That may take a little longer than a code review.

[mishanthrop]

@Mark-H browser compatibility is a good question for disqussion.

Semicolon

In most cases, the semicolon is a matter of taste. I tested my code in the IE 11 and didn't find any semicolon related errors.

In general

As is mentioned on https://docs.modx.com/current/en/getting-started/server-requirements#browser-support-for-the-manager the Internet Explorer 11 is supported. Despite this, the some code contains unsupported API.

setup\templates\header.tpl

url = new URL(url_string)

But... https://developer.mozilla.org/en-US/docs/Web/API/URL/URL#browser_compatibility ¯\_(ツ)_/¯

Conclusion

The only unsupported thing in my code is forEach (I fixed this).
But I have more radical proposals.
I think it is necessary to globally revise the approach to the Frontend in the MODX 3 and start to write code in the ES6 and higher (using transpilers of course).
First, it will allow developers to write more concise and safer code.
Second, writing code using the old standard (ES5) is nostalgic demotivation. This is tantamount to using PHP4.
MODX already uses SASS instead of vanilla CSS. So why not do the same with JS (I'm not even talking about the TypeScript)?

[Mark-H]

For sure, a discussion can be had about that (in an issue or the forum, not in a PR). Also worth keeping in mind the setup does not currently use the same build process as the manager.

Looks to me like that Url() syntax will need to be fixed then. ;)

[mishanthrop]

Looks to me like that Url() syntax will need to be fixed then. ;)

I hope not in this PR

For sure, a discussion can be had about that (in an issue or the forum, not in a PR). Also worth keeping in mind the setup does not currently use the same build process as the manager.

I'll try to find resources to research current architecture and propose something.

[JoshuaLuckers]

Looks to me like that Url() syntax will need to be fixed then. ;)

I hope not in this PR

It's indeed out of scope for this PR (however I don't think we need to fix it and just bump the browser requirements even although the documentations mentions the browser compatibility is for the manager).

[smg6511]

@mishantrop - Just a reminder in terms of the ability to use ES6+, a PR that I authored (#15793) (that you commented on recently) was merged recently so the use of the newer js features you mention are available now. The js you see in the manager to date is all ES5-compliant because the old build workflow was incompatible with anything higher.

All - I'll start a thread in the forum under development re syntax style and link to it here soon. There are a couple thoughts I've been meaning to put out in that regard and we can address things like the semicolon question there as well.

[rthrash]

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/thoughts-on-manager-code-style-includes-polls/4484/1

[Mark-H]

Looks good, works as advertised.

Changes made

[opengeek]

Please do not include the compiled CSS in the PRs you submit in the future.

[mishanthrop]

Please do not include the compiled CSS in the PRs you submit in the future.

I'll remember that.

P.S.
These files were already stored in the repository. There is no information that they should not be there. Since these files should not be in the repository, need to add them to the .gitignore and remove them from the repository.

[opengeek]

They have to be in the repository. Just like the CSS for the manager, these have to be compiled from the SCSS when merging or testing a PR. If you include the compiled changes to them in the PR, then they will potentially conflict with changes to those files from other PRs.

[mishanthrop]

Did not know about such CI-related details, thanks