Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] [a11y] Add SkipTo Navigation plugin #24142

Merged
merged 22 commits into from
Mar 26, 2019
Merged

Conversation

brianteeman
Copy link
Contributor

Integration of the great accessibility plugin by PayPal Accessibility Team and University of Illinois

SkipTo is a replacement for your old classic "Skipnav" link, (so please use it as such)! The SkipTo script creates a drop-down menu consisting of the links to the important places on a given web page. Once installed and configured, the menu makes it easier for keyboard and screen reader users to quickly jump to the desired location by simply choosing it from the list of options.

How it works

The SkipTo menu becomes visible the first time the user tabs into the page.
Once the keyboard focus is on the menu, pressing the ENTER or the SPACEBAR key will pull down the list of high-level headings and landmarks on the current page.
Use arrow keys to select your choice and press ENTER to skip to it.
If you decide to reach the menu again, simply press the built-in access key (0 by default).

Joomla options

Obviously added the ability for the strings to be translated
In the plugin you have the option to enable this navigation aid on Site/Administrator/Both - The default is admin

SkipTo options

By default, SkipTo menu will include the following places on the page:

Heading (e.g h1, h2, h3 and h4 elements).
ARIA landmarks (e.g. banner, navigation, main and search).
HTML5 Section Elements (e.g. main, section[aria-label], section[aria-labelledby]
The “access key” is set to 0.
The menu is set not to wrap.
The menu is visible on keyboard focus only

The above are configurable but I have not exposed the configuration to Joomla - but can do. I just think the complicate things and the defaults are fine

Testing

Testing will require a full npm install

skipto

Thanks to @wilsonge for getting me over the final hurdles

Integration of the great accessibility plugin by PayPal Accessibility Team and University of Illinois

SkipTo is a replacement for your old classic "Skipnav" link, (so please use it as such)! The SkipTo script creates a drop-down menu consisting of the links to the important places on a given web page. Once installed and configured, the menu makes it easier for keyboard and screen reader users to quickly jump to the desired location by simply choosing it from the list of options.

### How it works
The SkipTo menu becomes visible the first time the user tabs into the page.
Once the keyboard focus is on the menu, pressing the ENTER or the SPACEBAR key will pull down the list of high-level headings and landmarks on the current page.
Use arrow keys to select your choice and press ENTER to skip to it.
If you decide to reach the menu again, simply press the built-in access key (0 by default).

### Joomla options
Obviously added the ability for the strings to be translated
In the plugin you have the option to enable this navigation aid on Site/Administrator/Both - The default is admin

### SkipTo options
By default, SkipTo menu will include the following places on the page:

Heading (e.g h1, h2, h3 and h4 elements).
ARIA landmarks (e.g. banner, navigation, main and search).
HTML5 Section Elements (e.g. main, section[aria-label], section[aria-labelledby]
The “access key” is set to 0.
The menu is set not to wrap.
The menu is visible on keyboard focus only

The above are configurable but I have not exposed the configuration to Joomla - but can do. I just think the complicate things and the defaults are fine
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Mar 9, 2019
@brianteeman brianteeman mentioned this pull request Mar 9, 2019
$current_section = 0;
}

if (!($current_section & $section))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!($current_section & $section))
if (!($current_section && $section))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is a direct copy paste from the yubikey plugin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure we don't mean to do this http://www.php.net/manual/en/language.operators.bitwise.php - even if it does work. I'd change yubikey to match what @Quy is suggesting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work. The suggestion above does not.

brianteeman and others added 2 commits March 9, 2019 20:23
Co-Authored-By: brianteeman <brian@teeman.net>
Co-Authored-By: brianteeman <brian@teeman.net>
@SharkyKZ
Copy link
Contributor

SharkyKZ commented Mar 9, 2019

@brianteeman see brianteeman#78 for SQL fixes.

@astridx
Copy link
Contributor

astridx commented Mar 10, 2019

Thank you for this plugin. I just tested it. I noticed the following:

  1. After discovering I saw two new plugins. I ignored API Authentication - Basic Auth. I installed System - Skip-To Navigation.
    Extensions  Discover   Test   Administration(1)
  2. After click on one entry in the SkipTo menu , I get this error.
    Bildschirmfoto von 2019-03-10 13-35-51
  3. I do not understand, which links are included in the SkipTo menu . Here I find, that h4 should be included. But Most read Posts are not included in the next picture.
    Home(2)

