Skip to content
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

Block Supports: Add link hover color to individual blocks #4643

Closed

Conversation

youknowriad
Copy link
Contributor

This back ports the php changes from the following Gutenberg PR: WordPress/gutenberg#48893

Trac ticket: https://core.trac.wordpress.org/ticket/58575

It basically adds style generation for link hover color to the elements block support and adds a few default values to the core theme.json files.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to support the other link pseudo classes?

const VALID_ELEMENT_PSEUDO_SELECTORS = array(
'link' => array( ':link', ':any-link', ':visited', ':hover', ':focus', ':active' ),
'button' => array( ':link', ':any-link', ':visited', ':hover', ':focus', ':active' ),
);

src/wp-includes/block-supports/elements.php Outdated Show resolved Hide resolved
Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>
@youknowriad
Copy link
Contributor Author

Does this need to support the other link pseudo classes?

Potentially later, but for now, this just brings parity with what we offer in the Global Styles UI to the block UI.

@peterwilsoncc
Copy link
Contributor

this just brings parity with what we offer in the Global Styles UI to the block UI.

Thanks, I figured as much but I'd sure feel silly if I didn't ask ;)

As an accessibility thing, it might be worth aliasing :hover to :focus for keyboard and assistive technology users to ensure they get visual feedback too.

If :focus styles are covered in another way then you can consider the PR approved (cc @tellthemachines for timezone reasons).

@youknowriad
Copy link
Contributor Author

youknowriad commented Jun 21, 2023

As an accessibility thing, it might be worth aliasing :hover to :focus for keyboard and assistive technology users to ensure they get visual feedback too.
If :focus styles are covered in another way then you can consider the PR approved (cc @tellthemachines for timezone reasons).

It's a great idea to nudge people to have the right accessibility guidelines applies to their themes and content but I don't think that WordPress should be making this decision for theme authors or users just like we don't do it for font sizes or things like that. We should try to show hints like we do for color contrast and headings though, that would be a great addition.

That said, this is out of scope of this PR and I'd suggest that you open an issue for it. This is merely a back port of a small change in Gutenberg to fix an inconsistency we had between the UI in global styles and the UI in the blocks. That capability to style hover colors for buttons has been on Core for some releases now.

@tellthemachines
Copy link
Contributor

If :focus styles are covered in another way

I would consider :focus covered by default browser styles unless the theme is actively unsetting them (which we can't really avoid because themes can add a stylesheet with whatever styles they like in it). We don't have any way of unsetting focus styles in the UI, so users can't accidentally make their site keyboard inaccessible.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both, LGTM.

@tellthemachines
Copy link
Contributor

Committed in r56028 / f571f45.

@youknowriad youknowriad deleted the add/link-color-hover-support branch June 26, 2023 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants