- Issue created by @ckrina
- First commit to issue fork.
- π·πΈSerbia finnsky
We also need:
1. Own testing tag EG: '@tags': ['navigation'],
2. Add tests in different themes contexts. Umami / Claro / Olivero / NO_THEMELike was done https://git.drupalcode.org/project/navigation/-/merge_requests/79/diffs
- Status changed to Needs review
about 1 year ago 2:45pm 20 November 2023 - πΊπΈUnited States bnjmnm Ann Arbor, MI
The
nightwatch
MR has basic accessibility tests running. It's good to have these running, and this demonstrates that adding Nightwatch doesn't require any major errfor.The Axe A11y tests already caught a duplicate ID, which is now fixed in that same MR. Given that this already caught an accessibility bug, I think this basic implementation should get in sooner than later so it can surface other accessibility problems as this project evolves.
The NW testing can get more comprehensive than what is currently there if we see the need. I think beginning with something simple like this is helpful as it gets basic A11y checks going and is a starting point for additional NW tests.
- π·πΈSerbia finnsky
Also creating some `dark` testing theme could be useful here.
Like:
body { background-color: black; color: white; } - πͺπΈSpain ckrina Barcelona
I wonder if π Toolbar doesn't indicate active menu trail for pages not included in Toolbar Active should be a classic test or a Nightwatch one.
- π·πΈSerbia finnsky
finnsky β changed the visibility of the branch nightwatch to hidden.
- π·πΈSerbia finnsky
finnsky β changed the visibility of the branch 3393400-evaluate-adding-nightwatch to hidden.
- π·πΈSerbia finnsky
Added a11y test and expand/collapse test.
Ally reports now
β 1) Tests/a11y β Accessibility - Navigation Module - Claro page (10.3s) β β NightwatchAssertError aXe rule: button-name - Buttons must have discernible text In element: .toolbar-button--icon--sidebar-toggle β β NightwatchAssertError aXe rule: link-name - Links must have discernible text In element: .toolbar-button--icon--system-themes-page β β NightwatchAssertError aXe rule: link-name - Links must have discernible text In element: .toolbar-button--icon--system-modules-list β β NightwatchAssertError aXe rule: link-name - Links must have discernible text In element: .toolbar-button--icon--entity-user-collection β Accessibility - Navigation Module - HomePage (6.613s) β β NightwatchAssertError aXe rule: button-name - Buttons must have discernible text In element: .toolbar-button--icon--sidebar-toggle β β NightwatchAssertError aXe rule: heading-order - Heading levels should only increase by one In element: .label β β NightwatchAssertError aXe rule: landmark-unique - Ensures landmarks are unique In element: #admin-toolbar β β NightwatchAssertError aXe rule: link-name - Links must have discernible text In element: .toolbar-button--icon--system-themes-page β β NightwatchAssertError aXe rule: link-name - Links must have discernible text In element: .toolbar-button--icon--system-modules-list β β NightwatchAssertError aXe rule: link-name - Links must have discernible text In element: .toolbar-button--icon--entity-user-collection β β NightwatchAssertError aXe rule: region - All page content should be contained by landmarks In element: #primary-tabs-title
We need to add tooltip tests and mobile. basically will be fine
- π·πΈSerbia finnsky
hello all!
I have small misunderstanding.
We want to add Nightwatch testing. But i donβt really want to reproduce all Drupal Nightwatch infrastructure in module. Currently Iβm running tests from core dir like yarn run test:nightwatch --tag navigation
But it will be useless in Navigation module CI.So idk. Iβll continue in same approach. Probably we need to move Drupal Nightwatch functions into npm package. So it can be useful for modules/themes CI
Idk seems CI runned somehow. Need to check its context.
- Status changed to Needs work
10 months ago 6:53am 26 February 2024 - πͺπΈSpain ckrina Barcelona
Ideas on things that we could test:
- Collapse/expand the sidebar
- Scroll the sidebar (if the βcontentβ is higher than the viewport height + footer)
- Open/Close the drawer on hover (submenu level 2)
- Test it with edge cases, like moving fast to the top of the drawer - so the mouse leaves the targeted hover item
- Navigate to submenu level 3 (accordion)
- Navigate to User menu links
- See labels/tooltips of the 1st level links
- Active menu item on 1st, 2nd and 3rd level items
- (If logo) it takes you to the homepage
- Active item shows as so in the Toolbar
- πͺπΈSpain ckrina Barcelona
We've discussed this on several occasions and we've agreed on the need to implement these tests. So changing this from "evaluate" to "implement".
- First commit to issue fork.
- πΊπΈUnited States mglaman WI, USA
Started working on the tests, copied over @bnjmnm test https://git.drupalcode.org/project/drupal/-/merge_requests/7980
Experimental modules can't be installed using the install module command. since it won't be experimental for long, I just used a test setup script
- πΊπΈUnited States mglaman WI, USA
We have a problem here. The scroll wrapper,
.admin-toolbar__scroll-wrapper
, causes WebDriver to think any elements within it are not visible for interaction. Oddly enoughwaitForElementVisible
passes. Probably because the latter is inspecting the DOM and not interacting with it.When I made these changes, the tests passed:
.admin-toolbar__scroll-wrapper { /*display: flex;*/ /*overflow-x: hidden;*/ /*overflow-y: auto;*/ flex-direction: column; ... } @media (min-width: 64rem) { .admin-toolbar__scroll-wrapper { /*display: contents;*/ /*background: none;*/ } }
I also needed to fix the footer's z-index in π Fix z-index Needs review . Once the wrapper adjustments were made, the footer was hidden behind the navigation.
- πΊπΈUnited States mglaman WI, USA
Okay, I worked backward and added back styles. It is the
display: contents
@media (min-width: 64rem) { .admin-toolbar__scroll-wrapper { display: contents;
- πΊπΈUnited States mglaman WI, USA
- πΊπΈUnited States mglaman WI, USA
I guess this is blocked on π Buttons are not accessible with display: contents applied Active , then.
- πΊπΈUnited States mglaman WI, USA
mglaman β changed the visibility of the branch 3393400-nightwatch-tests to hidden.
- πΊπΈUnited States mglaman WI, USA
The test is green, but:
- cherry-pick fix from π Buttons are not accessible with display: contents applied Active
- reverts π Fix z-index Needs review - Status changed to Needs review
8 months ago 11:15pm 9 May 2024 - πΊπΈUnited States mglaman WI, USA
Also pushed the axe test. It is failing. I think we should:
- Merge π Buttons are not accessible with display: contents applied Active to apply fix
- Follow @ckrina idea of making this a meta issue
- Commit the test site install script and expandCollapseTest.js (this issue or new child?)
- Make the child issues from #14 including a11y test - π·πΈSerbia finnsky
I've added special tag for module for :
`yarn test:nightwatch --tag navigation`
- π·πΈSerbia finnsky
I've removed initial titles.
Now they are not defined.
Only block level titles we have now.Backender review required here.
Test passed now.
- π·πΈSerbia finnsky
I think we can merge it and create META for other tests.
- Status changed to Needs work
7 months ago 10:35am 20 May 2024 The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- π«π·France nod_ Lille
css fix in π Buttons are not accessible with display: contents applied Active committed
- Status changed to Needs review
7 months ago 8:38am 22 May 2024 - Status changed to Needs work
7 months ago 1:41pm 22 May 2024 - πΊπΈUnited States smustgrave
Left a comment on MR. Should be small fix.
- πͺπΈSpain ckrina Barcelona
We just defined an strategy to keep titles for the menus and avoid the problems with cache.
- First commit to issue fork.
- Status changed to Needs review
7 months ago 5:13pm 22 May 2024 - π¨π¦Canada m4olivei Grimsby, ON
Updated the merge request to address the
<h4>
issue. See MR comment for details. - Status changed to Needs work
7 months ago 9:44am 23 May 2024 - πͺπΈSpain plopesc Valladolid
Added comment suggesting a different approach to set the
<h4>
title using the original logic we had in place.
Let's decide the preferred approach and wrap this one! - Status changed to Needs review
7 months ago 12:11am 5 June 2024 - π¨π¦Canada m4olivei Grimsby, ON
Addressed comments from @plopesc. Ready for review.
- π·πΈSerbia finnsky
Looks good to me. Also i've removed outdated classnames. they are only should be on block level now.
- Status changed to Needs work
7 months ago 8:05am 5 June 2024 - πͺπΈSpain plopesc Valladolid
Thank you for the progress here!
I'm missing these 2 small tasks from the strategy defined in #37
- Remove ID from h4and aria-labelledby from ul
- Open a follow-up to add back the ID and aria-labelled-by with JS to avoid caching issues. Also in this follow-up replace the ID on the li items, r evaluate if they are needed.
Other than that, I think this is good to go!
- Status changed to Needs review
7 months ago 2:27pm 5 June 2024 - π·πΈSerbia finnsky
Follow up https://www.drupal.org/project/drupal/issues/3452724 β¨ back the ID and aria-labelled-by with JS Postponed
- Status changed to RTBC
7 months ago 2:44pm 5 June 2024 - π¨π¦Canada m4olivei Grimsby, ON
This is looking good to me! Items in #43 have been addressed. Tests are passing and making good sense to me.
- π·πΈSerbia finnsky
Sorry, keep in in rtbc. we also had that heading ID in footer.
- Status changed to Needs work
7 months ago 4:53am 9 June 2024 - Status changed to RTBC
6 months ago 12:43pm 17 June 2024 - πͺπΈSpain plopesc Valladolid
- Created follow-up issue π drupalInstallModule nightwatch function does not work with Experimental modules Active to deal with issues enabling experimental modules in Nightwatch tests
- Moved some reused selectors to constants
- Added references to followup issues in @todo comments
These changes are not affecting the code and tests are still green, so I'm moving it back to RTBC directly.
Thank you
- Status changed to Needs work
6 months ago 7:56pm 17 June 2024 - Status changed to RTBC
6 months ago 5:27am 18 June 2024 - πͺπΈSpain plopesc Valladolid
Back to RTBC once π drupalInstallModule nightwatch function does not work with Experimental modules Active was merged and the module installation process simplified.
- Status changed to Fixed
6 months ago 8:20pm 22 June 2024 - π«π·France nod_ Lille
not committing to 10.3.x because of the id removal, might backport once follow-up is in.
Committed and pushed 9e83195c5d to 11.x and 07d570cfc7 to 11.0.x and 74cd44af63 to 10.4.x. Thanks!
Automatically closed - issue fixed for 2 weeks with no activity.