WordPress / gutenberg

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

Allow html in pagination labels #60878

Open huubl opened 6 months ago

huubl commented 6 months ago

What problem does this address?

The current implementation of the core/query-pagination-previous and core/query-pagination-next blocks in Gutenberg use esc_html to escape the label text, which doesn't allow any HTML tags. This limits the freedom to set a label as Previous <span class="hide-on-mobile">Page</span>:

Right now this doesn't work:

function render_block_data_pagination($parsed_block)
{
    if ('core/query-pagination-previous' === $parsed_block['blockName']) {
        $parsed_block['attrs']['label'] = 'Previous <span class="hide-on-mobile">Page</span>';
    }

    return $parsed_block;
}
add_filter('render_block_data', 'render_block_data_pagination', 10, 3);

What is your proposed solution?

Switch from esc_html to wp_kses for escaping the label text, allowing the <span> tag, and maybe some other tags. This would give developers more freedom. Here's an example of how the code might look:

$allowed_html = array(
    'span' => array()
);

$label_text = isset($attributes['label']) && !empty($attributes['label'])
    ? wp_kses($attributes['label'], $allowed_html)
    : $default_label;
amitraj2203 commented 5 months ago

Hi @t-hamano and @huubl,

I tried to replicate it and this issue still exists, I would like to contribute on it. Regarding $allowed_html, which other tags should we include here apart from span?

t-hamano commented 5 months ago

Thanks for the report.

I think the following two points are important regarding this issue.

If we decide to allow arbitrary HTML elements, we'll need to discuss which inline formats to add back.

Personally, I think there is no problem with keeping the current specifications as the markup output by the filter below can be modified.

function replace_pagination_content( $block_content, $block ) {
    if ( 'core/query-pagination-previous' === $block['blockName'] ) {
        $block_content = preg_replace( '/<a(.*?)>.*?<\/a>/', '<a$1>Previous <span class="hide-on-mobile">Page</span></a>', $block_content );
    }
    return $block_content;
}

add_filter( 'render_block', 'replace_pagination_content', 10, 2 );
huubl commented 1 month ago

Thanks for the report.

I think the following two points are important regarding this issue.

  • This block used to support two inline formats, bold and italic, but the inline format was intentionally disabled in #28831.
  • Since this block is an a element, it cannot allow all HTML elements.

If we decide to allow arbitrary HTML elements, we'll need to discuss which inline formats to add back.

Personally, I think there is no problem with keeping the current specifications as the markup output by the filter below can be modified.

function replace_pagination_content( $block_content, $block ) {
  if ( 'core/query-pagination-previous' === $block['blockName'] ) {
      $block_content = preg_replace( '/<a(.*?)>.*?<\/a>/', '<a$1>Previous <span class="hide-on-mobile">Page</span></a>', $block_content );
  }
  return $block_content;
}

add_filter( 'render_block', 'replace_pagination_content', 10, 2 );

Hi @t-hamano, sorry a bit late, but thank you for your reply.

It seems <strong> and <em> are removed because these tags provide semantic meaning and probably should not be used in pagination labels. Additionally, bold and italic styling can be set at the block level.

Allowing the <span> tag, however, doesn't impact accessibility.

I believe it's better to add wp_kses as using regex is more error-prone. What do you think?

I'm not sure if more tags should be included besides <span>. Maybe the <i> tag to allow, for example, Font Awesome icons?