Fixed #36192 -- Used semantic HTML for buttons. #19283
Conversation
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
|
@Phinart98 can you look into the JavaScript test failures? Can you also rebase onto the latest main as we have some recent changes to the selenium test suite we will want to be reflected here 👍 |
Yes. I am currently looking into the JavaScript tests. Noted. I will also rebase onto main. |
|
Since #18958 was merged, there are a few more "buttons disguised as links" that have been identified. You'll need to rebase onto main and update these 👍 |
|
Alright, I will look into it. Thank you😊 |
c5f2dea to
79680dc
Compare
|
Hello @sarahboyce. I think the PR is ready for review now.😊 |
|
Hello @Phinart98! |
|
Thank you for the updates! |
|
Thank you, @sarahboyce. I will look at them as soon as possible. When I am done, should I mention you here, or should I toggle "needs better patch" on trac myself? I did not toggle it initially because I felt it wasn't in my power to make that assessment. 😅But now it feels like that is the right way to draw a reviewer's attention to the pull request. |
|
You can toggle it! It's how you alert people you want another review 😁 |
a29307e to
4a0d778
Compare
77ae496 to
6393277
Compare
|
Hello @sarahboyce , I have hit a brick wall and would love to hear what you suggest I do. My navigator suggested that I commented on the PR earlier during our final weekly check-in but I was trying different things on my own first😅. I made changes to the mobile view and rtl views as suggested but I rebased my branch onto main once again before pushing because I felt like it has been a while since my last commit. After pushing, the selenium tests failed again but this time around, the errors were unusual. They were unreasonably numerous and seemed to have nothing to do with the changes I made. My local machine is also not powerful enough to run the selenium tests smoothly so I like to use the CI checks on Github. The Django test suite passes successfully too. To see what could be going wrong, I reverted the change and kept things at the point of your last review to see if the selenium tests will pass. However, the 8000 y failed again and were not consistent as well. - they were flaky. I tried that a few more times and the same things happened(flaky test failures). Sorry about the many force pushes on the PR currently. I read a lot about rebasing when looking into the problem and realised that it's not a recommended thing to do often because it makes things difficult for reviewers because of the way it messes with commit history. So now I want to find out from you if there are some insights you could share on what could be going wrong? There is currently no new code to review. I haven't pushed the new changes you requested so that we can focus on the test failures. Currently, the state of the commits is the point of your last review(where all tests passed). This is separate from no.1 above. It's about the actual changes requested. Can you please share more details about the changes? The only difference I could point out on mobile based on the screenshot you shared is the "Add another XXX" button's padding getting reduced. If there are more changes I'm missing, please give me more details about them. For RTL views, I couldn't find any issues. I edited the CSS for the inline labels to stay with their heading text in RTL mode but it wasn't a button issue. Below are screenshots to demonstrate what I mean: Because It has nothing to do with the buttons, should I keep the change or does it deserve a ticket on its own? |
Refactored the admin interface by replacing buttons written with <a> tags with actual <button> tags to introduce semantic HTML and improve accessibility. Updated styling to match the previous implementation.
6393277 to
08c3463
Compare
|
Hi @Phinart98! Congratulations on making your first PR - sorry to hear about the trouble with rebasing and the Selenium tests. We are here at DjangoCon Europe and @sarahboyce kindly rebased your PR against I reviewed your changed and found that you've missed some buttons in the admin which also need updating. I found the following
I imagine you'll also need to update some more tests once you've changed these. I also found that visually, the buttons are now not the same as they were previously. They are actually now both differently and inconsistently sized. The previous buttons looked like this:
Where as now, after your changes, they look like this:
You can see that they are inconsistently sized now, and the size is not the same as before. You can also see the impact of not updating some of the old
I hope that gives you enough to think about and work with for now! 😄 |
|
Hello @anorthall , Thank you so much for the review and comprehensive comments. I appreciate you catching all that. It's insightful. I will begin working on them as soon as possible. Thank you @sarahboyce for helping fix the failing Selenium tests. |
There was a problem hiding this comment.
Hello :)
We were talking about removing some role="button" on anchors and Sarah redirected to here. :)
I took a look at the code and the admin after rebasing your branch on main (on my local installation).
I could fix a few styling issues (see my suggestions).
About the latest suggestions of Andrew, I don't agree with everything: in read_only_password_hash.html it's still a <a> but you should remove role=button. In both other templates he mentioned, these should definitely be buttons.
Let us know if you need some help on these. I can take over, too, if needed.
| } | ||
| } | ||
| addButton.on('click', addInlineClickHandler); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Hi @tut-tuuut, thank you very much for sharing your thoughts. They're insightful. I will incorporate them when I get back to working on this.
I would have asked you to take over 😅 because I haven't been able to make time to get back to it for the past few weeks. However, it's my first Django contribution, and I anticipate some free time in the coming weeks to work on it. I would love to see it through to completion.
I will circle back here if I need any help or clarification. Thanks for the help😊
There was a problem hiding this comment.
Perfect ❤️
I'm happy to help, feel free to @ me anytime.
| @@ -535,7 +535,7 @@ select[multiple] { | |||
|
|
|||
| /* FORM BUTTONS */ | |||
|
|
|||
| .button, input[type=submit], input[type=button], .submit-row input, a.button { | |||
| .button, input[type=submit], input[type=button], .submit-row input, button.button { | |||
There was a problem hiding this comment.
I did some checks, you can put back as it was previously:
| .button, input[type=submit], input[type=button], .submit-row input, button.button { | |
| .button, input[type=submit], input[type=button], .submit-row input, a.button { |
It won't break the style of buttons, and it will fix the styling of anchors. :)
| background-size: 16px auto; | ||
| top: -1px; | ||
| width: 16px; | ||
| height: 16px; |
There was a problem hiding this comment.
| background-size: 16px auto; | |
| top: -1px; | |
| width: 16px; | |
| height: 16px; | |
| background-size: 24px auto; | |
| top: -1px; | |
| width: 24px; | |
| height: 24px; |
| top: -1px; | ||
| width: 16px; | ||
| height: 16px; | ||
| display: inline-block; | ||
| } | ||
|
|
||
| .datetimeshortcuts a:focus .date-icon, |
There was a problem hiding this comment.
To have the button change color on focus too:
| .datetimeshortcuts a:focus .date-icon, | |
| .datetimeshortcuts button:focus .date-icon, |
Next line should be fixed too but Github won't let me add a suggestion on it.
| cursor: pointer; | ||
| } | ||
|
|
||
| .datetimeshortcuts button:hover { |
There was a problem hiding this comment.
| .datetimeshortcuts button:hover { | |
| .datetimeshortcuts button:hover, .datetimeshortcuts button:focus { |
| cursor: pointer; | ||
| } | ||
|
|
||
| .calendar-shortcuts button:hover { |
There was a problem hiding this comment.
| .calendar-shortcuts button:hover { | |
| .calendar-shortcuts button:hover, .calendar-shortcuts button:focus { |
| } | ||
| } |
There was a problem hiding this comment.
I think a new line is needed here!
There was a problem hiding this comment.
Thank you for working on this ticket @Phinart98 .
I was also discussing the removal of anchor tags with role="button", and Sarah directed me here.
In my opinion, not only the shortcuts inside the calendar and datetime widgets, but also the other buttons within them should be changed from anchor tags to proper elements.
index e295d33f76..46174f0c39 100644
--- a/django/contrib/admin/static/admin/css/widgets.css
+++ b/django/contrib/admin/static/admin/css/widgets.css
@@ -472,7 +472,7 @@ span.clearable-file-input label {
border-bottom: none;
}
-.calendar td.selected a {
+.calendar td.selected button {
background: var(--secondary);
color: var(--button-fg);
}
@@ -481,28 +481,32 @@ span.clearable-file-input label {
background: var(--darkened-bg);
}
-.calendar td.today a {
+.calendar td.today button {
font-weight: 700;
background: var(--secondary);
color: var(--button-fg);
}
-.calendar td a, .timelist a {
+.calendar td button, .timelist button {
display: block;
+ width: 100%;
+ height: 100%;
+ background: none;
+ border: none;
font-weight: 400;
padding: 6px;
text-decoration: none;
color: var(--body-quiet-color);
}
-.calendar td a:focus, .timelist a:focus,
-.calendar td a:hover, .timelist a:hover {
+.calendar td button:focus, .timelist button:focus,
+.calendar td button:hover, .timelist button:hover {
background: var(--primary);
color: white;
}
-.calendar td a:active, .timelist a:active {
+.calendar td button:active, .timelist button:active {
background: var(--header-bg);
color: white;
}
diff --git a/django/contrib/admin/static/admin/js/admin/DateTimeShortcuts.js b/django/contrib/admin/static/admin/js/admin/DateTimeShortcuts.js
index 9da2221f1e..0e136ebab1 100644
--- a/django/contrib/admin/static/admin/js/admin/DateTimeShortcuts.js
+++ b/django/contrib/admin/static/admin/js/admin/DateTimeShortcuts.js
@@ -164,7 +164,7 @@
// where name is the name attribute of the <input>.
const name = typeof DateTimeShortcuts.clockHours[inp.name] === 'undefined' ? 'default_' : inp.name;
DateTimeShortcuts.clockHours[name].forEach(function(element) {
- const time_link = quickElement('a', quickElement('li', time_list), gettext(element[0]), 'role', 'button', 'href', '#');
+ const time_link = quickElement('button', quickElement('li', time_list), gettext(element[0]), 'type', 'button');
time_link.addEventListener('click', function(e) {
e.preventDefault();
DateTimeShortcuts.handleClockQuicklink(num, element[1]);
@@ -173,7 +173,7 @@
const cancel_p = quickElement('p', clock_box);
cancel_p.className = 'calendar-cancel';
- const cancel_link = quickElement('a', cancel_p, gettext('Cancel'), 'role', 'button', 'href', '#');
+ const cancel_link = quickElement('button', cancel_p, gettext('Cancel'), 'type', 'button');
cancel_link.addEventListener('click', function(e) {
e.preventDefault();
DateTimeShortcuts.dismissClock(num);
diff --git a/django/contrib/admin/static/admin/js/calendar.js b/django/contrib/admin/static/admin/js/calendar.js
index b13242078a..78276312c8 100644
--- a/django/contrib/admin/static/admin/js/calendar.js
+++ b/django/contrib/admin/static/admin/js/calendar.js
@@ -160,7 +160,7 @@ depends on core.js for utility functions like removeChildren or quickElement
}
const cell = quickElement('td', tableRow, '', 'class', todayClass);
- const link = quickElement('a', cell, currentDay, 'role', 'button', 'href', '#');
+ const link = quickElement('button', cell, currentDay, 'type', 'button');
link.addEventListener('click', calendarMonth(year, month));
currentDay++;
}As mentioned above, I also changed the buttons inside the calendar and datetime widgets to
elements and did my best to preserve their original styling.Additionally, I think we should remove the role="button" attributes from the pagination buttons, as discussed in ticket #36366.
We should also remove the underlines from them.
Admin Pagination
Cheering you on for your first contribution! Feel free to reach out anytime if you need help. 🙌
|
@Antoliny0919 , Thanks for pointing that out. I'll look into that when I work on it. |
Refactored the admin interface by replacing buttons written with tags with actual tags to introduce semantic HTML and improve accessibility. Updated styling to match the previous implementation.
Trac ticket number
ticket-36192
Branch description
Some buttons on the admin page were not accessible because they were using tangs but did not link anywhere. I refactored them to use the correct semantic HTML elements.
Checklist
mainbranch.Screenshots
I changed all the following in the admin interface to use semantic HTML and styled them to look the same.
