[ci skip] Fix rails' guides dropdown when navigating with browser back button #54934
Conversation
- Removes *potentially* unnecessary js from the code - Closes a div in the index file
|
This breaks the accessibility of the menu. In my opinion we should revert it. These roles were carefully discussed in #51499. All the aria attributes and roles that are there have their reason for being. |
|
Actually, the reason behind using the JavaScript code there was because we only wanted to set the roles when we have JavaScript enabled. If that's not a concern and we'll assume that will always be the case, it's probably fine. |
@brunoprietog --Right, that's what I had thought and was hoping to get some clarification on as it seems in the other PR referenced removing the JS was potentially the play as well. If we wanted to keep that functionality, I think what we'd need to do is probably reference the existing attributes if present i.e. if aria-controls and aria-expand are already set then we should not set them as I mentioned in my conclusion. I'm happy to make that change, it might be the better compromise. |
|
Since it's a simple change I went ahead and made an update to it. I can make a PR if it is something you want to pursue. There shouldn't be a case, from what I can see, where the aria-attributes would not be present without at least the data-aria attributes being present to set them. Since the aria attributes will only be set when not present, we're assuming that 100% of the time aria attributes aren't there, the data attributes are. I'm not sure if it makes sense to implement sensible defaults as this should cover the basis. |
|
Anyway, I don't see it broken in edgeguides. The only problem I see is that someone with JavaScript disabled would see the link as if it were a button. But still, the link takes you to the same page anyway. If we conclude that that's a very extreme case, I don't know if it's worth pursuing this further, really. I think the more I think about it the more I'm convinced of that. I don't know anyone who uses the browser without JavaScript enabled to be honest 😂. That probably should never have been a link and perhaps a details summary would have been more appropriate. That would always work without JS. |
Fair enough! I was thinking the same thing--web developers are the primary audience using the site and if there are some javascript-free web developers out there, they know something that I don't 😂 . Thank you for your quick responses and feedback, much appreciated! |
[ci skip] Fix rails' guides dropdown when navigating with browser back button
|
👋🏽 I wrote that. @brunoprietog was right that making a link act like a button needs to be done in JavaScript so it works without JavaScript. Moving it to HTML is... not great but okay. The problem is that you have now also broken it from being a dropdown. It's no longer announced as such. Sure, there is enough information in my screenreader to tell us what's going on, but this is not great. The real fix is to revert the replacement on turbolinks before cache, or whatever the event is. @keegankb93 your suggestion here: #54934 (comment) would also work, and if that's easier, do that!
I have a few friend-developers that primarily browse the web using a text-based browser hooked up to text-to-speech and a braille display. @brunoprietog do you use a braille display too? Anyway, no JavaScript in that browser! And my gotttt it is soooo fast! |
Motivation / Background
There is an issue with the guide index not opening after navigating through history. I've provided a video below of the issue.
Screen.Recording.2025-04-17.at.1.08.47.AM.1.mp4
This Pull Request has been created because the guides index was not working in this scenario.
Detail
This Pull Request changes
The main issue is that because we set the aria attributes based off of psuedo aria attributes i.e. data-aria-expanded and then remove the psuedo aria attributes we have no way to set the actual aria attributes that are referenced throughout the code when navigating through partial refreshes. From my understanding, hotwire is being used for the main content frames, but the guides index is in the layout so we'll never see that HTML refresh unless a full reload is requested. Therefore, once those attributes are removed we have no way to select the guides dropdown from that point on.
I removed setting and adding of the attributes because from my perspective I don't see it as necessary since the original data-aria attributes are being statically set in the index.html.erb file and then we set the attributes on DOMContentLoad/Turbo load event, which in this case is roughly doing the same thing.
My only assumption of why it was written that way is because you want it to act as a link to the index of the guides page, but also act as a dropdown. That isn't how it is working today, so there shouldn't be any regression in functionality. If we wanted that functionality, we may want to change how that is done. For accessibility, we'd probably want to keep it as a button and have the index/home page be listed along side the guides.
Screen.Recording.2025-04-17.at.12.55.07.AM.1.mp4
Alternatively if we don't want these changes, I'd suggest we just do something that checks if the aria attributes are present first and use those otherwise use the psuedo attributes. That may be the easier solution, but I'm wondering if this is working as intended in production.