Buttons that are links don't have a focus style

[afercia]

Broken in #10031

Seems only Chrome understands :focus:enabled on a link. Firefox, Safari, Edge, and IE 11, don't.

I tend to think the Chrome behavior is wrong, as the :enabled pseudo class is supposed to work for elements that have a corresponding disabled state. Links don't have a disabled state.

Currently, links styled as buttons don't have a focus style and also the style for Windows high-contrast mode doesn't work any longer.

To reproduce:

• use a keyboard
• tab to the "Preview" button (it's a link)
• in Chrome, there's a subtle focus style
• in Firefox, Safari, Edge, IE11 there's no focus style

One more example to test:

• the "View Post" button (it's a link) at the end of the Publish flow

Remove :enabled from this line:

gutenberg/packages/components/src/button/style.scss

Line 174 in 36b1758

&:focus:enabled {

Then build and test again: the focus style is now visible in all browsers.

Of course, this is just for testing purposes. The CSS would need some additional adjustments. /Cc @jasmussen

Also, the current focus style is definitely too light. Focus indication needs to be highly visible for obvious reasons. I'd like to discuss this with the accessibility and design team and finally get to a positive action plan 🙂

[jasmussen]

I can take a stab at both the focus style and fixing this regression.

Short of reverting to the past behavior, do you have any instincts as to how we might address the same purpose as #10031 tried to address, but still work in all browsers? I'm sure I could think of something, but would appreciate ideas to test, so as to make this more rapidly fixed.

[afercia]

Looking at the current code, links can't be disabled because when they're, then they're rendered as buttons:

gutenberg/packages/components/src/button/index.js

Line 42 in 8ff1015

const tag = href !== undefined && ! disabled ? 'a' : 'button';

For example, this is what happens with the Preview button:

gutenberg/packages/editor/src/components/post-preview-button/index.js

Lines 179 to 186 in 8ff1015

	<Button
	isLarge
	className="editor-post-preview"
	href={ href }
	target={ this.getWindowTarget() }
	disabled={ ! isSaveable }
	onClick={ this.openPreviewWindow }
	>

When the post is not "saveable", the Preview button is not an <a> element, instead it's a <button> disabled element, with the disabled style.

So there's the need to apply the focus style specifically to links, meaning a.is-button. Hm to avoid a large refactoring... maybe using a class .enabled based on the disabled prop instead of :enabled?

[jasmussen]

Thanks for the ideas.

@Copons what do you think of #13267 (comment)? Do you have other ideas? Let me know and I can probably fix monday.

[jasmussen]

Quick sidenote, I just learned that Jacopo is on paternity leave 💓— so we'll have to do without his input for the time being.

I took a look at this today, and it seems like the primary purpose of #10031 was to simplify the specificity of the buttons so the !importants could be removed. That's noble on its own, but looking at the conversations it looks like disabled states played an important role here as well.

So I took a stab in https://codepen.io/joen/pen/LMqozY?editors=1100 to explore what might work. Correct, we can't use :enabled to increase the specificity in any browser but Chrome. But potentially we could use the slightly hacky duplication trick to increase specificity enough to ditch the !importants, by simply using :focus:focus. That seems to work so far, I'm going to take a stab in a PR in a bit.

But the other aspect was to not show a focus style for a disabled button. This bends my mind a little bit now that I'm focusing on it, pardon the pun — should a disabled element ever be focusable in the first place? @afercia touches on this briefly in #10031 (comment), but some of the details escape me, so I'll walk through the thought process:

• When a button is disabled, it's unfocusable, so it doesn't need special CSS to not show the focusring when focused. Because it won't ever be focused.
• A link (a) can't normally be disabled at all — we're sort of into hack territory with this, by adding an aria-disabled="true" property to inform screenreaders what's going on here.

But why are we using the a element at all, for something that needs to able to be disabled? As far as I can tell, we're using a link for the block mover (up/down) when at the start or end of the block list. And we're using a link for the Preview button, and this can be disabled when there is no content to preview.

Presumably the Preview button is a link (a) because it opens a URL, but I'm not entirely sure why the block mover can't be a button (though it's possibly related to this commit).

But in both those cases, these disabled links can still be focused using the keyboard. Should they not be unfocusable when disabled? I.e. have a tabindex="-1"?

If yes, then that could help unlock simpler CSS here, because we wouldn't have to provide specific CSS to "unstyle" a disabled and focused link, because a disabled link would never be focused.

[afercia]

But why are we using the a element at all, for something that needs to able to be disabled? As far as I can tell, we're using a link for the block mover (up/down) when at the start or end of the block list. And we're using a link for the Preview button, and this can be disabled when there is no content to preview.

To clarify:

• the Button component renders a button or a link depending on its props
• when there's a disabled prop, it's rendered as a button
• for example the Preview is a link, but when it has a disabled prop it becomes a button
• the Preview link doesn't have a focus style any longer after Buttons: CSS Specificity Clean-up #10031
• the Preview (disabled) button shouldn't have a focus style because it's disabled > not focusable
• as far as I can tell, the block movers (up/down) are always buttons

[jasmussen]

Understood. An problem with putting tabindex="-1" on any link-button that's disabled?

[afercia]

We can't disable links or make the not focusable 🙂Yes it's a problem. Why we would want to do that?

[jasmussen]

Because then we wouldn't have to have a rule such as this:

&:focus:not(:disabled):not([aria-disabled="true"]) {

And also — why would we want a disabled link to be focusable?

[afercia]

I think there are some big misunderstandings going on here 🙂

For no reason, ever, a link element <a> should have a disabled or aria-disabled HTML attribute. Links are meant to trigger navigation and have a href attribute pointing to a resource URI, they can't be disabled in any way.

To target it with CSS:

... there's the need to apply the focus style specifically to links, meaning a.is-button. Hm to avoid a large refactoring... maybe using a class .enabled based on the disabled prop instead of :enabled?

[afercia]

Screenshots:

"Preview" link-button focused in Firefox (focus style not visible):

Works on buttons that are buttons (although it's very subtle, see #15277, #15279, and #15432)

"View Post" link-button in the post-publish panel focused in Firefox (focus style not visible):

Works on the adjacent button:

[afercia]

Moving to the Accessibility Audit project, as #15544 greatly improved the focus style for link-buttons but those improvements are only visible in Chrome.

[aduth]

This was discussed in today's #core-editor triage session (link requires registration):

https://wordpress.slack.com/archives/C02QB2JS7/p1557752700099200

Based on the reading that there are styles which are meant to be applied only for focus on an enabled button, and conflict with the fact that “enabled” doesn’t mean anything for links (at least in Chrome), the following action item was proposed:

Change the following line:

gutenberg/packages/components/src/button/style.scss

Line 174 in 36b1758

&:focus:enabled {

To:

&:focus:not( :disabled ) {

...as this would apply both to (a) enabled buttons and (b) all links.

Noting that this contradicts the proposed suggestion at #11677 to avoid :not.

Alternatively, it could be expressed as separate style selectors, to cover both the enabled buttons and all links:

&:focus:enabled,
a&:focus,

[nicolad]

@aduth, updated.
Personally, I do like alternative more because it's easier to reason about without logic inversion.
But it throws me an error :(

Error: Invalid CSS after "a": expected "{", was "&"
"&" may only be used at the beginning of a compound selector.

[aduth]

@nicolad There's apparently a workaround, but it appears quite convoluted:

https://stackoverflow.com/a/25655130