🚀 Space Reviewers 👾 Episode 7 - Accessibility Edition

Speaker 1: We are here to learn how to review our Jago PRs from an accessibility perspective. With the accessibility team.

Speaker 2: Should we do a round of intros or is it too much? Yeah. Who wants to get us started? Eddie

Speaker 1: Yeah, sure. Hi, I'm Ellie. I'm a software engineer from Uruguay. I'm a member of Jenga's XSVDT team on some other gender data things.

Speaker 3: I can

Speaker 4: go

Speaker 2: next.

Speaker 4: Okay, yeah. Hey, I'm Satakam. I'm a self-proflaimed human rights center developer. I work with accessibility and security in different parts. I am also part of the accessibility working group and the website working group in general.

Speaker 3: Hi, I'm Ramat. I'm a software engineer and a technical advisor here in Ghana. Uh uh I'm part of the Django Node space um community and then also part of the Django accessibility team as well.

Speaker 2: And uh I'm Thibaut. I'm um the current president of the Django Software Foundation, but today I'm just here as an access team member. I review Django pull requests on a semi-regular basis and I have picked two to review today on the on this space reviewer session I don't know if we should just get going with the reviews or if we have more context to provide. Did you have anything planned, Ramat and Ellie, or should we just get to it? I

Speaker 1: think we should just get to it. Like maybe give a like a quick intro of what the PR is relevant. But yeah.

Speaker 2: Yeah. So we've already mentioned our team. So I'll just leave that on screen while I make sure my screen share is all set up and uh get going. I think I just wanted to mention the fact that we have a team because that means we have a Well quite a big goal in having a team is having standardized ways in which we do those things. So this is our team charter that explains how we do accessibility reviews among other things And uh recently we spent lots of time writing formal guidelines on how we do this testing. Um I don't think I'll go through the guidelines right now, but uh this is just to say like This information is out there for people who want to do this.

Speaker 2: So yeah, I'll demo how I do my reviews, but it's very much meant to be based on uh those share guidelines. Nothing you have to like figure out on the spot on your own. Um yeah and um I I picked two PRs for today. I thought it would just be nice to have things that are a bit varied and um I guess approachable within about 30 minutes worth of reviewing. So those are pull requests that come from any and all contributors on the Django project. Some of those tasks As an accessibility team, we might have been involved with the reporting of the underlying issue as a bug for others to fix. And sometimes there are other reports from other people.

Speaker 2: So I picked this one which is about the date hierarchy feature of uh listings, change change lists in the admin It's a small improvement to screen reader navigation. And I also picked another one which is about focus management. I think both of those things are like quite common points where there's improvements needed across web apps and websites to some extent. So should be relevant for lots of people. Before I get into the code of those changes, I think it's just like worth clarifying the review process. It can be really tricky when you get reviews like this to have a copy of the project that has

Speaker 2: the relevant bells and whistles configured so you can test the specific feature. of the Django admin in this case, where there's something that needs changing. So this what I'm screen sharing right now, Django admin tests. That's my personal test project for anything Django It is based on another test project by our XQDC team colleague Tom Carrick. I think his project is kind of a semi-official one at this point in time. All mine does is adding on top some automation of how those tests are set up and um and adding uh more tests for things that aren't strictly the admin.

Speaker 2: So I'll just show that what that looks like quickly. Again, it's just worth saying there's no like right or wrong answer there. Lots of people have their different local demo sites set up. So this is just mine. So this is a list of everything that this demo site has set up. So it's called Django Admin Tests, but definitely the admin is only one of many things we have to test. And yeah, when you go to the actual admin, you have the exact same setup as Tom 's demo site with a bunch of different apps with ready-made content. And uh so in this case, since we're gonna be testing um the uh date hierarchy layout first I know that this is something that this demo site has

Speaker 2: set up out of the box. So it's really helpful for me to be able to go directly to this kind of view. and know that's very likely it's a relevant test of um the admin features that this PR will change Yeah, so I hope this helps for context. If anyone who's here has questions, definitely pop them in the chat and I'll make sure to look at them as I go through and hopefully address them. Um yes, so reviewing pull requests, step one, understanding what the change is about. Which definitely takes time. People provide quite different information from PR to PR. We do have a set process for how we do those things in Django, but uh it is a bit heavyweight, so it takes some time.

Speaker 2: So step number one usually when I look at the PR will be finding out if there is corresponding uh ticket for it with a ticketing system called track with lots of metadata about the problem and I think on there I just want a sense of um whether this has been triaged correctly uh if i'm reviewing this i want to make sure that i'm reviewing things that are kind of like accepted problems uh and then um having an understanding of the actual issue we're solving So here it says navigating this specific feature area of the admin with a screen reader, no description provided for that section. So that section being this list of dates. So I think from there

Speaker 2: the next step is understanding from a screen reader user's perspective what might be the problem, what types of accessibility improvements we think we could make. Obviously we're looking at a PR, not just a bug report. So someone's already decided one way of solving this. We'll have to decide whether this is the right way, whether there might be other options. And also we'll have to review to some degree the quality of the implementation. So not just like is it the correct pattern to use in this scenario. post-crane users, but also is it implemented in like uh a way that's maintainable and uh yeah just good quality Yeah, so I think once I've seen this, like here I have a rough understanding of uh of the problem

Speaker 2: and it'll probably be more interesting if I demonstrate it. by looking at the Django admin without the changes from the pull request first. So again part of the automation I added with this demo site was so I could have many different copies of uh the Django admin that I could look at all at the same time. So it really helps me when I do PRs to just like a thousand different versions readily accessible Yes, so in this case we're looking at this exact same demo site but with the Django admin as of the current main branch. I think here this means uh 3rd of December, I think at 9 pm

Speaker 2: yesterday. Yeah, 9 pm yesterday, so pretty much the latest. uh version of the Django admin that's not been released yet. And we're looking at this area there with the dates. And if I flicker back and forth between the new version in the previous one, I can definitely see what changes have been made. So just for clarity There are other differences between those two versions. It's just because the changes from the PR are based on whenever that PR was made. So don't necessarily reflect changes done in main since then. So here's the addition of the label and I assume some screen reader specific

Speaker 2: markup that might be relevant for us to look at. So how do we review whether those changes are correct? There are a lot of different techniques. Personally, I quite like to start with the developer tools in my browser. Just to make sure that I understand what markup has been used before I try and test this like a screen reader user would So I tend to prefer Google Chrome for this. It's not necessarily a right or wrong version of it, version like which tool to use. It's just what I know how to use personally. So we're looking at this nav tap links area. And then I'll look at the same thing in the updated version and see what markup we had before and after.

Speaker 2: Okay, so it's still a nav element and it has an extra AIL labeled by attribute. So here I look at the markup as kind of like a a dev would have authored it in in their templates and then rendered as HTML. I also want to look more specifically at the accessibility perception of that markup. like a screen reader would uh like which information a screen reader would convey to their users. So because it's a nav element , it's called landmark region in area and HTML and it has some extra uh meaning attached to it. So here I can see in my accessibility panel of the developer tools that it has role

Speaker 2: navigation Which is definitely what we want since it's a navigation component within this interface. And yeah, I think the difference here is just the addition of the extra label. You might be wondering why we want an extra label for this. It's really simple. You don't necessarily know what this is for if you don't have a label that tells you what it is for. And I guess that's true regardless of whether you're like sighted using a mouse or whether using a screener. It's kind of not neat that you have those filters by dates. But Certainly easier to know that those are meant to be filters by dates if it says filter somewhere around the date at a at a minimum

Speaker 2: And I guess here it says filter by release date. So I'm guessing release date is meant to tell you exactly which field of the list below it is filtered by. If there were multiple date fields, I would definitely also I don't know which one exactly is used as the filter. So I make it sound like kind of like a like a duh, why hasn't it always been like that? I think you know when Django contributors and users have stared at those kinds of views for ages. Of course you know that this is what it's for. But for a new person, um It's super helpful to have the label. So I guess at a glance, without even having looking at looked at the code, I appreciate that there is a site

Speaker 2: label for cited users. And I also feel like it's gonna be definitely helpful for screen reader users that this is associated with this nav landmark. Any questions from

Speaker 1: Yeah

Speaker 2: Yes.

Speaker 1: I have a question. Uh 'cause I think you mentioned uh having like it's easier or like you usually just look at the different kind of visual levers or like in the code like in HTML before looking at like the actual PR code.

Speaker 2: Yeah. And

Speaker 1: like is that always your approach or like what if it's at a way bigger PR that where it might be harder like to track what the act like where the actual changes happened or or do you just always go to the admin first and then like the PR or do you sometimes look at the PR code

Speaker 2: first? Yeah, there's definitely some flexibility there. I think in this case, just because there was this screenshot here, seemed quite clear to me that probably the change is only with that. And um you know here improving accessibility of data hierarchy layouts. That's not really a super good title. Uh This here is somewhat meaningless for me just because accessibility improvement that's not describing what the problem is, but that hierarchy layout I do have a good sense of where that is specifically. If you don't have a good sense, you'll be spending way more time before you look at the admin just figuring out where the hell should I be looking.

Speaker 2: Does that make sense? Yeah, so the code view. Like this is how full context. Definitely sometimes I just go straight to the code because I have a much better understanding of the code sometimes than whatever is stated in the description. Um yeah, so the code. Let's have a look at the code. Uh any more questions before I switch to that? Okay, so I see three files change. Usually I'll start by looking at which files have changed roughly. I'll give me a good sense of um how much time this might all take me and here this is pretty good sign because I see there is a test so I kind of know okay it's just gonna like be validation of whichever changes are elsewhere

Speaker 2: I see something that looks like a relatively simple template change and then template tags. I don't really get what this is changed for at a glance, but going back and forth between the templates and uh template tags file it seems quite clear that it's adding extra context to use in that template so okay kind of makes sense to me So really here this is the crux of the changes we're after, which is the addition of that label and um associating it with the the nav element via the area labeled by um so from an accessibility perspective uh i think it's clear that this works better than it used to,

Speaker 2: then I think there's gotta be questions about is there some improvements we could suggest to make it work even better? uh there's a balance to be found there between do you want to encourage people to do incremental improvements clearly as is it's already big incremental improvements. Do you want to suggest people go to whatever you might think is the best possible thing ever for this? So that balance is really hard to find. I'm not gonna sugarcoat that. And I think as accessibility people, it's definitely easy to be a bit pedantic sometimes and go for perfection over you know incremental changes. So Have to navigate that. So what do I mean by like uh perception, uh

Speaker 2: sorry, perfection over uh incremental? Um I think a good way will be for us to um Switch to testing with the screen reader and then we can discuss the specifics of their approach. So in our team guidelines Uh we have a bunch of different screen readers we suggest people test with. And uh just because I'm on macOS For me it means uh macOS voiceover is the most approachable of them all I'm just gonna restart my screen share and make sure that I configured it to share the sound of my computer as well as the video.

Speaker 2: Mm-hmm. Of course, Zoom needs extra permissions to share the sound. Okay. So if all is well, you should see Safari overlaid on top of Google Chrome. The reason for this is really simple. It's because um macOS voiceover works much better with Safari than any other browser on macOS. So I think almost all macOS voiceover users will be on on Safari. It doesn't really make much sense to test voiceover support for Google Chrome.

Speaker 2: Could you hear any of that? Okay.

Speaker 1: No.

Speaker 2: So I'll just make sure to change my Can you see the

Speaker 1: Yep. The little option, yep.

