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-21375: Videos on the help page don't have alternatives #2466

Merged
merged 10 commits into from
Oct 18, 2023

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Oct 11, 2023

Jira

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

Forum discussion

Handle videos in the help page

PR Changes

  • Changed the list items to anchors sending to different documentation pages
  • Added a hint to explicitly set the text alternative of the videos

View

We can see that now all list items under the videos are anchors that redirect to various sections of the documentation.
21375-afterPR

Notes

The way I implemented the solution we converged to in the discussion is based upon this example in a proposal of atomic rule for ACT. There is no designated role for alternative to videos in aria, we rely on the user's understanding.
I decided to add a non-visual hint to highlight the bond between the video and its text counterpart. This element would clutter the visual UI, and fills a role for AT users that presentation handles visually.

Here are all the pairs label + URL added in this PR:

* Changed the list items to anchors sending to different documentation pages
* Added a hint to explicitly set the text alternative of the videos
@vmassol
Copy link
Member

vmassol commented Oct 11, 2023

I like it! I think it brings value and not just for impaired viewers but for everyone.
Thanks

@vmassol
Copy link
Member

vmassol commented Oct 11, 2023

Discover how to create an application --> http://localhost:8080/xwiki/bin/view/Help/Applications/

I think this should be a link on xwiki.org since we also need a link on https://www.xwiki.org/xwiki/bin/view/Documentation/UserGuide/

BTW shouldn't we have the same vides on https://www.xwiki.org/xwiki/bin/view/Documentation/UserGuide/ than on your screenshot? I see only 4 at https://www.xwiki.org/xwiki/bin/view/Documentation/UserGuide/ which is weird.

Copy link
Contributor

@michitux michitux left a comment

Choose a reason for hiding this comment

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

I have some doubts regarding the note. Also, the code style is quite inconsistent and I think there is missing escaping, see the detailed comments.

@@ -192,6 +192,7 @@ help.videos.videoCard5.topic2=Learn how to use your application
help.videos.videoCard6.title=Add / Remove Extensions
help.videos.videoCard6.topic1=Discover how to add/remove extensions using the Extension Manager
help.videos.videoCard6.topic2=Discover the XWiki Administration UI
help.videos.hint=The video in this card is an alternative to the documentation behind those links:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure a screen reader user is aware what "this card" is. Maybe rather something like:

Suggested change
help.videos.hint=The video in this card is an alternative to the documentation behind those links:
help.videos.hint=The following documentation links provide an alternative to the preceeding video:
  • but even then I'm not sure that it is clear to the user that there was a video, in particular as the iframe is marked with role="presentation". What about putting this information in the title of the iframe and maybe even removing the role="presentation"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 016b9d7 👍

I decided to remove the presentation role, and add the hint as an 'aria-description' to the iframe. This way, it can be read out to AT users and it's clearly concerning the content of this iframe. Updated the L10N value accordingly.

Comment on lines 110 to 111
'url': $xwiki.getURL('Help.Applications.WebHome'),
'label': "$services.localization.render('help.videos.videoCard5.topic1')"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the $xwiki.getURL not in quotes but $services.localization.render is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inconsistency 👍 Addressed in ae18d59 by removing quotes around the label values

* Removed the role='presentation' from the iframe
* Moved the hint to a description of the iframe
* Avoided breaking backward compatibility on the velocity macro 'helpVideoCard'
@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 12, 2023

@vmassol

BTW shouldn't we have the same vides on https://www.xwiki.org/xwiki/bin/view/Documentation/UserGuide/ than on your screenshot? I see only 4 at https://www.xwiki.org/xwiki/bin/view/Documentation/UserGuide/ which is weird.

From what I could see, this page is custom content, none of the videos are the same.
This page uses a video card macro, which is very similar to the one I changed in this file. I updated the PR in 291a7a1 to ensure there's retrocompatibility of the velocity macro (even if not useful in this case).

For creating an app there's https://www.xwiki.org/xwiki/bin/view/Documentation/UserGuide/GettingStarted/CreatingABasicApp

There's also https://www.xwiki.org/xwiki/bin/view/Documentation/DevGuide/Tutorials/Creating%20A%20Contact%20Manager%20Application/

👍 I chose to use the first one:

  • We already used other steps of the getting started in other links here
  • The video highlights app creation with AWM, so the first link is more appropriate

Updated in 29f8879


All comments so far have been addressed.

@vmassol
Copy link
Member

vmassol commented Oct 12, 2023

From what I could see, this page is custom content, none of the videos are the same.

hmm we might need to review this. I don't see how we would have enough manpower to have 2 sets of videos. We also need to check how much outdated both sets are.

I chose to use the first one:

I think the second one has more value but I've now linked the first one to the second so I believe it's now ok, see https://www.xwiki.org/xwiki/bin/view/Documentation/UserGuide/GettingStarted/CreatingABasicApp?viewer=changes&rev1=7.2&rev2=8.1&

Thanks

@@ -192,6 +192,7 @@ help.videos.videoCard5.topic2=Learn how to use your application
help.videos.videoCard6.title=Add / Remove Extensions
help.videos.videoCard6.topic1=Discover how to add/remove extensions using the Extension Manager
help.videos.videoCard6.topic2=Discover the XWiki Administration UI
help.videos.hint=The following documentation links provide an alternative to this video.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make this an actual description of the video? Like saying that you see a screencast showcasing the features that are listed below without audio (which could be interesting information for a blind user)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 758d90d 👍

<div class="videoCard-body">
<div class="videoCard-title">
$escapetool.xml($data.title)
</div>
<ul>
#foreach ($topic in $data.topics)
<li>$topic</li>
<li>#if ($topic.url)
<a href="$escapetool.html($topic.url)">$topic.label</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be $escapetool.xml as $escapetool.html doesn't escape {. Same for all other uses of $escapetool.html. Also, why is the label not escaped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, why is the label not escaped?

I went too fast when updating the definition of the topics. I thought I'd factorize the escaping but forgot to add it back.

👍 Addressed in 61140f9

<li>#if ($topic.url)
<a href="$escapetool.html($topic.url)">$topic.label</a>
#else
$topic
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, the topic should be escaped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Addressed in 61140f9

@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 18, 2023

Addressed all comments :)

@michitux michitux merged commit 877d3e6 into xwiki:master Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants