🇺🇸United States @kentr

Invalid ARIA attributes in menu (WCAG SC 4.1.2)

The menu has eight errors on ARIA attributes that are failures of WCAG SC 4.1.2 - Name, Role, Value (Level A).

Attached an Accessibility Insights FastPass report and screenshot with errors highlighted.

Steps to reproduce

1. Open the new home page.
2. Run a test with the Axe or Accessibility Insights browser extensions.

I suspect the pipeline failures are related to the changes. I put details in comments on the MR.

Based on the comments, esp #30 and #37, I wonder if this should be Normal priority rather than Major.

@rinku jacob 13, 📌 Potentially remove -ms-high-contrast Needs work is probably the correct issue for your changes.

Updated the title and IS to reflect that the code must be converted to use forced-colors rather than removed.

RE @longwave's comment, #6:

I suggest converting background image cases to mask-image or mask using the original (full-color) image and using the appropriate system-color as the background.

There are many examples of the technique in Claro. I linked to a couple in the IS.

What's the status of this issue?

Claro has overrides for these. If they are only fixed in the toolbar module, the changes won't appear in Claro.

Perhaps the correct solution involves removing the Claro overrides.

MR needs review

The Firefox prefers-color-scheme emulation doesn't work like I thought, so disregard point 1 in #8 🐛 FF & Chrome "dialog close" icon has poor contrast in WHCM + White theme Active . The icon appears with good contrast when the text / background colors are changed manually in the color settings.

However, I see the problem in dark themes on Edge Mac with the Page Colors setting.

It looks like dark theme forced-colors mode worked in Chrome as of 🐛 Dialog close icon not reliably visible in forced colors mode Fixed . I don't know what is different now.

Attaching screenshots of the problem in Edge, and in Edge & Chrome after the change in the MR.

Problem in Edge with Page Colors setting

After the changes in the MR

Edge with Page Colors setting

Chrome, forced-colors, dark

I see that the "x" is not appearing at all in these scenarios:

1. Firefox Mac, forced-colors enabled, prefers-color-scheme: dark simulation.
   
2. Chrome Mac, forced-colors enabled, prefers-color-scheme: dark simulation.
   

I'm going to work on the issue.

I'm also only finding this one instance in Claro of an inline SVG. Maybe it would be better to convert it to a background image for consistency.

Ultimately, it's up to the maintainers.

@sandip poddar:

It should work... That said, my proposal was biased because I had been working on an issue where a media query inside the svg didn't work.

Your proposal is simpler and less invasive, and I've also learned from @Curtis Wilcox in Slack that:

• in this case the role is a button and the color should be ButtonText
• because this is an inline SVG, using currentColor will allow the colors to inherit from the parent button which will have the correct color in forced-colors mode

I updated the IS proposed resolution based on the suggestions from you and @Curtis Wilcox.

Is this what remains to be done?

1. Apply changes in patch from #68 to 11.x
2. Incorporate review comments in #82
3. Create an MR

Forgot to change status / remove IS update.

Updated the IS, included testing notes, and attached the screenshots that didn't save for #57.

I'm working on the IS update.

kentr → created an issue.

I believe this is outdated.

kentr → changed the visibility of the branch 3171726-claro-shortcuts-star to hidden.

kentr → changed the visibility of the branch issue-3171726 to hidden.

There were merge conflicts when updating the existing MR branch, so here's a new MR against 11.x.

Some comments

1. I went back to the non-concentric fill for the "remove" states as specified by @ckrina's comment #16.
2. For the "add, hover" state, the previous MR had a solid fill for forced-colors: active.
   That wasn't in @ckrina's screenshots, and due to the + above the star in the Claro icons I wondered if it is necessary. So I left the fill empty for now as shown in the screenshots.
3. Inside the CSS url() function, the SVG with a forced-colors: active media query embedded in a style element did not display the LinkText color as expected.
   However, there are many instances in core of using mask-image for forced-colors: active, so I went with that method.
4. For the time being, I've put the same icons into both Claro and the shortcut module.
   I suggest creating a lower-priority followup to refine the shortcut module's icons since they'll appear on Olivero (which has different blues).

These screenshots are from Mac Firefox in standard mode and with browser.display.document_color_use set to 2.

Needs manual testing with & without forced-colors / WHCM. I will update the IS with details.

I'm going to try to move the MR forward a little.

Adding Needs issue summary update because the IS mentions IE11 and AFAIK Drupal dropped support for IE11.

I'm suggesting changing to WCAG 2.2. The Accessibility page under Drupal Features → says this, which implies that Drupal the software is expected to conform to WCAG 2.2:

The Drupal Accessibility Team and project governance will follow the latest recommended release of the WCAG guidelines (WCAG 2.2 AA).

@laurii, does that indicate that a decision on what we are striving for has been made? If not, who makes the decision?

I'm getting contrast errors for the "Apply to selected items" button in the Axe and Wave / WebAIM extensions.

I'm attaching screenshots of the errors.

I thought it might be due to the smaller font size, but a quick experiment with the regular font size didn't remove the error in Axe.

Is this resolved by the PHPCS and PHPSTAN checks in Gitlab?

Regarding the accessibility aspects of W3C validation, there is some overlap with ✨ Automate Accessibility Checks for Core Active

Specific implementations from that issue are in 📌 Automated A11y tests in Nightwatch Fixed , 📌 Automated A11y tests in PHPUnit Needs work .

Not currently working on this.

The breadcrumb is covered by 🐛 Mobile friendly breadcrumb (Lighthouse) Needs work .

The Lighthouse accessibility audit in Chrome DevTools covers this (WCAG SC 2.5.8).

The automated Axe tests will cover this, but we would need to explicitly enable (or really, enable WCAG 2.2 AA). The test for 🐛 Mobile friendly breadcrumb (Lighthouse) Needs work can serve as a starting point.

MR 11394 is against 11.x and includes a Nightwatch test.

Based on discussion in 🐛 [meta] Some interface components don’t meet the minimum target size Active , I reduced the height of the elements to 1.5rem, which comes to 24px.

I am attaching new screenshots with the changes, as they are different than the previous version.

Tagging for https://www.w3.org/WAI/WCAG22/Understanding/target-size-minimum.html

kentr → changed the visibility of the branch 3223147-mobile-friendly-breadcrumb-fix to hidden.

kentr → changed the visibility of the branch 3223147-mobile-friendly-breadcrumb to hidden.

Sorry, I mistook this page on Drupal accessibility → as the core gate.

That page says:

The Drupal Accessibility Team and project governance will follow the latest recommended release of the WCAG guidelines (WCAG 2.2 AA).

How about calling the page "Accessibility Requirements" instead of "Accessibility Coding Standards"? They are more akin to functional requirements.

Agree with @quietone in #9 regarding linking from the Accessibility core gate to here (or vice-versa):

Technical Standards - This is repeating information on Accessibility core gate. Should we change the core gate document to link to this new section?

Currently, the pages are out of sync. This document states WCAG 2.1 AA, Accessibility core gate states WCAG 2.2 AA.

I am able to reproduce on 11.x with different STR. I updated the IS and am attaching the HTML Lighthouse report.

I think I can fashion a failing Nightwatch Axe test.

The Axe rule currently referenced by Lighthouse now specifies 24x24 px instead of 48x48 px, but I didn't change that in the IS because I don't know the history.

On a meta-level, in case it is a policy problem:

This issue is WCAG 2.2 AA. A Nightwatch Axe-core test just for this issue will need to explicitly invoke WCAG 2.2 AA, because the existing automated Axe tests don't appear to run WCAG 2.2 AA. So, the standard for this test will differ from the rest unless all tests are updated.

The Accessibility Coding Standards are unclear IMO on whether core / Drupal CMS are required to meet WCAG 2.2 AA.

Though the existing Axe tests still wouldn't have caught this for other reasons, in general they would catch some target-size problems if we started including WCAG 2.2 AA.

Noting this DDEV add-on: https://github.com/Metadrop/ddev-lighthouse

I'm curious how to get lighthouse into functional javascript tests with the existing use of Selenium.

Would the accessibility tests provided by Lighthouse offer more than accessibility tests directly using Axe-core?

I created an MR against 11.x with patch #37 and the suggestions from #40.

Removed tag Drupal 9 compatibility as D9 is EOL.

kentr → made their first commit to this issue’s fork.

Added child issue 🐛 Improve accessibility of landmark regions in toolbar Active .

kentr → created an issue.

I applied the requested change RE inlining as well as other suggestions.

@smustgrave

What's a good way to test this one

The only method I know to test it manually is:

1. Enable automated_cron.
2. Set the cron interval to a very low number > 0, such as with drush cset -y automated_cron.settings interval 1
3. After the number of seconds used for the interval, load a page on the site.
4. Confirm that cron ran without errors as a result of the page load.

According to the IS Remaining Tasks, there are other cases to change. Are there any of these that we don't want to include in this issue?