Speaker 2: Yep. Okay, I can't hear it either, so we'll proceed without the sound and um I think it will make sense anyway. So just a quick introduction to voiceover and screen readers They are meant for people who have low to no vision and uh those people will use them. The ones that have no vision in particular will use them based on the audio only. So this little panel we see here at the bottom of my screen share. That is definitely helpful for people who have partial vision, but that is not the main way by which they understand how they're using the web for them. basically they don't see any of this UI. They just have to rely on the sound that comes out of the screen reader.

Speaker 2: And in addition to the sound, that's kind of the output. The input for them is going to be lots of different shortcuts So I'm not gonna explain necessarily all of the shortcuts, the keyboard shortcuts that I'm gonna use for this, just because it would take forever. But I'll just like navigate with the keyboard in a way that's similar to how they would do it with the big the big difference that I do have the cited uh benefit of seeing uh what exactly I am on on the screen. So it really helps. It's really something that's you should try not to rely on as you're testing those things. So I'll start with the very simple check which is understanding the structure of the page and how that element fits into it. For that I'm gonna switch to a mode of

Speaker 2: voiceover that is called the router UI where you can navigate through all of the elements on the page uh semantically So here we're looking at the form controls rotor and I'm going to switch to the different routers to find the landmarks one So here that's all the semantic regions of the page that are useful for navigation. Banner is going to be the header area at the top of the admin. I think it's outside of view currently. Yeah, here we are. Predcrumbs, sidebar. So both of these you see are navigation landmarks That's exactly why it's helpful to have explicit labels for those landmarks.

Speaker 2: So really good that's our navigation landmark for data hierarchies. We now have this added label for it. It's a big improvement so that when you arrive on this navigating through landmarks, you do know which landmark is for what. You could imagine if both of these only said navigation with no label be impossible for Go to know which one's a filter, which one's pagination. So clear improvement here. But if I was to look at other common ways to navigate the screen, for example, headings. It's a bit confusing to me that this text that we added, even though it's like quite prominent in bold, it's not actually

Speaker 2: marked up as a heading And that means that if I was to navigate this UI with headings, I don't have the benefits of that text being there to move straight to the state hierarchy filtering. So that's kind of the first field would make me tick here, like shouldn't this be a heading? Wouldn't it be better for those users? if they could navigate more easily to this uh via headings. If we have the text on the screen, if it's bold anyway. Kinda looks like a heading, shouldn't it be one? Anyone have opinions about this? I see some nods, kinda uncertain.

Speaker 2: Yeah, I don't really know if it's the right answer here. I feel like generally headings, you know, the text will be underneath rather than to the side. We don't want to have heading mania. But um yeah, uh we'll have to make up our mind on that. Um okay, so at least I can see it in the rotor, the landmark, um, and now I can just try and move straight to it Um see what happens Yeah, so here I'm going through all of the areas of the page sequentially, the search area And filter by release date navigation. So here we know for sure area labeled bytes working as expected.

Speaker 2: The nav tag is working as expected. So I think I know I can stop that. And I can trust the markup for other parts of my testing. So works great. And I'll just draft that for now. And I'll add more info once I've made up my mind. Moving in the Python. This here is something that I I don't really think I have the expertise to review super well. So I think Here it depends a bit on your review style, how much you just want to vouch for the accessibility correctness or how much you want to look at also

Speaker 2: the like quality of the changes So I think what makes me um uncertain here is just that this has been repeated so many times. It kind of feels to me like without knowing this code too well, why do we need to do this exact same thing one, two, three, four times? So maybe at this point we need to move away from GitHub and look at the actual whole code of the file in a format that's a bit more convenient. I don't know here how others do it, whether you look at the code in GitHub primarily or on your local copy as well. For me I use a bit of both, just because I do find it way more convenient if I want to navigate faster through many files

Speaker 2: to use my ID. But if I only have a few files to look at, I try and focus on GitHub. How do you do things, other people, do you have the whole copy on your local always? Do you only look at GitHub if you can?

Speaker 4: I definitely check it out in local but yeah mostly I think I follow the same logic where if it's a small enough change, I don't need to actually look into it. But if there are confusing parts, then yeah, definitely go to the local part and see how things are interacting. Or if there is other things, like sometimes it's like I don't know why they did this on this particular element, maybe that helps. to look in some different file that is not even included in the PR. So yeah I would check that locally instead of through the PR I can't hear you, Thibaut. I don't know if everyone.

Speaker 2: Oh, thank you. I messed that up. Uh yeah, I was saying yeah it can be really tricky for projects you don't know too well to um judge whether the the changes that are proposed are complete enough. Know which bits of the code might have been missed. There's no other way than spending time hunting around unless you know exactly like the specific area of the project. Yeah, so here I guess we're inside a template tag that creates the state hierarchy. as the data for this state hierarchy area. So I'm expecting I don't I don't know this part of the project too well but this is the context that's going to be provided and we just want to make sure that field name is consistently provided every time this is rendered because

Speaker 2: no matter what data is there that'll always be a field name um so I think You know, if I was wearing my like full stack dev hat, I probably would want this to be refactored somewhat so we avoid repeating the same thing so many times. But in this case I can definitely see for a change like this we want to keep the existing code pattern rather than refactor the code for such a small change. Um and you know I'm primarily there to provide accessibility feedback, not like code structure feedback. So to me this seems good enough I'll use the GitHub UI viewed feature to just mark that I'm done with that and we can move on to the next file.

Speaker 2: Here again we have a similar challenge of uh whether we understand uh I guess the structure of the tests, how Django does automated tests. enough to provide feedback. So there's definitely a case that we could look whether there's existing tests that we might be able to update rather than add a new one So I think in this case I'd expect there there's no other test for this, but let's just have a look around and see if we can find something. So this is um file that's set up specifically for uh this this component. So I think it gives me a good sense right away that uh if there were related tests to update, they would all be in here.

