Problem/Motivation
The CKEditor toolbar configuration UI (seen at e.g. admin/config/content/formats/manage/full_html
is very hard for sighted keyboard-only users to use. The available buttons CAN currently be moved in and out of the active toolbar, but keyboard focus is not indicated to the user, which makes it practicallly impossible to know which button you are dealing with.
This issue is part of our goal of meeting WCAG 2.0 level AA, specifically Success Criterion 2.4.7. Focus Visible.
This is a follow-up to #1872206: Improve CKEditor toolbar configuration accessibility. Accessibility of toolbar configuration for screen reader users was previously addressed there.
Before:
After (Well BarisW's first stab):
Proposed resolution
Add some Clear keyboard focus styles to the CKEditor toolbar configuration UI.
The scope of this issue is specifically about the experience of sighted keyboard users. It does not propose to change the screen reader experience.
Remaining tasks
- Decide how to style the keyboard focus. Should not use colour alone.
- Write CSS. Ensure it works in Seven theme (and Bartik?)
User interface changes
Adds keyboard focus styles for the CKEditor "Toolbar Configuration". Exact styles not yet decided. The design impact aims to be minimal, by only providing a focus style specific to the CKEditor admin UI.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff_65-69.txt | 1.74 KB | komalk |
#69 | make_seven_theme_s_link-2735421-69.patch | 1.35 KB | komalk |
#65 | make_seven_theme_s_link-2735421-65.patch | 1.44 KB | rensingh99 |
#65 | before_patch.png | 15.38 KB | rensingh99 |
#65 | after_patch.png | 20.95 KB | rensingh99 |
Comments
Comment #2
BarisW CreditAttribution: BarisW at LimoenGroen commentedHere's a first stab. I fiddled around with outlines and other styles, but eventually thought to apply the same yellow color that's used in the tabledrag interface.
Comment #3
BarisW CreditAttribution: BarisW at LimoenGroen commentedComment #4
Wim LeersInteresting issue! Thanks for starting this.
Initial thoughts/questions:
Comment #5
BarisW CreditAttribution: BarisW at LimoenGroen commentedHi Wim, I'm using Chrome. The problem is that the default focus is disabled on links.
The button groups work fine (since they are divs with a tabindex attribute. But the buttons itself are
a
's so we need to overrule the default link focus styling.Here's a screen recording. You can see that I move the button groups first (works fine) and then move the buttons within a group (it isn't clear which button is used).
Comment #6
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedLikewise, same experience in firefox. I can see a focus around the active button group, but not individual buttons. We've overridden default browser focus styles globally, and not provided replacements.
Comment #7
Wim LeersOMG!?
This seems to be the real problem, no?
seven
'selement.css
does this:Shouldn't we move this to the
seven.theme
component?Comment #8
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedRe: #4
Probably. Currently we have a mixture of focus styles in various places; custom and default, some obvious, others subtle. I'd like to review all of our focus styles eventually; a consistent set of focus styles. We also have #1993574: Add focus styling to all interactive elements but it's about Bartik.
The CKEditor config UI currently doesn't have any focus styles for the individual buttons, even though it works. In WCAG terms we have something which is operable, but not perceivable. So it's a brick-wall barrier for a clear user group.
Comment #9
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedRe: #7 Well, we've provided replacement link focus styles in lots of places, but when you override them globally you have a lot to maintain to reinstate them everywhere.
Comment #10
Wim Leers#9: right, but isn't the problem that the Seven theme is overriding them globally? Is there a valid reason to do that?
Comment #11
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedRe: #3, patch review:
Good idea re-using this yellow! It's actually nice and clear. However, it's also a colour-only indicator, and we shouldn't rely on colour alone.
I think we would be better to re-instate an outline as well, so we have a non-colour indication:
The 1px dotted outline is what you would have got by default... in Firefox. The really awkward thing about using outline: none is that there's no way to re-instate the default style from a particular browser.
The patch in #3 modifies the Stable theme - I don't think we'll be allowed to do that, but we can change the CSS in Seven.
Comment #12
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedre: #10
No. It would only be acceptable if you were introducing a global focus style at the same time to replace it, preferably in the same CSS block.
Comment #13
BarisW CreditAttribution: BarisW at LimoenGroen commentedI'm not sure. I discussed this with Cottser and emma.maria whether these small changes (that don't change the API) should be allowed on stable if they are accessibility improvements. I'd love to discuss this some more.
Comment #14
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedOK, if it can go in stable that's brilliant, otherwise we can change Seven and Bartik.
I'm bumping this to major, considering it's unusable for a particular group of users.
Also tagging for design review, since we need a visual change. We can minimise the impact by limiting ourselves ot a quick-fix in the CKEditor admin UI only.
Comment #15
Wim LeersMoving to Seven theme then.
Comment #16
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedOkay, that's an exaggeration ;-) Most links in Seven do get a focus style of some variety.
When Seven's elements.css does this:
It removes the default outline focus style, then replaces it immediately with another focus style (underlining). This is fine, it's just what I said in #12, and it works for most links.
The problem we're seeing with the CKEditor admin UI "buttons" is that they don't have any visible text to underline, so effectively there's no focus style.
So, Seven's approach breaks focus styles for some links ;-) I guess the next step is to search for any other examples of links with broken focus styles.
Comment #17
Wim LeersRight, my bad! Sorry!
Comment #18
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI am wondering, what about using the blue style as a border? I guess using our actual button style "blue" with an underline is a bit much.
Comment #19
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedIt turns out that Seven is the ONLY core theme where the CKEditor toolbar config UI buttons don't get a focus style.
I checked what the CKEditor toolbar config UI looked like with other core themes: Bartik, Stark, Classy, and Stable. I commented out the
hidden: true
lines to test them as admin themes. Apart from Seven, they all show default focus styles in Firefox, Chromium, Opera.So this is correctly assigned to the Seven theme component.
Comment #20
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented@Bojhan #18
Yes, the blue "glow" box shadow that we use on buttons elsewhere could be used. I just played around with it in Firebug, and it's nice and easy to follow when you use the tab and arrow keys in the CKEditor admin UI. It could do with having a stronger colour contrast against light backgrounds (more blue), but it provides a shape-based indication which is GREAT.
Comment #21
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #22
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #23
Bojhan CreditAttribution: Bojhan as a volunteer and commented@andrewmacpherson Do you have a screenshot. I'd largely prefer to reuse existing patterns when they suffice. Most of the other buttons are on on white backgrounds, except in the modal. So it should pass WCAG.
Comment #24
mgiffordI think the "Active toolbar" buttons focus disappearing the user tabs from the group to the buttons will be addressed here too. The focus works nicely on the group, just not on the button.
Also, wanted to through out this little appeal http://www.outlinenone.com/
Comment #25
Chernous_dn CreditAttribution: Chernous_dn at FFW for FFW commentedAdd style from comment #11:
To
core\themes\seven\css\components\ckeditor\ckeditor.admin.css
And include in
seven.libraries.yml
Pls look I'm moving in the right direction?
Comment #26
mgiffordThis looks great to me!
Comment #27
mgiffordThere's an earlier issue of CKEditor just leveraging
<a>
tags for everything here https://dev.ckeditor.com/ticket/9988Mostly highlighting it for those who are bothered by that.
Comment #29
BarisW CreditAttribution: BarisW at LimoenGroen commentedPerfect! Really happy with this. The patch in #25 misses a newline, but that can be fixed while committing.
Comment #30
alexpottThe CSS added here is loaded on every admin page. We should extend the drupal.ckeditor.admin library in seven to include this CSS - that seems like the best fix to me. You can use the libraries-extend in the theme's info.yml to do this.
Comment #31
BarisW CreditAttribution: BarisW at LimoenGroen commentedGood point Alex. I've used the extend-library technique.
Comment #32
mgiffordI've tested the new patch in Firefox & Chrome. Looks great. Just like the screenshot up in the issue's description.
I'm marking this RTBC.
Comment #33
Wim LeersAwesome work :)
Shouldn't this document that this is bringing back focus styles for this particular type of button? See #16, #19, and related comments.
Otherwise future maintainers will be surprised this is even there.
Comment #34
mgiffordSpeaking of comments, shouldn't we add this to the top of core/themes/seven/css/components/ckeditor/ckeditor.admin.css
Also, before bundling it in a patch, we'd want to add a comment something like this above .ckeditor-buttons to explain what this is here for, right?
Comment #35
BarisW CreditAttribution: BarisW at LimoenGroen commentedHere goes!
Comment #36
mgiffordComment #37
star-szrMinor: Maybe this should say "for CKEditor administration pages".
So if this is only for Seven, per #18 and #20 it would make sense to me to use the established Seven focus style (blue glow) instead of this. I didn't see a reason not do to this in the remaining comments.
https://groups.drupal.org/node/283223#Buttons
Comment #38
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI think Cottser is right, I am also quite sure we should just use the blue glow.
Comment #39
mgiffordI'm fine with going with the blue glow for the buttons. It won't be as noticeable as the yellow. The example though is quite different from the instance here. In most other cases there is more space around the buttons than there is here.
Comment #40
Bojhan CreditAttribution: Bojhan as a volunteer and commented@mgifford Am I correct in stating, that is a personal opinion not a accessibility issue?
We chose not to go as noticeable as you wish, that was a design decision on Seven. We can revisit that, but that would need to be a new issue - because we shouldn't do that as a one-off.
Comment #41
mgifford@Bojhan - sorry, I should have been more clear. Yes, I don't see an accessibility difference between one and the other. And yes, agree that this shouldn't be done as a one-off.
Comment #42
mgiffordreroll needed to just go with the blue glow rather than the yellow.
Comment #43
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedAdding a patch with above mentioned change.
Adding screenshot for the same.
Comment #44
mgiffordInterdiffs are always appreciated.
Comment #45
BarisW CreditAttribution: BarisW at LimoenGroen commentedTabs instead of spaces?
Other than that, it looks good.
Comment #47
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedAdding patch with corrections and interdiff as well.
Comment #48
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedComment #50
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedApply Patch #47 found issues.
Comment #51
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented@Vidushi Mehta update your patch.
Comment #52
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedUpdate patch #47
Comment #53
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedComment #54
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedYa checked that @amit.drupal. Updating a new patch. Hope it works.
Comment #57
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedI m not getting it why the patches are continuously failed testing
Comment #58
Manjit.SinghPatch applied cleanly on local env, but
Operation timed out after 30001 milliseconds with 0 bytes received.
Seems like this is diffrent issue, not related to code, I assume.
Comment #63
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedQueue another test against current dev branch.
It would be a good to get before-and-after screenshots of the most recent patch here.
It's worth getting screenshots for the patch here, but now that the new admin theme is underway (codename Claro) we can just aim to fix any poor/missing focus styles in Seven. I don't think it's worth a design overhaul for an admin theme we intend to replace.
Back in #16 I said:
However, I now think this is a bad idea, since the majority of our links in the Seven theme fail WCAG Use-of-Color, specifically by not having underlines in the resting state (not focused, not hovered). See #2958282: Links inside surrounding text fail WCAG Use-of-Color in Seven theme.
Comment #64
apadernoComment #65
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHello,
I have checked the patch #52 and it failed to apply with 8.9x.
I have made it compatible with 8.9x and uploading a patch.
I am also attaching a screenshot before and after applying the patch.
Below is a screenshot before applying the patch.
Below is a screenshot after applying the patch.
The patch is working great.
Thanks,
Ren
Comment #66
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedComment #67
bnjmnmThis looks great and I'm glad @kiamlaluno and @rensingh99 got this moving again. Among other things, I compared the use of box-shadow to other usages in core and it looks fine.
Two tiny things and I think I can RTBC it.
This would benefit from rephrasing. It's self-evident that that CSS is being added so that's unnecessary, and focus rings benefit more than keyboard-only users (voice navigation, for example), so it's not necessary to specify who this is for either. It's most helpful to explaining that a focus ring is being added due to it being disabled elsewhere in the theme with a
@see
referencing the file with the rule disabling the focus ring.This is an ubernit, but box-shadow should be the final rule. This is due to the declaration order specification in Drupal's CSS formatting guidelines. Border and outline are box-model declarations, which should come before box-shadow which is considered an "other" declaration. Which rule falls under which classification is not always clear or easy to remember, so its useful to reference
drupal/core/.stylelintrc.json
for this. This file can also be integrated in PHPStorm and other editors to highlight the formatting rules automatically.After those two small changes this should be in good shape, I think this will be very appreciated by users not using pointer devices!
Comment #69
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedUpdated patch as mentioned in #67. Please review.
Comment #70
apadernoThe first point on comment #67 has not been addressed.
Comment #71
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commented@kiamlaluno Covered both the point in patch #69
Comment #72
apadernoThere isn't any comment explaining that a focus ring is being added due to it being disabled elsewhere in the theme with a @see referencing the file with the rule disabling the focus ring. This is what I take one of the points in a previous comment meant.
Comment #74
mgiffordSo the comment needs work as per @kiamlaluno's last comment.
Comment #75
apadernoThe comment could be changed to the following, for example.
Comment #79
longwaveThe Seven theme has been removed from Drupal 10 core, and CKEditor 4 will soon be removed as well. I confirmed that this issue only affects Seven and no other themes included with Drupal core, so I am moving this to the contributed Seven project.