[A11Y] Add focus ring mixin to restore ring to elements which no longer have it

[davwheat]

Changes proposed in this pull request:

These mixins allow us to restore default browser focus rings on elements which no longer have them.

Reviewers should focus on:

I chose to make the styles mixin "private" as it shouldn't be used manually elsewhere in core -- we should instead use the other available mixins in the file. Is this the right choice?

Screenshot

This screenshot uses:

.add-keyboard-focus-ring();
.add-keyboard-focus-ring-offset(4px);

Confirmed

• Frontend changes: tested on a local Flarum installation.

[askvortsov1]

It doesn't seem like these are actually being used right now, unless I'm misunderstanding something?

[davwheat]

No, but I'm writing some improvements for the Notifications styles which will require this.

I felt that putting it in its own PR made a bit more sense than combining it with the current almost whole rewrite of the styles.

(Grid and flex!)

[SychO9]

wouldn't it be easier to remove any outline: none on element that don't have it and let the browser do it ? or is it better for us to specify the outline ourselves ?

[davwheat]

Removing outline: none is also good, but there are weird nuances and situations that result in outlines not existing anyway.

This is intended to be a temporary fix anyway -- I hope to get as many of our mixins removed as possible so we don't have border radius repeated 5 times when only one of them does anything :P

---

It could also allow for easier customisation of outlines if, for whatever terrible reason, someone wanted to do so.

[SychO9]

but there are weird nuances and situations that result in outlines not existing anyway.

that shouldn't be the case by default, what you're seeing locally (in the image) is most likely because you also technically have the button :active or something that sets off the outline as well (we probably have outline: none all over the place 🙈)

However you make a good point about customization! how does that work here, by overriding mixins ? I don't think that's possible in less 🤔, we might have to move the outline value to variables.less

[davwheat]

In Less, mixins get merged together, with the later mixins taking precedence.

Though, saying that, moving it to a variable might be a good idea.

Should the variable be the entire value? I'm leaning towards that instead of splitting it into 3 separate ones.

[davwheat]

Actually, we can't move it to a variable as we have two values to make it work. Leaving it as a mixin might be our only choice.

[SychO9]

In Less, mixins get merged together, with the later mixins taking precedence.

Aha, interesting that's the case indeed!

Let's leave it as is then.

[davwheat]

Just added a quick comment about how to override the outline styles, and switched to using a #private namespace instead of cluttering up the root namespace even more.

I'll give both of you a few mins to note any objections to that. I think core should have probably used a namespace for its own mixins from the start, and we should probably recommend extensions do the same.

[SychO9]

neat, no objections!

I think core should have probably used a namespace for its own mixins from the start, and we should probably recommend extensions do the same.

I like the idea, I think it'd be worth implementing that when we get to refactoring more of the frontend code. I think it'd be good to have an issue about it.

[davwheat]

I've created a docs issue -- hopefully I'll get the chance to write a small bit about it before stable releases, and we can note that we strongly recommend switching to that: flarum/docs#322