the_g_bomb → created an issue. See original summary → .
I changed it, same reasons as yours (hills etc)
As an FYI, this is what it looks like, which I don't think looks that clunky as it isn't duplicated.
Hey, I went with that to make it obvious the user had to go to a different site then and they would know where to go later. This might be their first introduction to Open AI or Anthropic and they may not know the URLs.
It is also possible that they know the tools but don't know where to get the API Keys.
Great idea, patches welcome, if you have the time.
the_g_bomb → created an issue.
Should this have gone into the 2.x branch as well?
Using the new service parameter also change the patch to make the module only available to Drupal 10.1 and above.
We can backport a different solution for earlier versions if required.
For the backport I would think we should copy the current core setting which is 200000 and set 'Lax' by default.
I have removed the additional config for samesite and lifetime settings, but grabbed those values from core and added them to the JS settings.
I left the cookie secure setting default to FALSE just in case sites don't have an SSL cert. I am also not sure what will happen if that is on without one, so thought it best to leave it as False as turning it on is easier than potentially debugging an error if it is on and causes problems.
Thank you, I have responded
the_g_bomb → created an issue.
Yeah, I did think there would be a reason for using a slimmed-down profile, but then as the a11y tests are testing the user experience rather than specific functions or features, using a controlled environment might not test what users actually experience.
In fact, there is a case that freshly installed Drupal may not have enough things in place to ensure everything is accessible. e.g. the homepage list of articles with pagination.
To be honest, I suspect we may want each profile to have its own set of tests. I would love to see umami_demo and the drupal cms out-of-the-box experience covered by some a11y tests.
Thanks for the feedback, looking forward to seeing how you get on with PHPUnit.
Thank you
I have now sent contact form messages to the maintainers who appear to have been active in the past 60 days based on recent commits.
Sorry, I completely forgot I opened this, I opened it and thought I would see if the maintainers responded, they haven't so I will procede and use any of the contact forms to see if I can get a response.
Thanks for the reminder
FYI, I think it would be a good idea to move the existing Nightwatch tests to either Playwright or PHPUnit as appropriate when they are available. Just keen to get the current tests working as optimally as possible in the meantime.
FYI, I think it would be a good idea to move the existing Nightwatch tests to either Playwright or PHPUnit as appropriate when they are available. Just keen to get the current tests working as optimally as possible in the meantime.
I think there is some work required in core/modules/user/src/Form/UserMultipleCancelConfirm.php as well.
I suspect this should be solved by: ✨ Expand the a11y nightwatch coverage in Core Active
In that PR Standard has been used, and as you suggest more Axe tests fail and as such those tests have had to be skipped for now.
the_g_bomb → created an issue.
Can I check, would moving to Playwirght solve the issue outlined here: 🐛 Impossible to run only Nightwatch tests in a given directory (f.e. for contrib modules) Needs work ?
If you think about what we are dealing with here.
There is core it has been forked at a point in time to make the fork repo and branch for this issue.
Since that point core has progressed, this repo and branch are still stuck in time, back when the repo was cloned.
To bring this up to date we need to get core 11.x up to date, we need to push that to the 11,.x version in this repo and then probably also need to merge the changes in this branch too.
If you look at the forked repo you may notice it says
`Forked from project / drupal
223 commits behind the upstream repository.` I think that refers to the default branch.
If you have permission there is usually a button you can click to update the repo, or sync the repo, which is probably the easiest, assuming you have the 11.x branch in your forked, that is probably easiest.
You can also manually do it with git.
Which I can imagine might look something like:
git checkout 11.x
git pull
git push drupal-3485202/11.x
git checkout 3485202-change-the-dialog
git merge 11.x
git push drupal-3485202/3485202-change-the-dialog
That should make your forked repo up to date with the current core as it is now, all the latest code will be in 11.x and your branch, so only the changes you are introducing should be shown in the MR.
Updating to use latest branch
I like this article about when to use alt text and when there is enough information nearby that it perhaps can create duplicate content.
https://webaim.org/techniques/alttext/
Also, there should be rules to ensure that if an image is being used to display text, the text should be conveyed in the alt text as well.
¯\_(ツ)_/¯ Sometimes the tests don't work until you retry them.
This was highlighted later as a follow up for contrast checking in Google Docs which is very useful:
https://gist.github.com/adamchaboryk/0dd2529ea9b8480f21905aaf11d1b5a5
As this issue was raised as a follow up to #2854884: Not able to disable account cancel confirm e-mail → , which looks like it was fixed in #2854252: User forms broken for admin without 'administer users' → . Hopefully the original tests were fixed there too.
This issue provides a form that allows the default setting to be changed and used if it is set.
We will need to check how the changes to the original issue has affected the tests and if they need to be updated to use the default settings.
I think I remember that the current tests are falsely passing (without any patch). As I recall, it was a while ago, I think I found that there were tests scenarios that were never activated and as such the tests always just passed, but I could be wrong.
I think with this new setting I tried to update the tests, to check all the scenarios but found I couldn't get to the point where they could pass.
Hopefully #32 or #35 have improved that.
That latest patch doesn't include any test changes at all, I would ignore that and can only assume it is here to get the functionality added to a site without tests.
I will try to take another look.
It is just the standard installation profile, so it sets the site up as everyone else sets it up. Wasn't aware that other tests don't use that, but I suspect that without doing that we won't check the accessibility of many tools included by default.
If I can get this running, it may be worth setting up umami_demo as well.
Basically it should just run the installer using the profile at:
/profiles/standard
the_g_bomb → created an issue. See original summary → .
The failing test looks to the related to image resizing and maybe related to:
https://www.drupal.org/project/drupal/issues/3484845
🐛
[random test failure] ImageUrlProviderTest::testResize
Active
Issue for the missing region test:
🐛
Address Primary tabs "missing region" A11y test failure
Active
Issue for the duplicate ID:
🐛
Address A11y test fails in search/node
Active
Although these are also related:
🐛
Accessibility: skipped heading level on for search "no results" text
Fixed
&
🐛
Cached forms can have duplicate HTML IDs, which disrupts accessible form labels
Needs work
Suggest first step is to use 'standard' install profile rather than the limited 'nightwatch_a11y_testing', even if we have to skip tests until the issues are fixed.
@smustgrave, it is curious as the test failure is now somewhere else, that seems unrelated to the changes. I am wondering if the fact that the test is now testing the correct bit, the issue is that there are problems with the article.
Looks like responsive images, if I am reading the failures correctly. I wonder if the limited profile that is enabled for the a11y tests doesn't have enough tools to do the job of setting up the article node as required.
There will also need to be follow up tickets to fix all the failures that led to the need for the disabled tests and the limited profile, so we can get to a place where everything passes and the full standard installation is tested.
Could do with a confirmation and a decision on which regex to use
Also using:
composer config --json --merge extra.patches.drupal/core '{"Issue #3323574: Fixing regex": "https://git.drupalcode.org/project/drupal/-/merge_requests/7485.diff"}'
The view still works and shows the articles with test in the name, even though the ?page=4 remains.
Using:
composer config --json --merge extra.patches.drupal/core '{"Issue #3323574: Fixing regex": "https://git.drupalcode.org/project/drupal/-/merge_requests/9355.diff"}'
The view works and shows the articles with test in the name, even though the ?page=4 remains.
After the comment in #34 by codebymikey. I have re-tested 11.x branch and created the view as described.
Using the pager works fine as the pages also use ajax.
The issue still exists if you visit the page:
/test?page=4 and then using the title filter. The page=4 remains after the filtering by ajax happens and so there is no results.
Seconded, patch applies
Used updated 11.x branch to create a new MR
the_g_bomb → changed the visibility of the branch 3501457-incorrect-path-used to hidden.
Also discussed was an appropriate translation for: ein vorschlag zur güte
the_g_bomb → created an issue. See original summary → .
I have also created: ✨ Expand the a11y nightwatch coverage in Core Active to pickup the rest of the coverage
the_g_bomb → created an issue.
I created this issue and fixed the path.
🐛
Incorrect path used in a A11y Test Admin
Active
the_g_bomb → created an issue.
the_g_bomb → changed the visibility of the branch 3484023-fix-cspell to hidden.
Thanks for the reviews
Certainly, that create article path looks wrong.
Sorry for confusion, that is correct.
Actually, this could be fixed easily in Drupal CMS as there are no tags on the homepage. So an override in the drupal_cms_olivero would sort that one out, but I do think this needs to be fixed in core for articles. Unfortunately the page variable isn't available in the tags field template, so my current solution doesn't work.
Homepage
h1 page title
- h2 node title
- h3 tags
- h2 node title
- h3 tags
- h2 node title
- h3 tags
Article
h1 node title
- h3 tags
Home in core has /node as the default homepage content, so h1 would be returned, which is fine.
h2 will (among others) be all the node titles and the tags should be h3 associated with the article node title. Technically everything is fine as it is for listing pages.
The problems currently appear on the article node pages. The node title is h1, and then the tags jump to h3.
Keeping the h3 on the homepage and listing pages would keep the tags hierarchically associated with the related h2 title, and so would be semantically better.
Isn't the site name usually the h1 on the homepage?
Do we need to check if the color: var(--color-text-neutral-soft) is inadequate in Drupal CMS, does that mean it is inadequate in claro itself, which means this issue should be moved to the Claro issue queue? Are there other places
I have followed the white rabbit on this one, and can see that neutral-soft is:
var(--color--gray-45); which itself is:
hsl(var(--color--gray-hue), var(--color--gray-saturation), 40%); (I love that gray-45 is 44% saturation, was it 45% at one point, or just a rounding thing?)
The thought was that I could find another variable colour to use instead. I suspected that that adjacent value, --color-text-neutral-medium, would work, but choosing that makes it the same colour as the text below, which seems to have the same effect as removing the colour value all together.
While looking at the colour variables, I also spotted --color-text-primary-medium: var(--color--primary-40), which is a blueish colour (/* Blue dark */ according to the comment), I think it looks kind of nice but may make the job title pop a little too much. However that fails the requirements as does --color-text-primary-loud: var(--color--primary-30);
The other thought I had was to play around with that saturation of --color--gray-45, perhaps moving it towards 40% which may mean it needs to be renamed --color--gray-40 instead, but could have more of a positive effect by darkening it enough to meet the contrast ratio required. Unfortunately we need to go all the way 30% to get a ratio that passes, which is actually closer to the --color--gray-20 than it is to original --color--gray-45. If we were to choose --color--gray-20 we would be back using --color-text-neutral-medium and that is like removing the color from the field in any case. Maybe we add a --color--gray-30... there is a --color--gray-60 and a --color--gray-65, so that's not out of the realm of impossibility, but it would need to be fixed in Claro as that is where the variables are.
The 2 options would be to flip this over to the Claro issue queue to get:
--color--gray-30: hsl(var(--color--gray-hue), var(--color--gray-saturation), 30%);
added and then either get the color-text-neutral-soft changed to:
--color-text-neutral-soft: var(--color--gray-30);
or just change the color in the MR here to:
color: var(--color--gray-30);
That being said having changed it locally, the difference between -color--gray-20 and -color--gray-30 in this context looks negligible. I would just remove it.
Updating the other meeting time to also match the Drupal Association calendar
Updating the time of the offset meeting to match the time listed in the Drupal Association Calendar.
Also adding a link to the Drupal Association calendar so people can add it to their personal calendars.
I agree the scope has changed somewhat.
I retested the issue after comment #17 and was not able to reproduce the issue with the latest 2.0.4 release using a keyboard only. I haven't been able to test with no keyboard, no mouse movements except a submit button click as my testing screen doesn't support that, I'll try on a larger screen asap.
I will also try to test the MR additions. Thank you
Confirmed, perhaps ddev updated it's library or something...
I am able to upload animated gifs in the latest version of Drupal CMS.
Thanks for the link to the comment. I see where you are coming from and agree that on a personal level, skipping the heading may not have that great an impact.
Unfortunately automated checkers are looking for it and so we are shipping Drupal CMS with something that will be flagged as an error on all blog posts.
On the homepage, H2 won't work across the board as users using headings navigate will get all the tags marked as the same importance as the headings rather than as a child of the article. Those tags without the context of which article it refers to, will be meaningless.
The other option would be to add a if page check to the template and use h3 on lists like the homepage, and h2 on the article itself.
Div it is
I changed it to an h2, which works for the node page in terms of hierarchy but it doesn't work for the home page where the node__title is an h2.
I have changed it to be span.
Updated the Steps to replicate to include Core intructions.
Whether to use a heading to label the tags section is debatable. If it is considered content and part of the content section, then that can be one argument. If it is separate from the content, then the section should be labelled in some way; a heading would make the most sense.
I do know that labelling sections is still important as is the heading hierarchy to define document structure.
the_g_bomb → created an issue.
Adding a A11y tags
As per the linked comment, I have added a check for the last added composer dependency. I picked a class from the username_enumeration_prevention module, the last module composer requires.
the_g_bomb → changed the visibility of the branch 3143862-add-a-check to hidden.
the_g_bomb → made their first commit to this issue’s fork.
This can be tested in Drupal 10 by using the following:
mkdir guardr-test-site && cd guardr-test-site
ddev config --project-type=drupal10 --docroot=web
ddev composer create drupal/recommended-project:^10
ddev composer require drush/drush
ddev composer config minimum-stability dev
ddev composer config prefer-stable true
ddev composer config repositories.guardr vcs git@git.drupal.org:issue/guardr-3231491.git
ddev composer require drupal/guardr:dev-3231491-create-9.x-release
ddev drush site:install guardr --account-name=admin --account-pass=admin -y
ddev launch $(ddev drush uli)
the_g_bomb → created an issue.
I have:
- Updated the fork with the latest changes.
- Created a new branch from the new target 2.x branch
- Update the MR with the fixes from 3308456-30.patch #30
So, what's left :
Force HttpOnly to false and remove it from the admin form
the_g_bomb → made their first commit to this issue’s fork.
Thanks again.
My approach for contrib modules is that patches often have to be applied to in-use modules. If several patches change things like version requirements or PHPCS errors, then it can prove tricky to get the correct cocktail of patches to apply in composer.
For example, an unneeded version change has been applied to 📌 username_enumeration_prevention compatiblity Needs work , which means this patch and that one cannot both be applied to the module without extra work.
If we keep the scope tight we might limit the overlap, especially if the patches aren't being merged in a timely manner. Lets hope this one goes through quickly.
We should keep the solutions specific to the issue scope. PHPCS issues should be fixed in an issue of its own.
drush site-install is an alias of site:install (https://www.drush.org/13.x/commands/site_install/) so either would work, but you have highlighted an issue in the instructions. drush cannot be called where you changed it as drush has not yet been installed.
I'll further change the instructions.