-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Buttons that are links don't have a focus style #13267
Comments
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. |
Looking at the current code, links can't be disabled because when they're, then they're rendered as buttons:
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
When the post is not "saveable", the Preview button is not an So there's the need to apply the focus style specifically to links, meaning |
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. |
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 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:
But why are we using the Presumably the Preview button is a link ( 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 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. |
To clarify:
|
Understood. An problem with putting |
We can't disable links or make the not focusable 🙂Yes it's a problem. Why we would want to do that? |
Because then we wouldn't have to have a rule such as this:
And also — why would we want a disabled link to be focusable? |
I think there are some big misunderstandings going on here 🙂 For no reason, ever, a link element To target it with CSS:
|
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: |
Moving to the Accessibility Audit project, as #15544 greatly improved the focus style for link-buttons but those improvements are only visible in Chrome. |
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:
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 Alternatively, it could be expressed as separate style selectors, to cover both the enabled buttons and all links: &:focus:enabled,
a&:focus, |
@aduth, updated.
|
@nicolad There's apparently a workaround, but it appears quite convoluted: |
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:
One more example to test:
Remove
:enabled
from this line:gutenberg/packages/components/src/button/style.scss
Line 174 in 36b1758
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 🙂
The text was updated successfully, but these errors were encountered: