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

Feature/add null alt text to several images #9296

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

Conversation

JanhviHarwani
Copy link

@JanhviHarwani JanhviHarwani commented May 21, 2024

See JENKINS-62424.

Testing done

Confirmed the memory monitor text is visible from /extensionList/hudson.diagnosis.MemoryUsageMonitor/0/ and also confirmed that the graph does not render on that page

Proposed changelog entries

  • Add alternate text for images to help visually impaired Jenkins users.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Open Issue: https://issues.jenkins.io/plugins/servlet/mobile#issue/JENKINS-62424
resolved: added null text to alt attribute to img tags wherever necessary

Copy link

welcome bot commented May 21, 2024

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

@MarkEWaite
Copy link
Contributor

@JanhviHarwani I'm leaving this pull request open because it might address the Jira issue that is mentioned. It does not complete the pull request template. It does not provide a description of the testing that was performed to assure that the pull request works as expected. It does not include a usable changelog entry.

Could you complete those items on the pull request?

We're happy to have more contributors to Jenkins, but we need those new contributors to follow the processes that we've outlined so that we don't waste the time of maintainers as they process pull requests that do not provide the required information.

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks for the proposals. I've suggested different text based on the context of the images.

@@ -20,6 +20,6 @@ firstPRMergeComment: >

<a href="https://www.jenkins.io/participate/" target="_blank">
<picture>
<img width="600" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
<img width="600" alt="jenkins welcome page" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

Jenkins is an English language proper noun. It needs to be capitalized.

Suggested change
<img width="600" alt="jenkins welcome page" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
<img width="600" alt="Jenkins welcome page" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">

Copy link
Member

@daniel-beck daniel-beck May 21, 2024

Choose a reason for hiding this comment

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

Needs to be localizable.

Suggested change
<img width="600" alt="jenkins welcome page" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
<img width="600" alt="${%Jenkins welcome page}" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">

Additionally I am not sure welcome page adequately describes what this is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we can localize the text in pages that are generated by GitHub Actions.

Copy link
Member

@daniel-beck daniel-beck May 22, 2024

Choose a reason for hiding this comment

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

Good point, I managed to comment this on the only file it doesn't apply to 😅 (all your suggestions are not localizable).

@@ -61,7 +61,7 @@ THE SOFTWARE.
</j:otherwise>
</j:choose>
</div>
<img src="graph?type=${type}&amp;width=500&amp;height=300" />
<img src="graph?type=${type}&amp;width=500&amp;height=300" alt="null" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is the memory usage monitor graph, I think that the alt text should say that. The word "null" won't help a disabled user understand what is being shown in the image.

Suggested change
<img src="graph?type=${type}&amp;width=500&amp;height=300" alt="null" />
<img src="graph?type=${type}&amp;width=500&amp;height=300" alt="memory usage monitor graph" />

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 904cb3e

core/src/main/resources/jenkins/model/Jenkins/_404.jelly Outdated Show resolved Hide resolved
@@ -32,7 +32,7 @@ THE SOFTWARE.
<l:header />
<l:main-panel>
<h1 style="text-align: center">
<img src="${imagesURL}/rage.svg" height="179" width="154"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
<img src="${imagesURL}/rage.svg" height="179" width="154" alt="null"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<img src="${imagesURL}/rage.svg" height="179" width="154" alt="null"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
<img src="${imagesURL}/rage.svg" height="179" width="154" alt="Oops"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>

Copy link
Member

Choose a reason for hiding this comment

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

Decorative only. Also, null isn't intended as a literal value, but to represent the explicitly specified empty value (alt="").

Suggested change
<img src="${imagesURL}/rage.svg" height="179" width="154" alt="null"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
<img src="${imagesURL}/rage.svg" height="179" width="154" alt=""/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 4d8fd42

core/src/main/resources/lib/hudson/executors.jelly Outdated Show resolved Hide resolved
core/src/main/resources/lib/layout/dropdowns/custom.jelly Outdated Show resolved Hide resolved
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

This PR misunderstands the recommendation in Jira. null isn't intended as a literal value, but to represent the explicitly specified empty value (alt="").

@@ -20,6 +20,6 @@ firstPRMergeComment: >

<a href="https://www.jenkins.io/participate/" target="_blank">
<picture>
<img width="600" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
<img width="600" alt="jenkins welcome page" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
Copy link
Member

@daniel-beck daniel-beck May 21, 2024

Choose a reason for hiding this comment

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

Needs to be localizable.

Suggested change
<img width="600" alt="jenkins welcome page" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
<img width="600" alt="${%Jenkins welcome page}" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">

Additionally I am not sure welcome page adequately describes what this is.

@@ -32,7 +32,7 @@ THE SOFTWARE.
<l:header />
<l:main-panel>
<h1 style="text-align: center">
<img src="${imagesURL}/rage.svg" height="179" width="154"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
<img src="${imagesURL}/rage.svg" height="179" width="154" alt="null"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Decorative only. Also, null isn't intended as a literal value, but to represent the explicitly specified empty value (alt="").

Suggested change
<img src="${imagesURL}/rage.svg" height="179" width="154" alt="null"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
<img src="${imagesURL}/rage.svg" height="179" width="154" alt=""/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>

@JanhviHarwani
Copy link
Author

I have resolved all the comments in the PR. Thank you for taking out time to look at the PR.

@@ -20,6 +20,6 @@ firstPRMergeComment: >

<a href="https://www.jenkins.io/participate/" target="_blank">
<picture>
<img width="600" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
<img width="600" alt=${%Jenkins welcome page} src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
Copy link
Member