Speaker 2: And I don't think I'll spend too much time looking around other files of the project to make sure So the tests that we are scrolling through so far, they all seem to be related to the logic of how we decide which filter options to display. I don't see anything that's testing the markup in there necessarily. And our new test is definitely more about like the markup, the UI of the tests than the logic. I'm kind of expecting that this will be the right place to write a new test and that we definitely need to write a new test. I think if I wanted to check more, maybe just like searching for

Speaker 2: other tests that might be querying this view. It seems like it's the only one. So Probably just leave it there. Um I guess we could search for date hierarchy Yeah, so this is the main implementation. We probably don't need to mention the docs. Sorry, the opposite. Okay, there are other tests about date hierarchy, so I'll be interesting to know why those state hierarchy tests are in different files

Speaker 1: I kind of have a question, I think. So here in this test file that you have if like they're using the reverse changeless view. It's not on the events, which is the one that I think the other test like on the PR was using, but like if there's a pattern of using the reverse uh instead of like the U You would comment on or would you just like let whoever's going to do a second pass, like not necessarily XSUT related, comment, those kinds of things

Speaker 2: Yeah, um yeah great point. It definitely seems like all of the other tests out there are using reverse. So I probably would flag that if I was reviewing all aspects of the PR. Here I don't really I'm not really meant to do this. So I think I'll just um use a way to make it sound like a question so that it's clear that I don't pretend that I know the answer I think I think I'm also tempted to question

Speaker 2: why there is other date hierarchy tests. And maybe um sorry I got lost through maybe those this new test will be better one way or the other. Um Yeah, so this series in particular I don't really get why. Um Well we have multiple tests, uh sorry, checks of uh of those tests. Maybe it's the time zone aspect that's relevant here. Um

Speaker 2: I think I'll probably just move this comment like that And um also I see there is So here what I'm trying to do is just

Speaker 2: help future reviewers that you know we spent time wondering about this might as well mention it here. And you know, we definitely expect that the answer is yes, it should be using it. Um hopefully from the contributor's perspective. is clear to them when they read this that we are not sure, but if they spend time looking at other test cases, they will see that a change is in order. Um didn't the other tests already? Oh yeah. Oh okay yes different file oh and the other tests don't even make requests

Speaker 2: Okay, yeah. At least as far as accessibility, there is definitely plenty enough reviewing of the of the tests. So yeah, I guess the last thing here is just to make up our minds about whether this should be a heading or no. Or which element. Why is it bold if it's a span? Why? I guess there's styles for the whole thing

Speaker 3: I mean, um I was thinking since the navigation like the dates, the same the same boat as well. That was probably rather made it bold because um if if it was heading like your um where you mentioned an example of where you have the heading at the top and then and other things come after it. That that's how I think of heading. But then everything is on the same line together with the navigations and then the filter by release data so but just bolding it to kind of like Make it send the same thing.

Speaker 2: Yeah, I think that makes sense to me. Um Yeah. I think I'll scratch that and probably just mention it here. Working all as expected. Was wondering whether um the label should be a heading rather than span, but since it's all in the same line

Speaker 2: There's many other elements we could use for this. Some people could say, oh, maybe it should be a paragraph tag. Maybe it's more semantic that way. Maybe it should be a strong tag. Maybe it's more semantic that way. I definitely feel like that's even more pedantic than heading, yes or no. The heading would have make a real difference for people who navigate through this because it would show up in the heading landmarks whereas the other ones I just mentioned there's very very few types of screen readers that support different navigation patterns with paragraphs and a strong emphasis. So yeah, I definitely something like this where I'm not sure will uh on the side of um

Speaker 2: Saying this is nice improvement as is. Maybe there's other changes we could do, but um There always is more changes we could do and it's clearly great as a fix for the issue this is about. Um took a while for quite small changes. SapTac, do you want to go next with your

Speaker 1: work function?

Speaker 2: Oh yes.

Speaker 1: Uh if you go back to the change files, uh there's like a comment there I think from another reviewer. Uh do you think that like that's a relevant thing or like do you think that should just be addressed as a separate ticket because yes, they have a point but like it's also not really related to the code change? Yeah.

Speaker 2: I didn't even notice or thought to mention it because it wasn't part of the changes in the PR. Um so This is about improving navigation through this issue. So I guess you could say that the markup used for the links is part of that But the proposed changes have have nothing to do with uh with the links. Using UI ally structure will be more appropriate in terms of accessibility. Yeah, that's kind of one of those things that I think there's always more appropriate ways to do things, but it's not necessarily uh Big difference. Um so if I guess the I guess the idea here is since it kind of looks like a list, maybe it should be one.

Speaker 2: Um I'm not entirely convinced to be honest. I feel like um The best way would be to test with the screen reader what it looks like if I navigate through this one way or the other and see which one might be more ergonomic So label link link link link And here if I wanted to try the screen reader version without breaking the bank, I'll probably copy the markup make quick edits to it in um vanilla HTML

Speaker 2: without modifying worrying about the Django source And see what happens. Yeah. Go back to Safari Um you can see I don't know how to use the developer tools in Safari. I don't think I even have yet on this computer. Do I even have them enabled? No. Ah things are backing out there.

Speaker 2: Okay. We don't really worry about what they look like right now. They might have I guess they might have some influence on um Um how do we remove um lists styles? Something like that is probably enough Yeah. So same navigation. Um List four items link. So here this is the key. We've introduced extra elements.

Speaker 2: It's gonna be dependent from screen reader to screen reader whether this Makes it easier or harder to navigate through the elements. Because now there is a link inside a list item inside a list. So here voiceover is clever enough to still allow navigation through all of the items one by one. And the difference we can see is it numbers which item you're on. Which sounds like it's really nice to know ahead of time how many items you'd have to go through. But knowing you're inside a list, I'm not sure it's so helpful. I guess you have the count of items ahead of times. So probably good, yeah I think I'll leave it there. Say that sounds like a good idea. Whether we need it, maybe.

Speaker 2: It's the kind of thing that I do

