-
-
Notifications
You must be signed in to change notification settings - Fork 601
XWIKI-23602: Error boxes expand, but the markup does not convey this information to AT users #4670
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
base: master
Are you sure you want to change the base?
Conversation
…information to AT users * Converted makeRenderingErrorsExpandable to a jquery based function instead of one relying on prototype. * Added the appropriate markup for a extend/collapse pattern. * Moved an inline style to its block in messages.less
$(content || 'body').find('.xwikirenderingerror').each(function (index) { | ||
let error = $(this); | ||
let description = error.next(".xwikirenderingerrordescription"); | ||
if (description.innerHTML !== "" && description.hasClass("xwikirenderingerrordescription")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this if: first .innerHTML
isn't jQuery but native HTML element, and I'm not sure you can access it directly without accessing the actual HTML element (with description[0]
basically). Then, you need to check if description contains any element: maybe the selector returns an empty list. Finally the hasClass test is useless: you already are selecting the elements matching that class...
error.attr('role', 'button'); | ||
description.attr('id', 'xwikirenderingerrordescription-' + index); | ||
error.attr('aria-controls', 'xwikirenderingerrordescription-' + index); | ||
error.attr('aria-expanded', false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you can use an object in attr, that would make the code maybe a bit more readable:
error.attr({
'id': 'xwikirenderingerror-' + index,
'role': 'button',
....
})
* If a content is passed, add click listener for errors reported in this content (usefull for AJAX requests response) | ||
* Otherwise make all the document's body errors expandable. | ||
*/ | ||
makeRenderingErrorsExpandable: function(content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this removing a public JavaScript API? While I can't find any usages, I'm not sure if it's okay to just remove it. I think this would need a vote on the forum to break this API.
Jira URL
https://jira.xwiki.org/browse/XWIKI-23602
Changes
Description
Clarifications
Screenshots & Video
Here is a look at the UI and DOM after the changes proposed here were applied on my local instance:

We can see that the UI in itself does not change at all. However the DOM contains the appropriate info in aria attributes.
Executed Tests
Manual tests, see above.
Checked WCAG validity with the description collapsed and expanded, using the axe browser extension. It did not report any failure.
I couldn't get SonarQube to run on
xwiki.js
, it's probably having a bit of trouble with the 2000 lines of mostly low quality code by today's project standards.As far as I can see, the exact DOM of the error message isn't used in any docker test validation. It's used in
getHTMLExceptionMessage
but AFAICS this util function is just a util in contexts where the regular error messages can't be used.Expected merging strategy