@daniel-beck daniel-beck May 24, 2024

Choose a reason for hiding this comment

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

This applied the wrong suggestion from the comment thread. Mark pointed out I was wrong. (Also would be syntactically invalid in Jelly.)

Suggested change
<img width="600" alt=${%Jenkins welcome page} src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">
<img width="600" alt="Jenkins welcome page" src="https://raw.githubusercontent.com/jenkinsci/jenkins/master/.github/images/jenkins-welcome.svg">

Copy link
Member

Choose a reason for hiding this comment

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

(Also I'm not sure this particular alt text is good, but that's a separate problem. I encourage you to consider what the image represents on this page, and how that would be accomplished in alt text.)

Copy link
Contributor

@MarkEWaite MarkEWaite Jul 30, 2024

Choose a reason for hiding this comment

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

Alt text updated in 87d4ab4

@@ -61,7 +61,7 @@ THE SOFTWARE.
</j:otherwise>
</j:choose>
</div>
<img src="graph?type=${type}&amp;width=500&amp;height=300" />
<img src="graph?type=${type}&amp;width=500&amp;height=300" alt="memory usage monitor graph" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<img src="graph?type=${type}&amp;width=500&amp;height=300" alt="memory usage monitor graph" />
<img src="graph?type=${type}&amp;width=500&amp;height=300" alt="${%memory usage monitor graph}" />

The same applies to all .jelly files. (Previously mentioned in #9296 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in 904cb3e

@@ -50,13 +50,13 @@ THE SOFTWARE.
${%No files in directory}
<j:if test="${showSymlinkWarning}">
<p>
<img id="symlinkalert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" />
<img id="symlinkalert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" alt="symlink alert"/>
Copy link
Member

Choose a reason for hiding this comment

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

Since the text in the next line exists, there is no need for an alt text here. This icon exists only to make the notice stand out.

Copy link
Contributor

@MarkEWaite MarkEWaite Jul 30, 2024

Choose a reason for hiding this comment

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

Resolved in 8d861dd

${%Symlinks are hidden}
</p>
</j:if>
<j:if test="${showTmpDirWarning}">
<p>
<img id="tmpdiralert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" />
<img id="tmpdiralert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" alt="temporary directory warning"/>
Copy link
Member

Choose a reason for hiding this comment

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

Since the text in the next line exists, there is no need for an alt text here. This icon exists only to make the notice stand out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 8d861dd

@@ -124,13 +124,13 @@ THE SOFTWARE.
</table>
<j:if test="${showSymlinkWarning}">
<p>
<img id="alert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" />
<img id="alert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" alt="symlink warning"/>
Copy link
Member

Choose a reason for hiding this comment

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

Since the text in the next line exists, there is no need for an alt text here. This icon exists only to make the notice stand out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 8d861dd

${%Symlinks are hidden}
</p>
</j:if>
<j:if test="${showTmpDirWarning}">
<p>
<img id="alert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" />
<img id="alert" src="${imagesURL}/svgs/warning.svg" width="16" height="16" alt="temporary directory warning"/>
Copy link
Member

Choose a reason for hiding this comment

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

Since the text in the next line exists, there is no need for an alt text here. This icon exists only to make the notice stand out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 8d861dd

@@ -32,7 +32,7 @@ THE SOFTWARE.
<l:header />
<l:main-panel>
<h1 style="text-align: center">
<img src="${imagesURL}/rage.svg" height="179" width="154"/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
<img src="${imagesURL}/rage.svg" height="179" width="154" alt=""/> <span style="font-size:50px"><st:nbsp/>${%Oops!}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent with the change in _404_simple.jelly, why?

(Personally I like this one more for the same reason as I mentioned in a number of other comments.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved in 23d56f1 by using empty alt text

Copy link
Member

Choose a reason for hiding this comment

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

Not true, this is the only line in this PR you didn't update 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching that. This was resolved in 4d8fd42

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jun 18, 2024
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jun 18, 2024
@MarkEWaite MarkEWaite added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Jun 24, 2024
As noted by Daniel Beck, it does not help the user of the screen
reader for us to show the same fundamental text message twice.  The
image in these cases is a hint for the reader of the message text that
immediately follows the image.  Cluttering the screen reader content
with a second copy of the same message is not helpful.
No need to display the "not found" message twice in locations very
near to each other.
@MarkEWaite MarkEWaite changed the title Feature/add null text alt Feature/add null alt text to several images Jul 27, 2024
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge. Needs a review from at least one other maintainer

@MarkEWaite
Copy link
Contributor

I attempted to display the memory usage monitor jelly change and could not find a way to reach it. The class is used in the system information page but the graph content is loaded JavaScript into a div on the page. The alt text in that JavaScript is "Memory usage graph" and does not appear to be internationalized.

@daniel-beck
Copy link
Member

I attempted to display the memory usage monitor jelly change and could not find a way to reach it.

A possible URL for this page is /extensionList/hudson.diagnosis.MemoryUsageMonitor/0/ and it appears it has always been broken (since afddd51), because the heap/nonHeap path elements are not present in the image URL.

@MarkEWaite
Copy link
Contributor

I attempted to display the memory usage monitor jelly change and could not find a way to reach it.

A possible URL for this page is /extensionList/hudson.diagnosis.MemoryUsageMonitor/0/ and it appears it has always been broken (since afddd51), because the heap/nonHeap path elements are not present in the image URL.

Thanks. I've opened that URL and confirmed that the updated text is displayed and that the image is not displayed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants