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.1] Joomla-Accessibility Checker (jooa11y). #36190

Merged
merged 69 commits into from
Dec 29, 2021

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Dec 3, 2021

Jooa11y is an accessibility and quality assurance tool that visually highlights common accessibility and usability issues.

Geared towards content authors, Jooa11y identifies many errors or warnings and provides guidance on on how to fix them. Jooa11y is not a comprehensive code analysis tool - it exclusively highlights content issues.

To see all the features of jooa11y you should visit https://joomla-projects.github.io/joomla-a11y-checker/ and see the examples and tests.

This PR is only for the implementation of the new Joomla-Accessibility Checker (jooa11y) in core. Any issue, bug or question about the actual accessibility checks must be made at https://github.com/joomla-projects/joomla-a11y-checker/issues

### This PR is still a Work In Progress as

1. Need to determine the best way to handle translations and none are loaded at this time in the plugin. Perhaps we go down the same path a tinymce? Suggestions welcome.

2. There are currently three different ways of activating the checker (see the tests below). Too many? Bad code? Better code? More options? Less options?

NOTES

  • As the objective of the checker is only to check the content that an author can reasonably be expected to be able to correct the check only runs on the main landmark on the page (<main>). This can be changed in the plugin to any other landmark/region
  • - The drone npm error is a known issue Drone failure - requires git #36160

Testing

  • Checkout this branch
  • npm i
  • discover the plugin
  • enable the plugin

Test 1.

  • open an existing article and see the new toolbar button
  • Click on the button in the preview window to open

image

Just like the existing preview button the button is not available until the article is saved

Test 2.

  • Go to the plugin settings and enable the first option
  • Go to the front end of the site and you can now see the plugin on every page

image

Test 3.

  • Go back to the plugin settings and disable the first option
  • Go to the front end of the site and add ?jooa11y=1 to the url
    -- (if the url already has a ? add &jooa11y=1 instead)
  • You can now see the plugin on just this page

The inclusion of this checker is a result of the research carried out by the EU funded We4authorsCluster accessibility project that I participated in.

This PR is the result of the combined work of @brianteeman, @bembelimen, @Fedik, @chmst, @carcam

@brianteeman
Copy link
Contributor Author

, it needs to change the name of the update SQL script to a later date so it runs when people update a beta 2 to the next beta 3.

really? whenever i have said that the update scripts should be immutable you have always said it doesnt matter as they all get checked

@richard67
Copy link
Member

@brianteeman It has nothing to do with the database checker. It is about the installer only running those update SQL script which have a version in their name which is newer than the schema version saved in database.

@richard67
Copy link
Member

P.S. and your files have a date older than the ones which have been added by other extensions and are already released.

@richard67
Copy link
Member

P.P.S.: Child templates was 4.1.0-2021-11-28.sql, so that will be the schema version in db for a 4.1-beta2.

@richard67
Copy link
Member

P.P.P.S: So scripts 4.1.0-2021-11-26.sql will not be run when updating a beta 2 to a beta 3.

@richard67
Copy link
Member

Clear now?

@brianteeman
Copy link
Contributor Author

even more proof that the sql located in com_admin must be immutable

@richard67
Copy link
Member

even more proof that the sql located in com_admin must be immutable

It seems you still do not understand how the updater works and how the db checker works. Anyway, I won't waste anymore time to try to explain it to you again and again. I will fix the issue instead.

@richard67
Copy link
Member

richard67 commented Dec 29, 2021

P.S.: This "immutable" discussion has absolutely nothing to do with the issue because as I clearly explained, your scripts haven't been released yet and haven't been even included in a nightly build yet, so they are definitely not immutable and can and should be changed.

@nikosdion
Copy link
Contributor

@brianteeman The CONTENT of the update SQL scripts is immutable. However, this has nothing to do with what @richard67 is telling you.

The NAMES of the update SQL files are versions. Joomla stores the name of the last update file applies for an extension in the #__schemas table. That's the “schema version” for that extensions. When you install update to that extension only update files whose name is a later version than the schema version of the extension will be executed. This is determined by using PHP's version_compare() function with two arguments: the filename (without the path and the .sql extension) and the stored schema version. If you read that page you will understand why Joomla's convention of using the next version and a dash followed by the date and time in YYYYMMDDHHmmSS (e.g. 20211229124300) format works.

This is a very important feature in Joomla and the reason why updates actually work in the first place! If this was not the case all SQL update files would execute on every update and we could no longer have immutable update file contents.

Do keep in mind that Joomla does list itself as a file type extension exactly so that core update as possible.

The names of the update SQL files you used are an older version than those which have already been installed by current beta releases. As a result, Joomla will skip over them and jooa11y won't appear installed in the database. So, yes, @richard67 is absolutely correct.

I've had to do the same in some of my PRs when the PR was far removed temporary from the merge time and other SQL updates had already been published. In most of my PRs I did not have to do that, even though other updates with later versions were already merged, because there was no officially published version of Joomla which was using these newer version SQL files.

@richard67
Copy link
Member

PR is #36475 (I have to fix the title in a minute). Please test.

@brianteeman
Copy link
Contributor Author

@nikosdion I understand exactly what Richard is saying. My point (which is kind of off topic) is that I have repeatedly been told that the content of those update scripts is NOT immutable and there are regular pull requests which change the content of those scripts

@richard67
Copy link
Member