Speaker 1: you would say like that should be a separate ticket like to keep this PR kind of well scoped. Like is that correct?

Speaker 2: I think it depends how much you wanna review based on accessibility improvements versus process, correct process for a given project. From my perspective, when I review pull requests for Django, I think it's definitely more valuable for my time to be focused on the accessibility, quality of a change, rather than whether it's the most correct or no. per process or per like quality uh guidelines. Uh but having said that, you know, it's Nice if you have an opinion on both of those things so that it saves some other people time to make decisions. So here I would say uh

Speaker 2: I agree, but not sure it's worthwhile. Um it would allow people to know how many um Truly related to other changes in this PR. So I guess maybe coming back to this issue and seeing how it's uh framed. Yeah, this is about the purpose of the links. So I guess the structure of the links could be improved too, but I don't think it's a must. I think if I wanted to have a stronger opinion on this, I

Speaker 2: probably need to review how other um screen readers implement this. the list inside nav inside sorry link inside list item inside list inside nav that's a really common pattern So I'm hoping that all screen readers make it super nice to navigate. But I think I want to make sure before I say, oh yes, let's uh extend the scope of this PR, we definitely want that too. Um okay. Always more fun to be had with even the smallest of changes. Subtech.

Speaker 4: Yep. Okay, I can share my screen. Let me find the great screen first. I think I'm sharing the right screen now. Okay. So yeah, I am going to try and show a similar kind of process, but for Django Project. com. I

Speaker 2: can't hear you, Sartak. Is it just me?

Speaker 1: I I can hear him.

Speaker 2: It is just me.

Speaker 4: Okay, I guess I'll continue then So yeah, so I'm going to do a review of the Django Project. com PR because we do get accessibility PRs in Django Project. com as well as Django. Since Django Project. com does have a front end and Django Project. com also includes the docs. So this pull request is actually part of the docs and not the other pages. So the first thing I would usually do is like Thibaut said go to the issue and realize what the actual issue is. So here The issue basically says that the language and docs version switcher is impossible to use with a keyboard.

Speaker 4: So first it's a good thing to verify that's actually true. So this is the live docs page right now So if I just tap through the navigations basically, I'm just using my keyboard to capture it. I see that it goes to getting help. I hope everyone can see that here. But then if I tab again it goes it skips the language and document version and goes to the go to top page uh button. So Yeah, so we can see definitely that these things can't be reached by keyboard, so that's an issue because everything should be keyboard navigable through tab, especially Given this is an interactive element. So this should be getting the focus when I capture

Speaker 4: it. So there is a pull request for it. I actually kind of do the opposite of what Thibaut said, like first look at the visual change and then the markup. I prefer looking at the markup first because that kind of helps me give some like feedback even if I am not I haven't fully tested out the things I feel like if the markup itself I find out there are some issues or some things need to be changed that's good So actually in this particular pull request, I have already, I had already started reviewing this before we decided to actually do this call. So I did give some feedback on the markup, which was like One of the things this

Speaker 4: PR first did was use tab index equals to zero to make something focusable. So tab index equals to zero means that that particular item gets added into the focus order of all the interactive elements and then positive which you should probably never use and then negative helps you To make it focusable with JavaScript and things like that. So here they were using tab index equals to zero, but the first thing I see is that tab index equals to zero on an anchor element Doesn't really make sense. So first thing I would do and go like first thing I did was then go to the live site and check why is that anchored element not focusable for some reason

Speaker 4: And it seems like it was already focusable. So an anchor element is already in the focus order. So tab index equals to zero adding doesn't really change much. It's an added syntax. And since it is an anchor link, that is actually a good semantic element to be used here. So that was one of the comments that I added that I don't think a dive index zero is really necessary here because it's already focusable. The other comment that I had was actually the opposite thing. So if we go here and see like why this thing is not focusable, the reason is I don't know if it's too small. The reason is it's basically a span inside a list item.

Speaker 4: So a span is neither like not an interactive element, neither is a list item an interactive element. When you tab, it doesn't come in the focus order, but the way we have written the CSS, when you hover on it, it actually does do something, but it should be focusable so that with keyboard also it does the same thing. So what the PR did again was actually add a tab index equals to zero. And theoretically that works. Like that would make the list item focusable. So Um these two things would be focusable. But uh and this again like Thibaut said, there are many like inaccessibility you can go into too many depths and there are

Speaker 4: proper ways and more proper ways and someone's more proper way might not be other person's more proper way and so user testing is good and having Screen reader testing and stuff like that is good. But I feel like here instead of a span, a button makes more sense because it does look like a button, even though it's on hover and not on click. But It kind of does something, so makes it change and it is also like um in the style wise visually it's a button so I think like making it a button actually makes more sense so that's one of the comments that I added which was Maybe instead of span we can just make it a button and the added advantage is then we don't need to add this type index equals to zero because button itself is a focusable element.

Speaker 4: So It just makes it focusable. It's much more semantic. The HTML is more happy. And I prefer to avoid tab index equals to zero if you can. Because you Um don't want the user to be able to focus on everything because if you're using a screen reader or tab, you don't there they do have other ways of navigating as well. So you don't need to make a heading also a focusable element just so that a screen reader user can reach there. There are other ways they would do that actually. So Yeah, here tab index equals to zero didn't make sense, so you can make it into a button now. And those have actually already been implemented.

Speaker 4: So if I go to the code, um And doc. html. So one thing is yes, I am also happy to see that this is a two -file chain, so not much to look into. I think for many PRs of accessibility, it might not be this small of a PR so you might have to do a lot more looking into things. Um but yeah this is the HTML now it's Basically has a button inside a list item which I think makes more sense and then also for the documentation button it's also a button inside a list item Um the strong, um, I mean it's fine, it doesn't hurt anything, so it's okay. And

