-
Notifications
You must be signed in to change notification settings - Fork 175
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
feat: Allow image descriptions to be read automatically by screen readers without needing to expand media. #2269
Conversation
originalStatus.media_attachments | ||
), | ||
sensitive: ({ originalStatus, $markMediaAsSensitive, $neverMarkMediaAsSensitive }) => ( | ||
!$neverMarkMediaAsSensitive && ($markMediaAsSensitive || originalStatus.sensitive) |
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.
It's not great that this duplicates code in StatusMediaAttachments, but I wasn't sure how I could get that info without some serious re-plumbing and I'm not quite experienced enough with this code base to pull that off.
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.
One way to avoid the duplication would be to pass sensitive
in to StatusMediaAttachments
. But It's not a big deal either way.
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.
So unfortunately it looks like there's an issue here. If there is a sensitive media, and you click the "show sensitive media" button, then the aria-label doesn't change. It permanently shows the status in "sensitive" mode.
Maybe this wouldn't matter much in practice, but it would matter if you e.g. are scrolling through the timeline, press the y
button to show the sensitive media, and the aria-label doesn't change at all.
I think in order to get this to work, we would have to plumb from the StatusMediaAttachments
back to the Status
. It looks like I already have a store value called sensitivesShown
that does this. I can look into this.
pinafore/src/routes/_components/status/StatusMediaAttachments.html
Lines 217 to 237 in a775bd9
toggleSensitiveMedia (changeFocus) { | |
const { uuid } = this.get() | |
const { sensitivesShown } = this.store.get() | |
sensitivesShown[uuid] = !sensitivesShown[uuid] | |
this.store.set({ sensitivesShown }) | |
this.fire('recalculateHeight') | |
// Only change focus for clicks, not for hotkeys. It's weird if, when the entire toot | |
// is focused and you press "y", that the focus changes to the sensitive media button. | |
if (changeFocus) { | |
requestAnimationFrame(() => { | |
const element = this.refs.hideSensitiveMedia || this.refs.showSensitiveMedia | |
try { | |
element.focus({ preventScroll: true }) | |
} catch (e) { | |
console.error('ignored focus error', e) | |
} | |
}) | |
} | |
return true | |
}, | |
async showFirstMedia () { |
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.
This should do it! 470cb15
Oops. I broke it. I'll look into it later. |
…ders without needing to expand media. Fixes nolanlawson#2257.
@@ -11,7 +11,7 @@ test('aria-labels for statuses with no content text', async t => { | |||
await t | |||
.hover(getNthStatus(1)) | |||
.expect(getNthStatus(1).getAttribute('aria-label')).match( | |||
/foobar, has media, (.+ ago|just now), @foobar, Public/i | |||
/foobar, has media, kitteh, (.+ ago|just now), @foobar, Public/i |
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.
Thank you for fixing the test! ❤️
…ders without needing to expand media. (nolanlawson#2269) Fixes nolanlawson#2257. Co-authored-by: Nolan Lawson <nolan@nolanlawson.com>
Fixes #2257.