XWIKI-22260: WCAG automatic testing for modals #3293
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Added wcag validation to modal elements. * Fixed a code style inconsistency in the BasePage wcag validation
surli
reviewed
Aug 8, 2024
| if (wcagContext.shouldWCAGStopOnError()) { | ||
| throw e; | ||
| } else { | ||
| LOGGER.debug("Error during WCAG execution, but ignored thanks to wcagStopOnError flag: ", e); |
There was a problem hiding this comment.
Better be an info than a debug here no? We don't enable debug often, and it could be useful to still see those, or could that be an issue on the CI?
There was a problem hiding this comment.
This is in line with
surli
reviewed
Aug 8, 2024
| @@ -51,6 +59,9 @@ public BaseModal(By selector) | |||
| String className = this.container.getAttribute("class"); | |||
| className = className.replace("fade", ""); | |||
| getDriver().executeScript("arguments[0].setAttribute(\"class\",arguments[1])", this.container, className); | |||
| // Once we're sure the modal is loaded, we can validate its content in regards to accessibility | |||
| className = this.container.getAttribute("class").split(" ")[0]; | |||
| validateWCAG(getUtil().getWCAGUtils().getWCAGContext(), true, "." + className); | |||
There was a problem hiding this comment.
Feels dangerous to rely on the className only: what if you have a page with 3 modals having the class modal, and only one is loaded? Right now the validation will be executed on the 3 modals AFAIU.
Feels like you could artificially add a new specific class (pretty much like we remove the fade class) just to identify the actual modal.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira URL
https://jira.xwiki.org/browse/XWIKI-22260
Changes
Description
Clarifications
WCAGUtils, but it doesn't share much scope with the page objects, so I scrapped it. Then I thought ofBaseElement, which is the parent of all the elements we would want. This looked pretty good, but I wasn't sure of the impact on performance adding a quite large method and dependencies to all the objects would have (I got a look at the class, it's pretty empty as of now, I'm not sure adding something specific to WCAG testing was correct). At last, I thought of having a new interface for
WCAG analyzable objects, IMO this is the best alternative, but I don't know in what module I'd include it in... Any of your opinion or help is welcome. I'm certain duplicating code like done now is not good for maintanability and I'd want to avoid it.Screenshots & Video
None, test changes only.
Executed Tests
I ran an emptied out version of flamingo skin docker tests, which only had the
XAR export IT(few tests with instantiations of modals) with the command:Without the changes in this PR, I had only 306 WCAG rule checks. With the changes in the PR, I have 361. When looking at the logs with the right parameters, I could see that as expected, only two modals got checked. All the other ones did not get checked because already in the cache. 55 rules checked seems like the order of magnitude I'd expect for two checks on a part of a page.
Out of the 55 new rule checks, 2 were marked as incomplete (automated testing alone couldn't deliver a proper answer) and all the others were passed succesfully. These numbers are in line with the repartition of current rule check results. Importantly, no rule check was failed.
I did not test everything to make sure none of the new checks failed. See the first item in the clarifications section above for more info about the strategies we could go with onwards.
Expected merging strategy