Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWIKI-22580: New release input fields have no text alternative #3580

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Oct 21, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22580

Changes

Description

  • Added a default aria-label for StringProperties editDisplay.
  • Added a default aria-label for Properties in the custom edition context.
  • Used this context variable to fill up an aria-label for the date fields custom display.

Clarifications

  • In the displayEdit test from StringClassTest, somehow this change also changed the order of other parameters. I modified it to match the new result, but I have no idea why this changed and how however this is stable across runs...
  • Before reaching this solution, I tried to provide a generic fix for all Property types. However, the displayEdit function is overridden for the type which interests us (and many other interesting types). This solution only fix String properties, but a similar solution could be used to provide text alternatives for the inputs of other property types.
  • There's multiple ways to label such an input. I decided to use aria-label:
    • PROS:
    • it's a fallback that'll only be considered after all more conventional ways to label an input (implicit or explicit labelling). It does not take precedence over other ways to label the input that have already been set up (which are surely more specific and better).
    • it does not change the structure of the output. The impacts on visuals and user customizations will be naught. Very good solution for backwards' compatibility.
    • CONS:
    • It's not standard HTML, we rely on ARIA. Some media might not make full use of the info we provide in this attribute. Minor con since this is a feature primarily implemented for assistive technologies, and ATs should implement ARIA.

Screenshots & Video

After the PR, we can see that both of the fields are labelled in a generic but correct way.
image
A test with axe devtool does not find any warning on this page. Before the changes it would have found two missing label.

Executed Tests

Successfully built oldcore with mvn clean install -f xwiki-platform-core/xwiki-platform-oldcore -Pquality After building the other changes with mvn clean install -f xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-templates, I ran the test that sent a WCAG warning on CI with mvn clean install -f xwiki-platform-core/xwiki-platform-release/xwiki-platform-release-test/xwiki-platform-release-test-docker -Pdocker -Dxwiki.test.ui.wcag=true. It returned only the warning for the date field (less than the two warnings from before). I do not understand why this automated test still warns about the date field. Manual tests on a local instance with updated StringClass.class, PropertyClass.class and displayer_date.vm showed that this date input warning is solved.
If this PR gets merged, I'll keep an eye on the CI tests to see how the results evolve. Hopefully it was just a test setup mistake from my end and this PR solves the ticket completely. If not, I'll try to figure out what's happening in the test suite and provide a patch to correct this test.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 15.10.x

* Implemented a text alt.

TODO: test the feature. Could only get the translation key to work so far...
* Implemented a text alt.

TODO: Fix the test in StringClassTest. Somehow it doesn't find the component for Localization...
* Fixed the test by providing the component.
* Fixed the date displayer and tests going along it
@Sereza7 Sereza7 added backport stable-15.10.x Used for automatic backport to 15.10.x branch. backport stable-16.4.x labels Oct 22, 2024
@Sereza7 Sereza7 removed the backport stable-15.10.x Used for automatic backport to 15.10.x branch. label Oct 31, 2024
@manuelleduc manuelleduc self-requested a review December 13, 2024 08:07
@@ -293,6 +293,12 @@ public void displayCustom(StringBuffer buffer, String fieldName, String prefix,
ScriptContext.ENGINE_SCOPE);
scontext.setAttribute("object", new com.xpn.xwiki.api.Object(object, context), ScriptContext.ENGINE_SCOPE);
scontext.setAttribute("type", type, ScriptContext.ENGINE_SCOPE);
/* This is a text alternative fallback to explain what the input is about.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does having an aria-label only apply to PropertyClass and StringClass only?
If we provide a default generic text alternative, it is probably better if we try to cover all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before reaching this solution, I tried to provide a generic fix for all Property types. However, the displayEdit function is overridden for a lot of object classes. This solution only fix String properties, but a similar solution could be used to provide text alternatives for the inputs of other property types.

Since there's no straightforward way to generalize this without updating all of the classes implementations for displayEdit, I don't think I want to merge it all at once.

These changes have a wide impact, so I'd rather keep them as minimal as necessary to avoid breaking things. See for example the number of test updates needed just for what was done until now.

Moreover it doesn't seem to me like it's needed for other property types. This might already be too generalized, in practice I only need this change to the displayEdit function for

#if ($xcontext.action == "edit" || $xcontext.action == "inline")
$doc.use("ReleaseCode.ReleaseClass")
Version: $doc.display("version")
Release Managers: $doc.display("releaseManagers")
Release Date: $doc.display("releaseDate")
#else
## Note: We use the include macro below to circumvent the "nested script" execution issue...
{{include reference="ReleaseCode.ReleaseSheetHeader"/}}
{{include reference=""/}}
#end
. I could probably just have hardcoded a text alternative in this form sheet.

WDYT?

@manuelleduc
Copy link
Contributor

It returned only the warning for the date field (less than the two warnings from before). I do not understand why this automated test still warns about the date field.

@Sereza7 you are probably missing a re-build of the distribution.

  1. build the impacted xwiki-platform-core modules
  2. build distribution again mvn clean install -f xwiki-platform-distribution (there is way to be more accurate, but this will do)
  3. run the functional tests

Let me know if this lead to better results.

Also, you can check if the server produced by the test contains the right version of displayer_date.vm

@Sereza7 Sereza7 marked this pull request as draft December 13, 2024 09:09
@Sereza7
Copy link
Contributor Author

Sereza7 commented Dec 18, 2024

Thanks for the help! After building the distribution again it used the right version in the tests.
There is no longer any WCAG failure in this test module :)

@Sereza7 Sereza7 marked this pull request as ready for review December 18, 2024 09:52
@Sereza7 Sereza7 requested a review from manuelleduc December 18, 2024 09:52
Copy link
Contributor

@manuelleduc manuelleduc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants