Problem/Motivation
By @finnsky in #3391723: [PLAN] Accessibility review for new Navigation bar:
We could add Nightwatch tests because we have automated Nightwatch a11y tests in core: #3293469: Automated A11y tests in Nightwatch. Added simple tests using it.
How to start with Nightwatch testing can be found in `core/tests/README.md` file from Drupal core.
This issue should be focused on implementing a first simple test, like "(If logo) it takes you to the homepage" and define the structure the rest of the tests will follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 3393400-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3393400
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3393400-implement-nightwatch-tests
changes, plain diff MR !7980
1 hidden branch
Issue fork navigation-3393400
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
finnsky commentedWe also need:
1. Own testing tag EG: '@tags': ['navigation'],
2. Add tests in different themes contexts. Umami / Claro / Olivero / NO_THEME
Like was done https://git.drupalcode.org/project/navigation/-/merge_requests/79/diffs
Comment #4
bnjmnmThe
nightwatchMR 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.
Comment #6
finnsky commentedAlso creating some `dark` testing theme could be useful here.
Like:
body { background-color: black; color: white; }
Comment #7
ckrinaI wonder if #3402901: Toolbar doesn't indicate active menu trail for pages not included in Toolbar should be a classic test or a Nightwatch one.
Comment #11
finnsky commentedAdded a11y test and expand/collapse test.
Ally reports now
We need to add tooltip tests and mobile. basically will be fine
Comment #12
finnsky commentedIdk seems CI runned somehow. Need to check its context.
Comment #13
finnsky commentedComment #14
ckrinaIdeas on things that we could test:
Comment #15
finnsky commentedComment #16
ckrinaWe've discussed this on several occasions and we've agreed on the need to implement these tests. So changing this from "evaluate" to "implement".
Comment #17
ckrinaComment #18
ckrinaComment #21
mglamanStarted 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
Comment #22
mglamanWe 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 enoughwaitForElementVisiblepasses. Probably because the latter is inspecting the DOM and not interacting with it.When I made these changes, the tests passed:
I also needed to fix the footer's z-index in #3446340: Fix z-index . Once the wrapper adjustments were made, the footer was hidden behind the navigation.
Comment #23
mglamanOkay, I worked backward and added back styles. It is the
display: contentsComment #24
mglamanFrom caniuse https://caniuse.com/?search=display%3A%20contents
Buttons are not accessible with display: contents applied. See issues for Chromium, Firefox, and WebKit
Comment #25
mglamanI guess this is blocked on #3446357: Fix overflow visibility for wrapper content in navigation CSS, then.
Comment #27
mglamanThe test is green, but:
- cherry-pick fix from #3446357: Fix overflow visibility for wrapper content in navigation CSS
- reverts #3446340: Fix z-index
Comment #28
mglamanComment #29
mglamanAlso pushed the axe test. It is failing. I think we should:
- Merge #3446357: Fix overflow visibility for wrapper content in navigation CSS 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
Comment #30
finnsky commentedI've added special tag for module for :
`yarn test:nightwatch --tag navigation`
Comment #31
finnsky commentedI've removed initial titles.
Now they are not defined.
Only block level titles we have now.
Backender review required here.
Test passed now.
Comment #32
finnsky commentedI think we can merge it and create META for other tests.
Comment #33
needs-review-queue-bot commentedThe 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.
Comment #34
nod_css fix in #3446357: Fix overflow visibility for wrapper content in navigation CSS committed
Comment #35
finnsky commentedComment #36
smustgrave commentedLeft a comment on MR. Should be small fix.
Comment #37
ckrinaWe just defined an strategy to keep titles for the menus and avoid the problems with cache.
Comment #39
m4oliveiUpdated the merge request to address the
<h4>issue. See MR comment for details.Comment #40
plopescAdded 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!
Comment #41
m4oliveiAddressed comments from @plopesc. Ready for review.
Comment #42
finnsky commentedLooks good to me. Also i've removed outdated classnames. they are only should be on block level now.
Comment #43
plopescThank you for the progress here!
I'm missing these 2 small tasks from the strategy defined in #37
Other than that, I think this is good to go!
Comment #44
finnsky commentedComment #45
finnsky commentedFollow up https://www.drupal.org/project/drupal/issues/3452724
Comment #46
m4oliveiThis is looking good to me! Items in #43 have been addressed. Tests are passing and making good sense to me.
Comment #47
finnsky commentedSorry, keep in in rtbc. we also had that heading ID in footer.
Comment #48
nod_few comments
Comment #49
plopescThese changes are not affecting the code and tests are still green, so I'm moving it back to RTBC directly.
Thank you
Comment #50
nod_committed followup, we can clean up this MR
Comment #51
plopescBack to RTBC once #3455058: drupalInstallModule nightwatch function does not work with Experimental modules was merged and the module installation process simplified.
Comment #52
nod_Thanks
Comment #53
nod_Comment #58
nod_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!