The content is not immutable because for example if we had added an index in past and later had removed it, it was not possible to add back that index with the same name, and there is no alter index rename command, what a pity.

So there were cases in past when we HAD TO comment out lines in old update SQL scripts.

I have explained that so often that I am sick of it.

We can agree that it would be good if they were immutable.

But with our current database checker that simply is not possible.

And again, all that has absolutely nothing to do with what I am doing with my PR.

@brianteeman
Copy link
Contributor Author

this pr should not have been merged without this being changed and hopefully the people who have merge access are more careful to check for this in the future.

Yes the immutable comment is off-topic but important. So it is interesting that @nikosdion says the content is immutable and you say the opposite

@richard67
Copy link
Member

You can find lots of examples in J3 when we had to disable statements in old update SQL scripts:

richard@vmubu01:~/lamp/public_html/joomla-cms-3.10-dev/administrator/components/com_admin/sql/updates$ find ./ -type f -name "*\.sql" -exec grep -Hn "\-- The following" {} \;
./sqlazure/3.4.0-2014-09-16.sql:3:-- The following statement has to be disabled because it conflicts with
./sqlazure/3.5.0-2016-03-01.sql:5:-- The following statement had to be modified for 3.6.0 by removing the
./postgresql/3.4.0-2014-09-16.sql:3:-- The following statement has to be disabled because it conflicts with
./postgresql/3.6.3-2016-10-04.sql:4:-- The following statement has to be disabled because it conflicts with
./postgresql/3.7.0-2017-01-09.sql:4:-- The following statement has to be disabled because it conflicts with
./postgresql/3.1.0.sql:4:-- The following statement has to be disabled because it conflicts with
./postgresql/3.1.0.sql:9:-- The following statement has to be disabled because it conflicts with
./postgresql/3.7.0-2017-01-08.sql:5:-- The following statements have to be disabled because they conflict with
./mysql/3.4.0-2014-09-16.sql:3:-- The following statement has to be disabled because it conflicts with
./mysql/3.5.0-2016-03-01.sql:5:-- The following statement had to be modified for 3.6.0 by removing the
./mysql/2.5.0-2011-12-24.sql:4:-- The following statment had to be modified for utf8mb4 in Joomla! 3.5.1, changing
richard@vmubu01:~/lamp/public_html/joomla-cms-3.10-dev/administrator/components/com_admin/sql/updates$

@richard67
Copy link
Member

@brianteeman It is interesting that after all those years now in which I have proven that I never have broken any update or other SQL stuff with my activities but only fixed things which others had broken, you still don't trust me and talk with me as if I was somebody who has no idea about all that stuff or who always messes up things.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Dec 29, 2021

@richard67 a simple Github action could be the answer here. Check if the PR has any new update SQL files and rename them to the current date. Let computers deal with it instead of us fighting over and over (this is the second time we got the dates mismatch in the last 4 weeks)...

@brianteeman
Copy link
Contributor Author

Richard you are misunderstanding me

@richard67
Copy link
Member

@richard67 a simple Github action could be the answer here. Check if the PR has any new update SQL files and rename them to the current date. Let computers deal with it instead of us fighting over and over (this is the second time we got the dates mismatch in the last 4 weeks)...

@dgrammatiko Yes, that's a good idea.

@nikosdion
Copy link
Contributor

@richard67 @dgrammatiko I would be all for making that the default workflow on merging PRs.

@richard67
Copy link
Member

@dgrammatiko @nikosdion Sometimes PRs were rebased e.g. from 4.0-dev to 4.1-dev, so it would be good to adjust not only the date but also the version part of the file name. But that should not be a problem.

Version should be taken from target branch of the merge, and date should be the actual date of the merge plus a check if there is already a file with that name and adding a day until no file found, or something like that. Sometimes maintainers are not lazy and merge more than one PR at a day 😄 , and sometimes more than one of these PRs adds an update SQL.

Another thing is that sometimes PRs add more than one because they are new features e.g. from GSoC or other SoC projects and during review nobody has said "Please combine the scripts into one". Maybe that should be handled, too, somehow.

@richard67
Copy link
Member

@dgrammatiko Maybe that will be my next project. Currently I'm working on automation of the deleted files and folders lists updates, and am close to finishing it.

@infograf768
Copy link
Member

There are useless escape characters before single quotes in 3 strings.

administrator/language/en-GB/plg_system_jooa11y.ini:63: PLG_SYSTEM_JOOA11Y_PANEL_STATUS_12="The item you are trying to view is not visible; it may be hidden or inside of an accordion or tab component. Here\'s a preview="
administrator/language/en-GB/plg_system_jooa11y.ini:83: PLG_SYSTEM_JOOA11Y_LINK_URL="Longer, less intelligible URLs used as link text might be difficult to listen to with assistive technology. In most cases, it is better to use human-readable text instead of the URL. Short URLs (such as a site\'s homepage) are okay."
administrator/language/en-GB/plg_system_jooa11y.ini:91: PLG_SYSTEM_JOOA11Y_NEW_TAB_WARNING="Link opens in a new tab or window without warning. Doing so can be disorienting, especially for people who have difficulty perceiving visual content. Secondly, it is not always a good practice to control someone\'s experience or make decisions for them. Indicate that the link opens in a new window within the link text."

@infograf768
Copy link
Member

PR here

#37025


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

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.