Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Conditionally enable aria-live region for the timeline scroll pannel #1939

Closed

Conversation

pvagner
Copy link
Contributor

@pvagner pvagner commented May 28, 2018

(fixes element-hq/element-web#5696 and element-hq/element-web#5697)
We only wish to set aria-live to polite when the list is scrolled to the
bottom
to avoid flooding screen readers with loads of DOM changes when
scrolling
causing presentation of random message events.
To avoid presenting outgoing events twice we are only setting aria-live
when there is
a new content to add so e.g. style changes and similar won't be
propagated to assistive tools.

(fixes element-hq/element-web#5696 and element-hq/element-web#5697)
We only wish to set aria-live to polite when the list is scrolled to the
bottom
to avoid flooding screen readers with loads of DOM changes when
scrolling
causing presentation of random message events.
To avoid presenting outgoing events twice we are only setting aria-live
when there is
a new content to add so e.g. style changes and similar won't be
propagated to assistive tools.
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Hmm, I don't understand this fix. In particular I don't understand how it fixes the time bug. You mention in that bug about moving aria-live to an inner div, but this doesn't move it. I also don't understand how it would fix double-announcing messages since neither stuckAtBottom nor this._pendingFillRequests['f'] would be affected by that.

@pvagner
Copy link
Contributor Author

pvagner commented Jun 8, 2018

Hello,
Originally there were two issues.
One where the time is being repeat over and over. Initially I had difficulties reproducing that while using screen reader specific features combined with the keyboard navigation. After trying out more and more I have discovered that the time is spoken whenever it shows for any event after hovering over the link. That inspired me to move the region into some inner div where the content is. Problem with that approach is that the inner div is rendered along with its content. The region has to be rendered before its content in order to allow screen readers to react on the live region changes.
So this is more anoying to people using the mouse.
Later on I have discovered there is another related issue. When rapidly pressing page up or page down key the list is scrollyng quickly causing random times and random events to be announced by screen readers. The feel is very sluggish so it's even more usefull to do something to improue this.
So I have came with this solution that enables aria-live on the last item when list is stuck at bottom because what we do really want is to hear when a new message arrives. That is only working when the list is scrolled to the bottom.
By doing all this incoming messages are reported by screen readers and random spurious noyse is suppressed.
Outgoing messages were presented twice by screen readers so I ve added another condition to the patch.
Now outgoing messages are not read in full only the timestamp is reported however I find it more usefull than the previous implementation.
Still if you have some hints on how I might be make it even better, I'm happy to take another look.
Does it look too hacky to you?

@turt2live
Copy link
Member

Thank you for the PR! I know it's been a while, and this doesn't merge correctly anymore. I've bundled a fix for element-hq/element-web#5697 into #3007 but did not yet look at element-hq/element-web#5696.

I'm failing to parse how this fixes the time bug as well, and I'm somewhat convinced it isn't necessarily a bug. As mentioned in #3007 we can do a bit more to improve the communication here. What are your thoughts @pvagner ?

@jryans
Copy link
Collaborator

jryans commented Jul 31, 2019

Thanks for making this contribution a while back. Since the code base has changed since this was opened, it no longer applies cleanly, and I don't think there's a need to keep it open in this state, as we can always find the code here again if needed. If you are still interested in pursuing this feature, please discuss with us in #riot-dev:matrix.org to find a good approach forward.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screen readers should not announce current time every time a new message is received
4 participants