Make Seven theme's link + button focus styles more robust for keyboard accessibility

Here'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.

Interesting issue! Thanks for starting this.

Initial thoughts/questions:

1. I've never found this to be unclear in Safari or Chrome on OS X: those browsers on that operating system seem to have very clear focus styles already. So: does this problem only exist for other browsers or operating systems?
2. If we do this, shouldn't apply these styles/this pattern everywhere?

Hi Wim, I'm using Chrome. The problem is that the default focus is disabled on links.

a:focus {
  outline: 0;
}

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).

We've overridden default browser focus styles globally, and not provided replacements.

OMG!?

This seems to be the real problem, no?

seven's element.css does this:

a:hover,
.link:hover,
a:focus,
.link:focus {
  text-decoration: underline;
  outline: 0;
}

Shouldn't we move this to the seven.theme component?

#9: right, but isn't the problem that the Seven theme is overriding them globally? Is there a valid reason to do that?

The patch in #3 modifies the Stable theme - I don't think we'll be allowed to do that

I'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.

Moving to Seven theme then.

Right, my bad! Sorry!

I 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.

@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.

I 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/

Add style from comment #11:

.ckeditor-buttons li a:focus {
  background: #ffee77;
  outline: 1px dotted;
}

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?

This looks great to me!

There's an earlier issue of CKEditor just leveraging <a> tags for everything here https://dev.ckeditor.com/ticket/9988

Mostly highlighting it for those who are bothered by that.

Perfect! Really happy with this. The patch in #25 misses a newline, but that can be fixed while committing.

The 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.

Good point Alex. I've used the extend-library technique.

I'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.

Awesome work :)

+++ b/core/themes/seven/css/components/ckeditor/ckeditor.admin.css
@@ -0,0 +1,4 @@
+.ckeditor-buttons li a:focus {

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.

Speaking of comments, shouldn't we add this to the top of core/themes/seven/css/components/ckeditor/ckeditor.admin.css

/**
 * @file
 * Seven styles for the administration pages.
 */

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?

/**
 * Add CSS to see that the buttons are visible on focus for keyboard-only users.
 */

Here goes!

+++ b/core/themes/seven/css/components/ckeditor/ckeditor.admin.css
@@ -0,0 +1,12 @@
+ * Seven styles for the administration pages.

Minor: 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

I think Cottser is right, I am also quite sure we should just use the blue glow.

I'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.

@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.

@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.

reroll needed to just go with the blue glow rather than the yellow.

Adding a patch with above mentioned change.
Adding screenshot for the same.

Interdiffs are always appreciated.

+++ b/core/themes/seven/css/components/ckeditor/ckeditor.admin.css
@@ -7,7 +7,7 @@
+	border-color: #40b6ff;
+	box-shadow: 0 1px 3px rgba(0, 0, 0, 0.05) inset, 0 0 8px #40b6ff;
+	outline: medium none;

Tabs instead of spaces?
Other than that, it looks good.

Adding patch with corrections and interdiff as well.

Apply Patch #47 found issues.

@Vidushi Mehta update your patch.

Update patch #47

Ya checked that @amit.drupal. Updating a new patch. Hope it works.

I m not getting it why the patches are continuously failed testing

Patch 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.

Hello,

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

This 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.

1. +++ b/core/themes/seven/css/components/ckeditor/ckeditor.admin.css
   @@ -0,0 +1,13 @@
   + * Add CSS to see that the buttons are visible on focus for keyboard-only users.
   

   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.

2. +++ b/core/themes/seven/css/components/ckeditor/ckeditor.admin.css
   @@ -0,0 +1,13 @@
   +  outline: medium none;
   

   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!

Updated patch as mentioned in #67. Please review.

The first point on comment #67 has not been addressed.

@kiamlaluno Covered both the point in patch #69

There 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.

+++ b/core/themes/seven/css/components/ckeditor/ckeditor.admin.css
@@ -0,0 +1,13 @@
+ * Add CSS to see that the buttons are visible on focus for keyboard-only users.

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.

So the comment needs work as per @kiamlaluno's last comment.

The comment could be changed to the following, for example.

The focus ring is being added because it's disabled in [relative path for the file containing the disabling rule].
@see [link to the file]

The 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.