Speaker 4: Basically, the other thing that I notice here is and this is where you can also check out it locally and that might make it more visible, but the um this particular list item has now been moved into the beginning of the list instead of the end of the list and The reason that this is done and I can understand it is you would want it to be focused the first item because and then You would go to the other element. So let me basically show you what this does. So this is my local checked-out version of the PR, so that should Fix the keyboard navigation issues. So I will again tab through the different elements

Speaker 4: and then I reach getting help. So if you remember in the live one, it just jumped to the get to the top of the page button so instead of that here I see that when I actually go there um It actually focuses on the language user. And then if I tab, it goes to the other language. And then if I go to this documentation version, again it's focusing. And then if I tab it goes to def. So moving it in the beginning of the element basically allows it to first choose that and then go to the next element which is visible. So this is means that this is now definitely more keyboard accessible than what is there in the live thing. So In a way it does solve the issue, uh, but I

Speaker 4: also like to do something which uh like Thiba was saying. I am using Firefox on a Linux, so very different system. I think that is helpful because you get to see a few different things. So yes, Firefox on a Linux basically you can go right-click and they have this option called inspect accessibility properties And that just takes you to the tree itself. So they have a different tree for the DOM and different tree for the accessibility dome. So here you can see is the first element is a list item inside a list with a link. So if you click here you can see it's a role link name getting held. And the second thing that has now become a button before it was just a span, which is why it was not focusable.

Speaker 4: So you can see the role is button and the name is language colon N. And Here there are a few issues. Um and that's where you kind of have to decide like do we make uh like those comments in this particular pull request or create a new issue. I personally prefer opening new issues if I find something else accessibility related is broken. Which this PR was not even trying to fix. I would maybe create a new issue so that that can be dealt with separately and this can already be merged so that we have at least some fix for the keyboard navigation quickly So the issue that I see is language

Speaker 4: colon en um I feel like that can cause some issues because you might not know what is EN. So it could probably have a better accessible name. And actually if you focus on this and you can see I can make this key focused, I guess. If I do this, um it's this Should I focus also, I guess. It does with focus, but then that's fine. Um yeah, so if I go here and check the FR

Speaker 4: so I can see actually that list item it just says link fr so my screen reader also I can connect to Zoom but I did test it before this call and it just says Link FR or link and in fact EN it just says N, so that's not often a great thing. So maybe there is a way we can improve this accessible names itself to say Like language French and then language English instead of saying language Ian. But I feel that can also be a separate issue. So in this case I would maybe create a separate issue saying that improve accessibility for the accessible names of the language users so that it's better understandable rather than

Speaker 4: making it a part of the keyboard navigation. But that's um my way of reviewing I guess. Similar thing you can see also is in documentation version. If I go you can see role equals to button, name is there, and then you can see what are the actions. So you can press and something will happen. and documentation version colon. So it will basically say like when you go to the screen readable set button documentation version 5. 2 Um but here also it's little bit of a similar issue where if I do Go to the dev instead. Uh what really happens is

Speaker 4: I see it says link dev. So it might not be very clear when they go to the next item which just says link dev because You are just stabbing through it. So there are many different ways this can be fixed. Easy way, maybe like just give a better accessible name again, saying documentation version, colon div and stuff just saying dev so you can have this look dev but add an RO label or something like that or the other ways also is basically um you can Maybe make the interaction different. Like right now everything is since it becomes visible, so then you can focus onto it, but they there might be ways where you are like you make it more like a

Speaker 4: similar to tab navigation where you have to click enter on the documentation version and then use the arrow keys to move around it which maybe mix More sense. So there are very different ways that you can use a test and all, but I again feel like this is a completely different issue altogether. So I do find this issue while reviewing this, but I don't think this Specifically needs to go with this um this particular pull request. So yeah, so I see that this is navigable Everything is working good there. So I think the keyboard navigation part is definitely fixed. Is this the best way? Maybe at least because you can tap

Speaker 4: through it. There are different things to consider here where you might like feel differently, which is like if you actually go and check here, the language is a huge list. Similarly with the documentation version, it's a huge list. So basically someone tabbing would actually have to tab through all these versions now, like all these numbers now and all these languages now to go to the button. uh top of the page button. So which is why I was saying like maybe it makes more sense that instead of just tap into all of them, there should be an interaction in the language button which would actually move the focus to the first element in this list instead of just having to tap through all of them because that would

Speaker 4: make like navigating very um it would be a big navigation thing. Or you could do things like skip like how we have a skip to main content. So you can do like the skip to Top of the page button or things like that. So in this case, I'm not going to live write like Tiva the reviews, but yeah, I would probably leave a comment here saying that We should consider not just stabbing through all these items when it's stabbing, but instead Allowing users to kind of what happens in the navigation as well. Like when you go to the navigation, sometimes you would click enter and then you would go to the sub-navigations So I think something like that would make more sense here than actually typing through all of them.

Speaker 4: But I don't feel strong Like I don't have a strong opinion on that, but I would definitely mention that on the publicist, um, but not as a blocker maybe So yeah, that's the thing that I see has been changed primarily related to the keyboard navigation. Um I also see that They have removed the title. Now this is also I feel like title attribute is another very dividing opinion in the accessibility community. I belong to the people who don't like titles Because they are not keyboard navigable and since this PR is about keyboard navigation, I'm fine with them removing the title altogether. Um I would If we do want to have this information visible, it should be visible in a different way that is also keyboard navigable and screen reader.

Speaker 4: friendly and not use that right to attribute so I am totally okay with this thing so I'm happy to see that go. Same thing happens here So uh that makes me feel good about the interactions and the HTML. I will leave just that one comment saying that maybe consider um not tabbing through all the items and instead making another like press action which would actually help move the focus to those other languages. And then go and create an issue saying that we can maybe have better accessible names for the languages itself So those are the two actions that I would do for this review and then I kind of move to the style sheet as well

