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

Link styles: underline enforcement overrides browser preferences #63647

Open
2 tasks done
markhowellsmead opened this issue Jul 17, 2024 · 7 comments
Open
2 tasks done
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Bug An existing feature does not function as intended

Comments

@markhowellsmead
Copy link

markhowellsmead commented Jul 17, 2024

Description

The definition at

gutenberg/lib/theme.json

Lines 360 to 364 in d1211ff

"link": {
"typography": {
"textDecoration": "underline"
}
}
forces links to be underlined unless an additional, more specific configuration is present. This overrides the default or preferred browser behaviour and adds unnecessary complexity to setting link styles based on user/browser preferences (potentially taken for accessibility reasons).

Step-by-step reproduction instructions

See above

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 6.6

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@markhowellsmead markhowellsmead added the [Type] Bug An existing feature does not function as intended label Jul 17, 2024
@aaronrobertshaw aaronrobertshaw added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Jul 17, 2024
@aaronrobertshaw
Copy link
Contributor

Thanks for raising this one @markhowellsmead 👍

I'm unsure of the best path forward here.

The point you make regarding the potential override of user/browser preferences for accessibility reasons is a good one. On the other hand, removing this style definition from the core lib/theme.json file might result in a sudden loss of link underlines for some sites.

@richtabor do you have any wisdom you can share that might help tip the scales here?

@cbirdsong
Copy link

cbirdsong commented Jul 26, 2024

Hypothetically, should this remove all the styles that ship in the default theme.json?

// I don't work, don't copy/paste me!
add_filter("wp_theme_json_data_default", function ($theme_json) {
	$data = $theme_json->get_data();

	// Remove default color palette.
	$data["settings"]["color"]["palette"]["default"] = [];

	// Remove default duotone.
	$data["settings"]["color"]["duotone"]["default"] = [];

	// Remove default gradients.
	$data["settings"]["color"]["gradients"]["default"] = [];

	// Remove default styles.
	$data["styles"] = [];

	// Update the theme data.
	$theme_json->update_with($data);

	return $theme_json;
});

In my testing, it does remove all the default colors/gradients/etc, but the styles defined here are still included in <style id="global-styles-inline-css">.

@aaronrobertshaw
Copy link
Contributor

@cbirdsong the theme json merge function leverages PHP's array_replace_recursive. That means providing an empty array for the incoming styles doesn't actually filter out any of the existing styles. The reason it works for the settings nodes is there has been custom logic implemented for presets.

I'm not sure that we can alter the logic for styles in a backwards-compatible manner.

To your main point though about the possibility of using a filter if you wish to opt out of these defaults for links. That should be achievable with something like this:

add_filter(
	'wp_theme_json_data_default',
	function ( $theme_json ) {
		$data = $theme_json->get_data();

		// Remove default link decoration styles.
		$data['styles'] = array(
			'elements' => array(
				'link' => array(
					'typography' => array(
						'textDecoration' => 'initial',
					),
					':hover'     => array(
						'typography' => array(
							'textDecoration' => 'initial',
						),
					),
				),
			),
		);

		// Update the theme data.
		$theme_json->update_with( $data );

		return $theme_json;
	},
);

@markhowellsmead does the above snippet help address your immediate need?

@markhowellsmead
Copy link
Author

@aaronrobertshaw The issue is more of a matter of principle right now, although I’ve already implemented a filtering solution to theme.json on a number of current projects.

@cbirdsong
Copy link

cbirdsong commented Jul 29, 2024

To your main point though about the possibility of using a filter if you wish to opt out of these defaults for links. That should be achievable with something like this:

Doing that outputs the following:

a:where(:not(.wp-element-button)) {
  text-decoration: initial;
}

a:where(:not(.wp-element-button)):hover {
  text-decoration: initial;
}

This still causes issues overriding default link styles.

Setting the contents to "" seems to work okay:

$data["styles"] = [
	"elements" => [
		"link" => [
			"typography" => [
				"textDecoration" => "",
			],
		],
	],
];

(I'm also not sure the :hover one is necessary - I don't see anything that needs to be overridden there)

However, looking at the bigger picture, in order to get rid of this:

.wp-element-button, .wp-block-button__link {
	background-color: #32373c;
	border-width: 0;
	color: #fff;
	font-family: inherit;
	font-size: inherit;
	line-height: inherit;
	padding: calc(0.667em + 2px) calc(1.333em + 2px);
	text-decoration: none;
}

I would have look at the default theme.json and then to do this:

$data["styles"] = [
	"elements" => [
		"button" => [
			"color" => [
				"color" => "",
				"background" => "",
			],
			"padding" => [
				"top" => "",
				"right" => "",
				"bottom" => "",
				"left" => "",
			],
			// etc, etc
		],
	],
];

Then I'd have to do that again on every site every time this file changes. Is there really no way to do this more systematically?

@cbirdsong
Copy link

cbirdsong commented Jul 29, 2024

@aaronrobertshaw How practical would it be to add another function that replaces instead of merges, like $theme_json->replace_with( $data );?

I'm sure there are edge cases I haven't thought of, but it's very unintuitive that passing an empty doesn't work.

I’ve already implemented a filtering solution to theme.json on a number of current projects.

Would be curious to see anything if you are able to share!

@aaronrobertshaw
Copy link
Contributor

The issue is more of a matter of principle right now

@markhowellsmead I definitely understand the argument 👍

Given sites might be relying on those default styles now, I'm leaning towards a pragmatic approach.

I appreciate it isn't ideal but the status quo doesn't impact existing sites that might depend on these defaults and the workaround to filter them out is available to address the concern expressed in this issue.

Should there be widespread consensus that the risk of removing these defaults is worth it, perhaps we can find a means of deprecating them or making them opt-in.

I'm not sure how that can be achieved in a backward compatible manner though. Maybe it would require a new theme.json version so the migration of older versions could inject the deprecated defaults.

This still causes issues overriding default link styles.

@cbirdsong, my apologies, I pasted in the wrong iteration of my test code. Thank you for correcting it to the empty string option.

Is there really no way to do this more systematically?

Taking a step back, rather than overriding values in the default theme.json data, it should be possible to completely discard the defaults.

The following might provide an option for you to explore:

add_filter(
	'wp_theme_json_data_default',
	function ( $theme_json ) {
		return new WP_Theme_JSON_Data(
			array( 'version' => WP_Theme_JSON::LATEST_SCHEMA ),
			'default'
		);
	}
);

You can also then set the priority for the filter to ensure it's run last etc.

How practical would it be to add another function that replaces instead of merges, like $theme_json->replace_with( $data );?

I'm honestly not sure. While it does provide some desired control it also means there's differing ways to modify that data that could make debugging issues in a very complex area much harder.

I'm not trying to suggest it is impossible but that all the pros and cons would need to be weighed to determine whether it is feasible and beneficial in the long run.

I'm sure there are edge cases I haven't thought of, but it's very unintuitive that passing an empty doesn't work.

Agreed. Once the presets were handled differently it certainly made the behaviour less predictable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants