WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.58k stars 4.23k forks source link

Link styles: underline enforcement overrides browser preferences #63647

Open markhowellsmead opened 4 months ago

markhowellsmead commented 4 months ago

Description

The definition at https://github.com/WordPress/gutenberg/blob/d1211ff7de85c66d45ad144f8c80323d875f964e/lib/theme.json#L360-L364 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

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

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

aaronrobertshaw commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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.