Problem/Motivation
This is a followup from #3217926: Olivero: Rename and consolidate all color CSS variable names..
With the color consolidation in #3217926: Olivero: Rename and consolidate all color CSS variable names., there is no longer a differentiation in background color between the hover and drag states within tabledrag.
There should be a slight change of background color, and text needs to still meet contrast minimums
Steps to reproduce
- Fresh install of Drupal, with patch applied
- On
/admin/appearance, set administration theme to "Olivero" - On
/admin/structure/block, use the tabledrag to re-order the blocks. Note the color differences
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | Screenshot 2023-01-02 at 10.02.21 AM.png | 337.2 KB | andy-blum |
| #11 | Screenshot 2023-01-02 at 10.02.13 AM.png | 347.9 KB | andy-blum |
| #11 | Screenshot 2023-01-02 at 10.02.04 AM.png | 333.84 KB | andy-blum |
| #11 | Screenshot 2023-01-02 at 10.01.57 AM.png | 313.61 KB | andy-blum |
| #9 | 3260118-9.patch | 3.17 KB | ameymudras |
Issue fork drupal-3260118
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #4
kostyashupenkoComment #5
kristen polThanks for the MR.
1. Code changes seem straight forward.
2. MR applies to Drupal 10 only. Need MR or patch for 9.3 and 9.4.
3. Tagging for issue summary as it could use the issue summary template and have explicit steps to reproduce.
4. This can be tested on Drupal 10 with steps to reproduce.
Comment #6
mherchelThanks for the review @Kristen Pol!
We don't need a patch for 9.x, though, because the changes that caused this (#3217926: Olivero: Rename and consolidate all color CSS variable names.) were D10 only.
Comment #7
kristen polGood to know, thanks!
Comment #8
andy-blumThis will need rebased to 10.1.x (it may be easier to open a new MR than to attempt a rebase for over a year of commits).
Also, steps to reproduce would be super helpful
Comment #9
ameymudras commentedUploading a patch for 10.1.x
Comment #10
Bushra Shaikh commentedComment #11
andy-blumPer #1 the text-to-background contrast ratio is fine, what I'm not sure about is if the differences between the different background colors is enough. The background-color differences are between 1.06 and 1.12, but I'm not sure if the background colors need to have any specific CCR amount.
Per accessibleweb:
However, WCAG SC 1.4.1:
We change the mouse cursor over the tabledrag handle, which is another visual indicator of interactivity, but there's no non-color visual difference while and element is "active". But I'm also not sure we need one. Tagging for a11y review, and per @xjm's comments in other issues, accessibility issues are not minor. Screenshots attached.
Comment #12
andy-blumComment #13
bnjmnmWe don't need a non-color visual difference, but the reasoning is a little fuzzier than just "Meets WCAG 123". We already have the "show row weights" alternative to address accessibility limitations. I suspect it was created with mouse/pointer related disabilities in mind, but it also happens to be quite good for users with color or contrast perception issues. There's nothing wrong with implementing the approach here IMO, especially since it is addressing a recent regression.
I would like to see a tabledrag-specific followup created, however. I was initially going to say this is fine because seeing table row move as you drag is the non-color indicator. Then I realized the table row doesn't really move with the drag action until the mouse has traveled a decent distance. Compare that to the Sort A List example on this page. That an unambiguous not-color-only visual queue, and tabledrag would feel much nicer were it to do that.
Comment #14
andy-blumPer my review in #11, and @bnjmnm's review in #13, moving to RTBC.
Comment #15
bnjmnmFollowup: #3332668: Tabledrag should provide quicker/smoother visual feedback when dragging