Account created on 21 April 2009, almost 16 years ago
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom the_g_bomb

I changed it, same reasons as yours (hills etc)

🇬🇧United Kingdom the_g_bomb

As an FYI, this is what it looks like, which I don't think looks that clunky as it isn't duplicated.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

Great idea, patches welcome, if you have the time.

🇬🇧United Kingdom the_g_bomb

Should this have gone into the 2.x branch as well?

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

Thank you, I have responded

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

I think there is some work required in core/modules/user/src/Form/UserMultipleCancelConfirm.php as well.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

Updating to use latest branch

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

¯\_(ツ)_/¯ Sometimes the tests don't work until you retry them.

🇬🇧United Kingdom the_g_bomb

This was highlighted later as a follow up for contrast checking in Google Docs which is very useful:
https://gist.github.com/adamchaboryk/0dd2529ea9b8480f21905aaf11d1b5a5

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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

🇬🇧United Kingdom the_g_bomb

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

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

@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.

🇬🇧United Kingdom the_g_bomb

Could do with a confirmation and a decision on which regex to use

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

Used updated 11.x branch to create a new MR

🇬🇧United Kingdom the_g_bomb

the_g_bomb changed the visibility of the branch 3501457-incorrect-path-used to hidden.

🇬🇧United Kingdom the_g_bomb

Also discussed was an appropriate translation for: ein vorschlag zur güte

🇬🇧United Kingdom the_g_bomb

the_g_bomb changed the visibility of the branch 3484023-fix-cspell to hidden.

🇬🇧United Kingdom the_g_bomb

Certainly, that create article path looks wrong.

🇬🇧United Kingdom the_g_bomb

Sorry for confusion, that is correct.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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
🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

Isn't the site name usually the h1 on the homepage?

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

Updating the other meeting time to also match the Drupal Association calendar

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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

🇬🇧United Kingdom the_g_bomb

Confirmed, perhaps ddev updated it's library or something...

I am able to upload animated gifs in the latest version of Drupal CMS.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

Updated the Steps to replicate to include Core intructions.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

the_g_bomb changed the visibility of the branch 3143862-add-a-check to hidden.

🇬🇧United Kingdom the_g_bomb

the_g_bomb made their first commit to this issue’s fork.

🇬🇧United Kingdom the_g_bomb

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)
🇬🇧United Kingdom the_g_bomb

I have:

  1. Updated the fork with the latest changes.
  2. Created a new branch from the new target 2.x branch
  3. 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

🇬🇧United Kingdom the_g_bomb

the_g_bomb made their first commit to this issue’s fork.

🇬🇧United Kingdom the_g_bomb

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.

🇬🇧United Kingdom the_g_bomb

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.

Production build 0.71.5 2024