Speaker 4: because there can be certain things in the style sheet which um affect accessibility so it's good to see and I think in this style sheet it's better in a split view Okay. Um so yeah I can see that things have been like some things have been moved with display flex and flex direction column and stuff so these are more or less visual changes not really anything affecting the accessibility per se and I you can see that the color has been changed a little bit, which is like this used to be dark green and this is now black. So I will leave a comment about that, though I I feel like the black is a better

Speaker 4: um uh probably solves a contrast ratio issue there, so But yeah, we have a lot of green color contrast issues at Lane our website, which we are hoping to fix very soon. But yeah, you can see there are a few visual changes. None of them really affect the accessibility. One of the things that does affect the accessibility, which is kind of related to how the tabbing is happening, which is this thing. So before you can see that it was just on hover, so that is why it changed only on hover. Now basically what is happening is if there is a focus within the um like if there is a focus within this entire

Speaker 4: list or yeah so anything that gets a focus within this list will actually show this other list items which are otherwise hidden So one good thing about this is it's kind of handled by CSS completely and doesn't require any kind of um Java JavaScript related toggle thing, which I like most of the time but again that this is what is also actually causing the focus thing to move um inside this thing immediately after it's focused on language and which makes which might make the navigation big so my idea would be either you skip add a skip button or you make it a press action so This is where I would probably leave that comment

Speaker 4: that instead of focus within we can think something different maybe for So that the other like the thing showing up is a little bit different and then that would change the dab navigation as well. So yeah, that's how I would review this for Ligas. And yeah, I did go through the screen reader. So I can show, but I can't really show which is uh Fedora also has a screen reader, so you don't have voice over is not the only screen reader, you can use the screen data that you have in Fedora, Debian, or anything which you could basically which comes with um Like most of the Linux operating systems actually have Orca. So there is this.

Speaker 4: And you can see like if you are using a Linux-based operating system and you're like, oh no, I can't. Test this pull request, not really. You can use Anka, it's pretty good. It's a GNOME-based system. So if you have a GNOME-based Linux, it's fine. This article actually covers some of the like shortcut gigs and things that mostly you would need for Orca. But the thing about Orca is you don't get uh visual indication like voiceover. Even if I can listen, you can't listen, so that would kind of defeat the purpose, I think. Yeah. I guess I went pretty quick, but yeah, any questions

Speaker 4: related to everything I said

Speaker 3: Um I have a question. So um I about the tab index. I wasn't sure if you um mentioned it earlier, but Um is it just from maybe perhaps you reviewing um accessibility PRs or even like doing it a lot of times that you feel like um it's not necessary or you test it out first to see how it works? then you go back to say okay and perhaps this um um tab index is not necessary for this party I wasn't sure if you mentioned the last.

Speaker 4: Right, yeah. So I mean I usually do is I go through the code. I like Like I like writing HTML, so I feel like I like looking at HTML codes as well, but that's probably just me. But the thing that I started looking at the code and I feel is like you can definitely just go first to this thing and figure out if it's keyboard accessible and then go back and check the code and find out if there is some issues. But I usually love looking at the HTML code directly. And one of the things that I noticed first is there is an anchor element and the tab index zero. And I know that tab index zero is added to an element to make it focusable, but also an anchor

Speaker 4: element is focusable by default. So Um I could just leave a comment, but I like to go back and verify if there is a reason that was done as in If for example this is where the thing was added, so I would kind of check whether this was not focusable. So that would actually not change my comment I guess in the sense I would still say maybe you don't need to use a tab index equals to zero because it's an anchor element but the comment would be little different where I would be like it is an anchor element so it should be focusable so it should fix that instead of adding tab index zero but here I can see it's already focusable so um tab index zero

Speaker 4: adding is returned here But as of this thing, this thing won't change behaviorally. Like if you actually do tab, it will actually look the exactly same like how it's looking now But where it will change is when you go to this accessibility tree here, it won't say button, it will still say a span. So it would be a span and it will be basically a list, list item and span spare us This here is a little bit more semantic where it says it's a button. So it's a little bit clearer that this is supposed to do something, so it's changing something. And when you make a button, then you don't need the tab index equals to zero again, similar to the previous reason.

Speaker 4: So then you can remove that. Other questions

Speaker 2: Do you think there is ever a time where you can use the title attributes?

Speaker 4: I don't think so. Like I don't particularly hate the UI, I hate the UX I guess, which is like I have heard arguments where people are like well at least people who are using hover on desktop can see it and I'm like that's really sad. Um I feel like if you're making it accessible the inter I would rather have no one have it than one very small people have it because Title also doesn't work on like touch screen phones or tablets. So it's not like just screen readers. You can't use it on phone or anything. So yeah, definitely should not be used. You can use the same UI. So there are ways that you can

Speaker 4: basically make another information pop up when you hover or you focus. So Which is kind of what this PR is already doing. So yeah, but if it's just a title attribute and you go, you're adding the title attribute for the default behavior of browser, then you are definitely nowhere. Okay. Did Tiva, did you want to do the review of another pull request also or whatever?

Speaker 2: Um there was another do we have time? I don't know if we have time.

Speaker 1: I mean the meeting was set for an hour, but okay You know, yeah.

Speaker 2: Maybe I can say a few words. Um And uh maybe it will be interesting and then we'll leave it there. But yeah, quickly we can look at it. Um yeah, so I did um Also line up this separate pull request for review, which is about focus management. I thought it'd be interesting because um It's entirely JavaScript changes to quite a complex widget, which is I think a bit different in how you approach any improvements there So specifically it's about the focus management of the dates and time pickers in in the Django admin. The admin is

Speaker 2: Quite good in when it has those those date and time fields that you can always type the value directly. People might not think as this is an XCD feature, but um If you know the format, that's the one COVID, then you can always just dictate what you want in the field. I know it will work. But yeah, those those speakers are definitely a nice shortcut for people who don't know the format or just like the convenience. of the more visual UI. The problem we had with them is when you open them, moving focus to be inside the picker. And then focus management in there. So here we're looking at the the fixed version where focus is trapped inside that component.

Speaker 2: I can't move out of it job by just tabbing around and um I can actually move to it which is a big deal until that PR you couldn't actually use this UI with your keyboard at all Which is obviously a big uh big problem. And um yeah, same for the time picker. Um So here as far as testing I think it's pretty similar to what I'd shown earlier with um with voiceover in my case But this is also something that's relevant for all keyboard users. So here I'm just tabbing back and forth. I think what's really trickier about this though is uh knowing exactly like

Speaker 2: how best to manage this this focus So in particular, which which parts of those elements should the focus move to when you arrive on it the first time? And that's something that, you know, unless you're very familiar with those very specific widgets, you have to look it up. What's the expectation for focus management of date pickers? Here it's placing the focus on the on today's date. which matches the browser native date pickers behavior as well as the behavior of other date pickers. So it makes sense to me. For time pickers, I don't recall

Speaker 2: if browsers have uh I don't think browsers have a time picker with preset options like this And I don't think it's a really standard pattern. So I don't really know if um there is like um obvious choice, but I guess in the spirit of how dates picking works If we have a now option in our time picker, then that's probably a good option. Yeah, there's not necessarily any right or wrong here, it's just thinking of what's convenient for people So it all works with the tabbing. I want to check the same with the screen reader, but I will skip just in interest of time. And I think the review struggle

Speaker 2: will be whether the implementation is um I guess commensurate with the benefits we want to get. There are some changes in there that seem unrelated to me to focus management, like adding a title attribute. So This is more like quality feedback than accessibility correctness. And I think the moving the focus to the elements that's relatively simple The only nuance here is just also keeping tabs of what elements had focused before so you move back to it when closing dialogue

Speaker 2: That makes sense. But then what's actually quite complex to understand is the focus looping. So you see those new methods, add focus loop, remove focus loop. um that have been added. They are quite um lengthy bits of code and um It makes me tick a bit to see us uh implement our own custom focus trapping like this. Focus trapping is a very very generic concept. So I don't see why we would need to implement it ourselves and certainly not implement it per component like it's done here So just gives me a lot of pause whether the obvious UI

Speaker 2: UX accessibility improvement is worth so much code that I think have already done in d a different way. Um yeah, I guess specifically when I said different way, um the dialogue elements That's been added in HTML a few years ago now, that has pretty good browser and also screen reader support and support across other assistive texts. um that element like it's taken it some time to get um good access accessibility but now it's really really good and um if you couldn't refactor to that element the inert

Speaker 2: attribute in HTML um that is also meant to make it much easier to implement focus management like this. The idea is rather than um having to do lots of um event handling like this checking like oh did we press the tab key blah blah blah you can just declare which parts of the document shouldn't receive focus at all you mark them as inert And then the focus would naturally be trapped between elements that aren't marked as inert. So Yeah, I think this just gives me pause, but I'm a bit hesitant to share that feedback in here

Speaker 2: just because I haven't yet used this element this attribute myself. I feel like uh if I wanna review this I'll probably take the time to test that this does uh do what I expect. and ideally have some practical guidance on how to do this inside the Django code base. Does that make sense Yeah. So I'm a bit hesitant. I think for this , the best probably be to for me to come back to this after this meeting and say, hey, Maybe there is a much better way to implement the focus trapping.

Speaker 2: But I'm worried that I will not come back to it. So Maybe just I'll drop a comment there and then if I come back to it I will edit that comment. This is all working as expected. and a marked improvements over the current behavior Um , Even if um JavaScript, custom JavaScript was the correct approach for this.

Speaker 2: If it was any other project than Django, I'd definitely try and use the package for that. Because like again, it's not just like the amount of code or whatever. It's code that's really easy to get wrong and then creates more issues than you thought. for or like just you know corner cases with different assistive technologies so I'd much rather reuse someone else's code for this. Um could we try using the um and yeah just something like this I guess maybe worth a mention. Um We have quite strict browser support targets with Django and this uh this baseline project. It's a bit short of what I would recommend as a baseline.

Speaker 2: So because of it, um I tend to always go back to the can I use websites and actually check myself which versions of different browsers In the case of Django, I think we're gonna take particular care on So I guess I should say obviously this whole line should be green. If there is any browser that isn't supported, it is unacceptable for Django. for like core functionality that people rely on. But then within those lines the versions. So Safari 15. 5, I know Safari 15, there'll be people blocked on it from

Speaker 2: like three years ago they can't upgrade anymore to newer versions. Firefox, I want to think of the latest Firefox extended support version I think we're good for that. The latest extended support is probably around version numbers 125 to 135 of Firefox. So just keeping this in mind. Like it's not enough for it to just be green over lots of versions. I want it to be green since three, four, five years ago, depending on how critical the functionality is. Yeah, so in this case, 15. 5. I think in an ideal world I'd check the exact uh devices that are blocked on Firefox

Speaker 2: on um sorry Safari 15. Yeah, I think I'll leave it there just in in the interest of time. Anyone have questions about this? No questions, okay. Well I hope everyone watching enjoyed this Certainly always fun to test for accessibility and it's definitely super helpful to lots of open source projects when people take time to do this because it takes some some proper expertise to review um with with such depth. Um and it is daunting because it takes lots of practice to get to such depth.

Speaker 2: But uh again that's why we have guidelines for this stuff that I already made and um Yeah, I think you can you can get pretty far fast with this if you read those guidelines and just try for yourself to test with those things here and there you pick it up over time and um yeah it's super helpful when when people then make it happen on a on a regular basis for Django. Uh any closing words? Ramat? Eddie, anything you wanna say?

Speaker 1: Thank you to Timo and TapTag for bringing Pierce to review and to Tim and the Janglet space. Thank you, folks, for providing the space.

Speaker 3: And um thank you to everyone listening as well.

Speaker 2: Excellent. You have a good rest of the day everyone. See you on the internet.