Move note content handling into the event #2692
Conversation
Rather than trying to determine which note goes with which anchor at page load, we wait until a click has occurred and use the link's href. This fixes a bug where ALL links' popups showed the last note on the page.
|
Syd (@sydb) has informed me that, while this passes QA tests, the fix does not work. I think I know why: event bubbling means that the "thing the user clicked on" may have actually been the |
Also, include a fallback if the currentTarget is still not what we expect: a JS console message. (It will not help the user, but it will help anyone trying to debug the problem.)
There was a problem hiding this comment.
Given that we're updating, I think it's worth taking the opportunity to build on @amclark42 's work and update the surrounding bits of code too.
P5/webnav/popupFootnotes.js
Outdated
| @@ -30,13 +30,24 @@ document.addEventListener("DOMContentLoaded", function() { | |||
| for (var i=0; i<links.length; i++){ | |||
| if (links[i].hasAttribute("href") && links[i].getAttribute('href').substring(0, 5) == '#Note'){ | |||
| if (links[i].getAttribute('class') != 'link_return'){ | |||
| var dest = links[i].getAttribute('href').substring(1); | |||
| /* When an anchor link is clicked, we use its @href value to determine which | |||
| note's content should be loaded into the popup. */ | |||
| links[i].onclick = function (e) { | |||
There was a problem hiding this comment.
I know this is from the original, but if we're changing this, then why not move more toward ES6
const links = document.querySelectorAll('a[href ^= "#Note"]:not('.link_return')');
links.forEach(link => {
link.addEventListener('click', (e) => {
e.preventDefault();
// And then the rest as @amclark42 proposed
})
}
|
@joeytakeda I think updating the code is a good idea. I went for a minimum of changes because I had no way to test my changes against the site — the more dramatic my changes, the less confident I would have been in the fix. Syd helpfully tested the code on his local setup. I've now downloaded a copy of Chapter 23's HTML, and hooked it up to assets in my copy of the Guidelines repo. So I can make more substantial changes, because I can test them. But rereading your comment, it sounds like someone else is being asked to make the change. Can you confirm who is making the requested changes? |
|
Hi @amclark42 ! Thanks very much for this and apologies for the vagueness —I hadn’t meant anyone specifically, so if you are so inclined, I’d say please feel free to make the changes. |
|
@joeytakeda Awesome, thank you for clarifying! |
Use 'let' and 'const' keywords instead of 'var'. Use arrow functions, `addEventListener()`, and filtering on arrays. I cut out a function layer by triggering `showPopupFootnote()` directly from a link click. That function now takes an event object and uses it to determine the current event target and the footnote ID.
When a link is clicked, focus is moved into the popup, so screen readers can access the content of the note. When the close button (now an actual <button> with an ARIA label) is clicked, focus returns to the link which triggered the popup. These popups still aren't robustly accessible, but they're an improvement.
|
@joeytakeda @sydb I've just pushed up new commits which update and streamline most of the JS. I did not update I noticed that the footnote popups were doing a disservice to people using screen readers. If you tabbed to a link and hit enter, a popup would appear (good), but the popup would not be immediately in focus (bad). So anyone using a screen reader would have to read through the entire narrative (including the footnotes) before they noticed the popup and its content. I've made tweaks such that when a link is clicked, focus goes to the close button (yes, it's now actually a button, with an ARIA label to boot). When the close button is clicked or the escape key is pressed, focus returns to the link which triggered the popup. This is not a robust solution, accessibility-wise — ideally, we'd indicate that a link opens a popup, and constrain interaction to the bounds of the popup while it's open — but I think it's an improvement. I think I will leave the rest of the JS to the Council. |
Rather than trying to determine which note goes with which anchor at page load, wait until a click has occurred and use the link's
@hrefto determine the note.This fixes a bug where all links' popups showed the last note on the page. For example, clicking anchor 96 in chapter 23 currently shows note 101. (And the same is true for anchors 97 through 101.)
On page load,
var destwas set iteratively, concluding with the last anchor link. Because functions aren't run when they are declared, when the anonymous event ran and setshowPopupFootnote()in motion, it would always set the second argument to the current value ofdest— the last-processed note.I have not tested this fix, but I'm 90% certain it should work.