Channels: aria-polite announcement of new items and channels #4882
Conversation
crhallberg
added
accessibility
new language strings
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg. Haven't done any hands-on testing yet, but this looks good from a quick review of the code. Just one minor question below:
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg! See below for one minor suggestion which shouldn't change functionality but might improve readability. Beyond that, a few more general questions:
1.) Do you have a preference for whether we merge this PR or #4924 first? I imagine there may be some interaction between the two changes, so I want to be sure we focus our testing and time in the most useful way.
2.) Do you have any advice on how to test this? How have you tested it during development? Should we try with NVDA? Should we put this on hold until the A11y SIG is fully formed and ask them to take a look at it?
| data-channel-id="<?=$this->escapeHtmlAttr($channelID)?>" | ||
| data-record-id="<?=$this->escapeHtmlAttr($item['id']) ?>" | ||
| data-record-source="<?=$this->escapeHtmlAttr($item['source']) ?>" | ||
| id=<?=$channelID ?>-item-<?=str_pad($index, 3, '0') ?>" |
There was a problem hiding this comment.
Maybe it's worth using the htmlAttributes helper for this <li> -- I think that would improve readability.
(I mention it because I had questions about whether escaping was needed here. I think it's safe as-is, but if we used htmlAttributes, it would be easier to read everything, and there would be no need to question about escaping since it's all automatic).
2 participants
Manually incorporating 384259d 2c7b0af 109ada7 from #4557.