@brianteeman
Copy link
Contributor Author

Sorry my original post is incorrect - the default settings are h1, h2, h3
We can add h4 if we want to

Copy link
Contributor

@SharkyKZ SharkyKZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL please.

@brianteeman
Copy link
Contributor Author

@SharkyKZ Why. I thought we were supposed to set the default values in the sql

@SharkyKZ
Copy link
Contributor

Based on #20563 (comment), no. As long as you set them in the plugin code.

Copy link
Contributor

@SharkyKZ SharkyKZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SQL still not fixed.

…0-2019-03-09.sql

Co-Authored-By: brianteeman <brian@teeman.net>
@brianteeman
Copy link
Contributor Author

Thanks @SharkyKZ should be fixed now

@SharkyKZ
Copy link
Contributor

Thanks, now the review starts 😁.

@brianteeman
Copy link
Contributor Author

I can't decide if we should expose the ability to allow the user to edit the following

Heading (e.g h1, h2, h3 and h4 elements).
ARIA landmarks (e.g. banner, navigation, main and search).
HTML5 Section Elements (e.g. main, section[aria-label], section[aria-labelledby]
The “access key” is set to 0.


// TODO remove this line when bug is fixed
$this->loadLanguage();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove one more tab.

@brianteeman
Copy link
Contributor Author

appveyor failed with

fatal: unable to access 'https://github.com/joomla/joomla-cms.git/': Could not resolve host: github.com


try
{
$app = $this->app;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You told me to put it in #24142 (comment)

}

// TODO remove this line when bug is fixed
$this->loadLanguage();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would keep this and remove $autoloadLanguage instead. There's no need to load language files in backend if the plugin runs only in frontend, and vice versa.

@dgrammatiko
Copy link
Contributor

@wilsonge fwiw if was still the JS leader I wouldnt accept this skipto script to enter the project's dependencies

@brianteeman
Copy link
Contributor Author

are you going to say why?

@dgrammatiko
Copy link
Contributor

are you going to say why?

  • Search for //IE8 fix:
  • The dropdown is totally useless and complete bloated code as we already have our own implementation, @C-Lodder mentioned that already.
    The very obvious ones but there are more about that code. Not everything that works is also fit for the job...

@brianteeman
Copy link
Contributor Author

  1. I was told NOT to change the code
  2. The dropdown is separate code and if someone wants to integrate a different dropdown script then great. You know my skill set.

@dgrammatiko
Copy link
Contributor

I was told NOT to change the code

That's then the reason you're looking for. You're simply adding technical debt here which is totally unneeded. We don't care for IE < 11 we shouldn't drag scripts that patch things for dead browsers...
Also, the fact that almost all functions have a patch for IE8 indicates clearly that this script is not up to date or maintained or what is my point of view that it will be maintained for the next 4-5 years that Joomla 4 will be out there...

All and all I am not against a skipto but let's do it properly, at the end of the day all there is here are some DOM manipulations nothing that even a JS noobie can't do...

@brianteeman
Copy link
Contributor Author

I would rather use working code (that can always be improved later) than wait for someone with JavaScript skills write a working solution.

You might find JavaScript easy to write. I certainly don't or I would have fixed the password strength and switcher js which are totally not accessible.

@dgrammatiko
Copy link
Contributor

I would rather use working code

That's why Joomla is a mess, everything is important and needs to be implemented right now, no matter what the technical debt that it introduces. Sorry I totally disagree here...

I would have fixed the password strength

Well, I had a PR standing there for 3 months and finally, @wilsonge said "no new custom elements". I'm not a magician, I do my best but I can't fix something if people don't merge my stupid code...

@wilsonge
Copy link
Contributor

@zwiastunsw Can you retest this please

@zwiastunsw
Copy link
Contributor

I have tested this item ✅ successfully on 44775ee


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24142.

@zwiastunsw
Copy link
Contributor

I can't decide if we should expose the ability to allow the user to edit the following ....

In my opinion it is not necessary. Theplugin has to do exactly what it does. Adding configuration options will only make the system operation more complicated.

@wilsonge wilsonge merged commit f6c4e45 into joomla:4.0-dev Mar 26, 2019
@wilsonge
Copy link
Contributor

Thanks guys!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Mar 26, 2019
@brianteeman
Copy link
Contributor Author

Thanks for testing and merging. If we ever get a new admin template this will be essential for testing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants