GAUD-9134: make announce helper abortable#7043
Conversation
- Make accounce to return the id of the message to be announced - Refactor code to encapsulate the logic to create the message to be announced. Now the message is wrapped on a div. - Refactors logic that handles duplicated messages. - Adds messages global variable that maps message to its messageIndex
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
3f012ba to
b5054e3
Compare
…d Listener custom event d2l-navigation
| reset(); | ||
| } | ||
|
|
||
| document.addEventListener('d2l-navigation', () => clearAllAnnounce()); |
There was a problem hiding this comment.
This would be to handle the d2l-navigation event that the router would be dispatching. I am not sure if leave it as this (the whole document to listen to that event) or create a registerD2lNavigationEventListener that wraps this logic and would be attached to whatever node/element they passed as a parameter.
There was a problem hiding this comment.
Or perhaps this should go within the announce method and doing the deregistration logic in the reset ?
There was a problem hiding this comment.
Hmm yeah thinking more about how this should work, I don't think a single event that clears everything is going to work. Because there could be things added before or after the router navigates that still need to be announced.
Let's chat tomorrow about some options for how it could work!
There was a problem hiding this comment.
I'm kinda questioning whether we should be doing this.
|
|
||
| let timeoutId = null; | ||
| let container = null; | ||
| let messages = null; |
There was a problem hiding this comment.
Let's create it right away to avoid having to check for it later.
| let messages = null; | |
| const messages = new Map(); |
There was a problem hiding this comment.
well that would work but IMHO I think that variables should only exists when needed. Since this function resets these variables (timeoutId and container) maybe that reference to the map (messages) should also be dropped. What I agree with is with your comment down below to clear before the map before nulling it.
There was a problem hiding this comment.
variables should only exists when needed
Why? The assignment of the Map to the variable is negligible in terms of CPU and memory, especially when you consider that just defining a function (of which there are 4 in this file alone) is doing the same, and we define functions even if they don't end up getting called.
Like this:
function foo() {}Is the same as this:
const foo = function() {};The only time it makes sense to defer defining a variable until it's needed is if calculating the value you're assigning to it is expensive. At that point I agree and I like the pattern of setting it to null initially.
But if it's not expensive (it isn't here), then having it maybe-be-null and having to check for null each time we need to access it just adds complexity and increases the chance of forgetting to check in the future and introducing a bug.
There was a problem hiding this comment.
we define functions even if they don't end up getting called.
I think that functions and variables fulfill a different purpose even though they are both treated as objects in JS. and that is tightly aligned with the programming paradigm as well.
I know this could lead to a very insightful discussion. And just for giving an example, in OOP usually you initialize variables in constructors (including data structures such as arrays or maps or stacks, etc..). Of course there are more ways to initialize (i.e., in place where the variable is defined) but constructor's purpose is that: to initialize variables.
Depend of the case and the problem we try to solve. Here I fill announce is the place to initialize the map because there are different variables being initialized there (specially the actual container where messages will be read out loud).
But memory wise yep I think this makes not a big difference. is more about styles I think. I have my rule of thumb of not using variables whose scope is all the file, because handling or managing them could get tricky (here seems it is not the case though, so makes sense to have it as a file scoped variable initialized in place).
There was a problem hiding this comment.
Totally yeah. OOP is different in that sense in that the variables don't get initialized until the class is new'd, deferring that execution. But of course you get a copy of them for every instance of a class, so not really what we want here.
In terms of deferring the initialization of the container until announce is called, that makes sense to me because it's creating an actual DOM node so the browser is doing real work.
| reset(); | ||
| } | ||
|
|
||
| document.addEventListener('d2l-navigation', () => clearAllAnnounce()); |
There was a problem hiding this comment.
Hmm yeah thinking more about how this should work, I don't think a single event that clears everything is going to work. Because there could be things added before or after the router navigates that still need to be announced.
Let's chat tomorrow about some options for how it could work!
|
This PR is being closed. The main reason is because there is under development a new browser API for making screen readers to announce a message: ariaNotify. This API as I mentioned is still in progress. So if we continue the path of this PR, then ariaNotify gets stable, switching to it would be really hard. Here is the slack thread from where it is being discussed to continue or not this work, with the to not continue decision made. Regarding the ticket, it will be put into the backlog. I know @dlockhart created (or is about to) an alarm for six months from now to review the state of the API and if it is a good moment at that time to tackle this with the |
No description provided.