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.

Sign up for GitHub

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

Jump to bottom

Add alt text to histograms on stats page. #2624

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add alt text to histograms on stats page. #2624

wants to merge 1 commit into from

Conversation

somiaj
Copy link
Contributor

@somiaj somiaj commented Nov 17, 2024

This adds alt text to the histogram images on the stats page which fixes the issue mentioned in #2598.

In addition this fixes some issues I found with the stats page.

  • Links on the problem bars weren't working so forced them to be a string.
  • JitarSets problem stats - The adjusted status bars were reversed and didn't need to be and the adjusted scores were multiplet by 100 and didn't need to be.

Should the additional fixes be made into a hot fix, as these are present in 2.19 too?

@somiaj
Copy link
Contributor Author

somiaj commented Nov 17, 2024

I should note, it would be nice if someone who has actual Jitar set data can test this out. I only have normal set data that I transformed into a jitar set. It just felt like the bars on the problems page were backwards, maybe this won't be the case with real jitar data (not quite sure how jitar adjusted grade after review works). But the graphs I saw all seemed mirrored, so I removed the reverse. This way the adjusted data looks the same in my test case on both the set and problem stats page.

drgrice1
drgrice1 requested changes Nov 18, 2024
View reviewed changes
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

Unfortunately, the comment in #2598 (originally in an email from @glarose) is misguided. The alt attribute is not valid for svg tags. In fact, the only thing that the alt attribute is valid for is an img tag. Furthermore, screen readers will not read an alt attribute on anything other than an img tag. So at this point, this pull request doesn't fix anything, and adds invalid html. Another way to do this will need to be found.

The aria-label, aria-labelledby, and aria-describedby attributes do work. However, aria-labelledby is already set for the svg to be the svg title (which is read by screen readers). So the aria-label attribute also can't be used. You could add a visually hidden span and use the aria-describedby attribute. Then both the content of the element pointed to by the aria-labelledby attribute and the content of the element pointed to by the aria-describedby attribute are read.

You may also need to add tabindex="0" to the svg. In my testing (with Gnome Orca) in Firefox, the aria-describedby and aria-labelledby attributes were not read in browse mode at all, but if the svg is in the tab order, it could be focused and then the attributes are read. However, in Google Chrome, it does work in browse mode without the tabindex attribute. So I am not sure on this. It could be an issue with the Gnome Orca screen reader, or it could be a Firefox issue.

@Alex-Jordan
Copy link
Contributor

Alex-Jordan commented Nov 18, 2024 via email

@drgrice1
Copy link
Member

Note that this is an inline svg, so the img patterns listed on that site are not an option. It looks like the only options that work reliably are patterns 5 and 8. Although those patterns do not have both a title and a description. It seems that none of the patters with both a title and a description work reliably for all browsers. Although pattern 11 works to give both, but the description is repeated twice in some cases.

The warning about the results with screen readers being outdated may apply though, and some cases may have been fixed (or broken?).

@somiaj
Copy link
Contributor Author

somiaj commented Nov 19, 2024 •
edited
Loading

I moved the alt tag text into a <desc> tag for a long-text description of the image. Should I go with #11 mentioned above and also add this long description to the aria-labeledby of the image, or just leave it as a long-text description for browsers that support that?

@somiaj somiaj force-pushed the alt-text-histograms branch 3 times, most recently from 6a12607 to 2112faf Compare November 19, 2024 04:31
@drgrice1
Copy link
Member

This works for me with Google Chrome and Gnome Orca. I still haven't been able to get anything read for the SVG images in Firefox other than by adding tabindex="0" to the SVG.

I note that currently the SVG title is not a title. Instead it is a text tag that the SVG is aria-labelledby. Should that be moved to an actual title tag? Basically that would put this at pattern #10, and if the aria-labelledby includes both the title and desc then it is pattern #11.

@somiaj
Copy link
Contributor Author

somiaj commented Nov 19, 2024

Changing the text tag to a title tag means the title won't be visible on the graph in my testing, so this would mean we would need both a text and a title tag, so probably leaving the aria-labelledby is better?

@somiaj
Copy link
Contributor Author

somiaj commented Nov 19, 2024

I added tabindex="0" to the svg tag.

@drgrice1
Copy link
Member

Yeah, it would mean that. I think the current setup for the title should be fine.

@drgrice1
Copy link
Member

I am not sure on the tabindex="0" thing. That may be an issue with Firefox or Gnome Orca. It may be that we don't need to worry about it. It should be tested without that on other systems where screen readers would be used more frequently.

@somiaj
Copy link
Contributor Author

somiaj commented Nov 19, 2024

Okay, reverted the tabindex="0" change.

@somiaj
Copy link
Contributor Author

somiaj commented Nov 19, 2024

Would remove the aria-labeledby and only including a description be worth considering, as the description contains the title information and more?

@somiaj
Add alt text to histograms on stats page.
d630574 
This adds a long-text `<desc>` tag the histogram images on the stats
which fixes the issue mentioned in openwebwork#2598.

In addition this fixes some issues I found with the stats page.
* Links on the problem bars weren't working so forced them to
  be a string, along with wrapping the jitar status bar inside
  the anchor tag to the corresponding problem.
* JitarSets - The adjusted status bars were reversed and didn't
  need to be and the adjusted scores were multiplet by 100 and
  didn't need to be.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drgrice1 drgrice1 drgrice1 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@somiaj @Alex-Jordan @drgrice1