Also noticed that @wim leers said in 📌 DX: Creating lazy services is too difficult/obscure/bespoke/brittle Active :

IIRC this was critical for certain aspects of the Drupal bootstrap process to become sufficiently fast in the majority of cases.

So the work that gets done here almost certainly will need to provide profiling data if it changes lazy services, and certainly if it removes them altogether.

I don't know how to do that, but I'm game to learn.

It looks like the config for the install profile(s) is also still needed.

The IS says:

editor users on NEW sites using Standard install profile: new button will be displayed in CKEditor 5 editor

Is that meant to include Demo Umami?

We want it enabled by default in both Basic HTML and Full HTML formats, correct?

Regarding the position of the button within the toolbar, my thought is to place it in the very first position because it provides info on using the other features through the keyboard and won't be hidden by wrapping. Attaching a screenshot.

How does one reproduce this?

What needs to be reviewed on this?

According to the IS, this needs tests. I didn't see any in the MR.

I made the changes discussed in Slack and updated the IS.

Details on the logic changes are in the comments and commit messages.

Pipeline is green.

This might be fixed by 🐛 Gitlab yml is out of date Active , which upgrades yarn to 4.6.0 and playwright to 1.50.1.

I suspect this is fixed by 🐛 Gitlab yml is out of date Active . I'm not getting these errors locally in docker, and the pipeline is running except for actual test failures.

lint and audit jobs are passing.

Tests are running, but one is failing in Firefox. Can someone familiar with the tests look at it?

One of the audit errors was for eslint. Upgrading that and eslint-plugin-jsdoc as required introduced a lot of jsdoc lint warnings. Therefore, I set --max-warnings=-1 temporarily to keep the warnings from causing job failure. Someone will need to review the warnings and tweak the rules or the code.

I upgraded both the playwright and main docker images.

For the main image, I used drupalci/php-8.3-apache:production because the drupalci images are used in the gitlab_templates project.

I reasoned that there would be maintenance benefits with using the "official" images, and there might be a caching benefit in Gitlab. Also, the drupalci image already has git.

Another audit error was for rollup-plugin-terser, which minifies the build artifact a11y.autocomplete.min.js. After switching to @rollup/plugin-terser, the resulting a11y.autocomplete.min.js is slightly different than before. So, it should be checked.

The browserlist DB still needs updating.

Hid the original MR because the new MR builds on it.

kentr → changed the visibility of the branch 3338664-automated-a11y-tests to hidden.

@the_g_bomb

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.

Agreed. It sounds like phpunit is the first choice based on 📌 Consider dropping Nightwatch in favor of Functional Javascript tests Active .

I've done some work on 📌 Automated A11y tests in PHPUnit Needs work and replicated the current Nightwatch a11y tests, but hit a snag 📌 Automated A11y tests in PHPUnit Needs work .

FYI, there was some concern about using the standard profile when the nightwatch a11y tests were created: #3293469-23: Automated A11y tests in Nightwatch →

Adding to my last comment:

In case it's preferable to commit the axe javascript file into core, AFAICT it is Mozilla Public License 2.0 and is 541K minified.

I need implementation advice.

The axe core javascript is an npm package.

In a Slack discussion debugging the failed job, @fjgarlin said that core functional javascript tests don't have access to the core/node_modules directory. So, the file must be provided some other way.

It has to be built and the dist files aren't in the repo, so it can't be retrieved directly from the repo.

I didn't find any other cases like this, so I experimented with installing it locally via Composer into vendor by defining a repository in composer.json as follows and then requiring it as a Composer dev dependency. This is working locally.

Is this a viable solution, or are there other preferred solutions?

        {
            "type": "package",
            "package": {
                "name": "npmjs/axe-core",
                "version": "4.10.2",
                "dist": {
                    "url": "https://registry.npmjs.org/axe-core/-/axe-core-4.10.2.tgz",
                    "type": "tar",
                    "shasum": "85228e3e1d8b8532a27659b332e39b7fa0e022df"
                }
            }
        }

Putting back to RTBC because it was before #62.

Updated issue summary to indicate that the MR to be reviewed is !10482.

In light of #3467492-15: [policy, no patch] Replace Nightwatch with Playwright → , I'm working on this against 11.x and planning to add support for options so that we can use it like we use Nightwatch's axeRun().

🌱 [META] improve accessibility of toolbar Active covers these.

I suggest closing this issue.

These don't have their own issues yet, correct?

1. Too many ARIA group/landmark roles
2. Navigation landmark regions without accessible names
3. Empty navigation regions

#3 is a quick fix.

From looking at the template, removing the nested navs as suggested for #1 might also fix #2.

The toolbar is wrapped by an ARIA group (not a landmark) and a navigation role, each with their own label. This double wrapper doesn't serve much purpose, it merely increases verbosity.

It's also throwing this Axe error, which resolves when I change the div to nav and remove the role="group".

All page content should be contained by landmarks

Ensure all page content is contained by landmarks

Element Location: #toolbar-administration

<div id="toolbar-administration" role="group" aria-label="Site administration toolbar" data-drupal-claro-processed-toolbar="" class="toolbar claro-toolbar toolbar-oriented" data-once="toolbar toolbarAntiFlicker">

Definitely a core issue.

kentr → created an issue.

I think it would be great if a11y tests ran automatically for most paths requested by a test. Currently, it appears that Axe tests in Nightwatch are only run for explicitly-defined paths in dedicated tests.

Based on limited experience with Nightwatch, setting up test conditions appears to be easier for PHPUnit functional javascript tests than it is for Nightwatch tests.

It also appears that there are currently more functional javascript tests than Nightwatch tests (at least, going by the number of test files turned up with a quick find): 242 vs 34.

Doing the a11y tests with PHPUnit might help cast the widest net.

There are some issues sneaking into Core that should have been caught by automated testing.

Some of these missed issues may be a result of the limited nightwatch_a11y_testing profile. A bunch of Axe errors appear when I use the modules from the standard profile.

I've been digging into it a little more before I create an issue.

It looks like the Api Client module → has been created in the meantime (by @brianperry ;-).

To the general discussion here, a stable & agnostic JS client library that includes admin / content management features could also be used as an interface in Nightwatch commands for setting up a test site.

RE #25, I found these test case URLs.

• "/"
• "/admin/content"
• "/admin/structure"
• "/admin/structure/block"
• "/admin/structure/taxonomy/add"
• "/admin/structure/types/add"
• "/node/add/page?destination=/admin/content"
• "/search/node"
• "/user/1/edit" (tested with and without the navigation module
• "/user/login"

In these files:

• a11yTestDefault.js
• a11yTestAdmin.js
• a11yTest.js

I'm wondering if there's an error in the admin tests.

{ name: 'Create Article', path: '/user/1/edit' },

Should perhaps be:

{ name: 'Create Article', path: '/node/add/article?destination=/admin/content' },

I'm seeing almost identical errors without this module, so these problems may be in core. I'm looking into it further and will file core issues if needed.

Does this issue still need backport to D7?

Drupal core version 7 has reached end of life, and is no longer community supported on Drupal.org.

For me on 11.1.0 (without the patch), the timestamp column is updated on reads at the interval specified by $settings['session_write_interval'].

Details of the mechanism ( credit to @znerol for this description 🐛 Session fixation for anonymous users - discard invalid session identifiers instead of accepting them Needs work ):

1. Drupal core has the session_write_interval setting which defaults to 180 seconds.
2. That setting is used in core MetadataBag to initialize its parent.
3. The Symfony MetadataBag will update its timestamp when the update threshold is reached.
4. When that happens, session metadata is changed and SessionHandler:write() is called when the request terminates.
5. That in turn will also update the timestamp field.

Note that this also happens for read-only requests because Symfony's MetadataBag::initialize is also called when existing session data is loaded for the request.

This test verifies that timestamp is updated on reads.

Regarding logging users out at a shorter interval: for me it works as expected when I set gc_maxlifetime and cookie_lifetime, and do a cache rebuild. The existing cookie isn't cleared, though. It's only working for logins that occur after the settings are changed.

So, the suggested change should not be necessary. It also looks like it writes to the DB after every read, which would have a performance penalty.

I added an empty hook_post_update_NAME() to the automated_cron module because the new lazy service closure is on that module.

The pipeline is green, but I could only reproduce the error once locally anyway in spite of multiple tries with full wipe / reset.

Updated the issue summary.

There's an MR for review.

A few FunctionalJavascript tests errored but passed on retry.

FWIW, access to PHP files in sites/default/files is currently blocked by the root .htaccess.

With this change, the Security Review overview still states that PHP files in the Drupal files directory can be executed.

However, I've confirmed manually that they are not executed: the raw PHP source is served. It looks like a case of ambiguity in the messaging. Here's the details message:

The Drupal files directory is for user-uploaded files and by default provides some protection against a malicious user executing arbitrary PHP code against your site. Read more about the risk of PHP code execution on Drupal.org.

The .htaccess file is writable which poses a risk should a malicious user find a way to execute PHP code they could alter the .htaccess file to allow further PHP code execution.

I've attached a new PHP-FPM config to reproduce this.

NR for the change that fixed the test.

The remaining failing test was:

1) Drupal\Tests\system\Kernel\System\CronQueueTest::testDelayException
    Failed asserting that 0 matches expected 1001.
    
    /builds/issue/drupal-3483996/core/modules/system/tests/src/Kernel/System/CronQueueTest.php:137

I made a change to fix it and commented in GitLab explaining the change.

A couple of other tests failed in the subsequent automatic pipeline, but they passed on with manual reruns:

There was 1 failure:

1) Drupal\Tests\path\Functional\PathAliasTest::testPathCache
Cache entry was created.
Failed asserting that a boolean is not empty.
/builds/issue/drupal-3483996/core/modules/path/tests/src/Functional/PathAliasTest.php:84
FAILURES!
Tests: 4, Assertions: 113, Failures: 1.

There was 1 error:

1) Drupal\FunctionalJavascriptTests\Tests\JSWebAssertTest::testJsWebAssert
WebDriver\Exception\StaleElementReference: stale element reference: element
is not attached to the page document

This test passed when rerun:

    1)
    Drupal\Tests\file\Functional\DownloadTest::testPrivateFileTransferWithoutPageCache
    Correctly denied access to a file when file_test sets the header to -1.
    Failed asserting that 200 is identical to 403.
    
    /builds/issue/drupal-3483996/core/modules/file/tests/src/Functional/DownloadTest.php:138
    /builds/issue/drupal-3483996/core/modules/file/tests/src/Functional/DownloadTest.php:76

kentr → created an issue.

phpDocumentor support is similar to #45, also overriding parameter & return declarations. The layout is similar to the VS Code Intelephense version.

This code:

class A {
    /**
     * Parent method summary.
     *
     * Parent method long description.
     *
     * @param bool $input The input.
     *
     * @return bool
     */
    public function method($input = TRUE) {
        return $input;
    }
}

class B extends A {
    /**
     * {@inheritDoc}
     *
     * Child method additonal details.
     *
     * @param string $input The input.
     *
     * @return string
     */
    public function method($input = 'default') {
        return $input;
    }
}

Outputs:

Changes made.

Is this module still relevant? I find that the CKEditor5 inline code button provides this functionality.

My understanding is that updateTimestamp() is only invoked when session data is saved and the data is unmodified. This matches my observation with the traces.

So, I added trace tests for these cases:

1. The previously-stored session is read by a request with the corresponding session cookie.
   • validateId() is invoked.
   • write() is not invoked.
   • updateTimestamp() is not invoked.
2. The previously-stored session is modified by a request with the corresponding session cookie.
   • validateId() is invoked.
   • write() is invoked.
   • updateTimestamp() is not invoked.
3. The previously-stored session is re-saved as-is (unmodified) by a request with the corresponding session cookie.
   • validateId() is invoked.
   • updateTimestamp() is invoked.
   • write() is not invoked.

The last case should also apply to 📌 Use session.lazy-write once Drupal 8 only supports PHP 7 Active .

regarding {@inheritdoc}...let's leave this up to a core committer to decide.

Got it. I thought it was OK because the phpDoc documentation gives examples of providing additional details along with {@inheritdoc}.

[StackSessionHandlerIntegrationTest] doesn't cover the situation where the new methods are called from within the php session subsystem

I'll look into these tests.

Yes, you are right. A no-op implementation of updateTimestamp() needs to return TRUE.

Ok. I changed updateTimestamp() to return TRUE.

Returning FALSE from updateTimestamp() causes PHP warnings and errored tests. I believe the error originates from PHP session.c, line 521.

I pushed it so that others can observe, but I believe updateTimestamp() should return TRUE to be seen as a successful no-op.

In Watchdog:

Warning: session_write_close(): Failed to write session data with "Drupal\session_test\Session\TestSessionHandlerProxy" handler in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->Symfony\Component\HttpFoundation\Session\Storage\{closure}() (line 232 of /var/www/html/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php)

In tests:

Exception: Warning: session_write_close(): Failed to write session data with "Drupal\session_test\Session\TestSessionHandlerProxy" handler
Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->Symfony\Component\HttpFoundation\Session\Storage\{closure}()() (Line: 232)

Suggestions applied. There are a few outstanding threads.