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 Styles preview name: Character limit is not working correctly for some languages. #40463

Closed
shimotmk opened this issue Apr 20, 2022 · 9 comments · Fixed by #42975
Closed
Assignees
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@shimotmk
Copy link
Contributor

Description

Block Styles preview name is not working properly with character limit depending on language.
Please see the following screenshots.

style

https://github.com/WordPress/gutenberg/tree/trunk/packages/components/src/text#limit

Step-by-step reproduction instructions

  1. Refer to the following code to add a block style.
function twenty_twenty_two_register_block_styles() {
	// Hello,Hello,…
	register_block_style(
		'core/image',
		array(
			'name'  => 'twentytwentytwo-image-1',
			'label' => esc_html__( 'Hello,Hello,Hello', 'twentytwentyone' ),
		)
	);

	// Hola,Hola,Ho…
	register_block_style(
		'core/image',
		array(
			'name'  => 'twentytwentytwo-image-2',
			'label' => esc_html__( 'Hola,Hola,Hola', 'twentytwentyone' ),
		)
	);

	// こんにちは,こんにちは
	register_block_style(
		'core/image',
		array(
			'name'  => 'twentytwentytwo-image-3',
			'label' => esc_html__( 'こんにちは,こんにちは', 'twentytwentyone' ),
		)
	);

	// 你好,你好,你好,你好
	register_block_style(
		'core/image',
		array(
			'name'  => 'twentytwentytwo-image-4',
			'label' => esc_html__( '你好,你好,你好,你好', 'twentytwentyone' ),
		)
	);
}
add_action( 'init', 'twenty_twenty_two_register_block_styles' );
  1. Go to the block editor and place the image block.

  2. Open the side panel and look at the style preview.

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 5.9.4
  • Gutenberg 13.0.0
  • Twenty Twenty-Two

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

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Apr 20, 2022
@ramonjd ramonjd added the Internationalization (i18n) Issues or PRs related to internationalization efforts label May 4, 2022
@ramonjd
Copy link
Member

ramonjd commented May 4, 2022

I originally added the Text component in #34522 so that Block Style buttons could take advantage of updates to the Text or Truncate components. My original intention had been to apply the following CSS to the button:

text-overflow: ellipsis;
overflow: hidden;
white-space: nowrap;

This works everywhere

Screen Shot 2022-05-04 at 12 30 34 pm

I imagine however that the desire would be to solve this from within the Truncate component so that all instances of <Text /> would benefit.

The issue is that Chinese, Korean and Japanese characters are about twice the width of latin characters, so when specifying limit={ 12 } on <Text />, we'd really want something like 6 for Chinese, Korean and Japanese.

See:

Consumers of <Text /> and <Truncate /> could always detect the local and adjust their limits accordingly. Or we could detect CJK characters using a unicode regex.

Whatever happens, there's a risk of mismatching between the value of limit in <Text limit={ 12 } ... /> and the final length of the truncated string. For example, if a consumer of <Text /> specifies a limit while in a Chinese-first locale, what's the expectation? That the value of limit would be respected? I would think so.

One option is to try out a library (e.g., string-width) or equivalent to check the string length from within Truncate. Consumers of <Text /> and <Truncate /> could pass an optional maxWidth prop.

In a roll-our-own approach, we could calculate the width of the text using canvas's measureText. It only support pixels it seems.

Even more clever, we could infer the maximum width based on the width of a single latin character times the value of 'limit'.

If the incoming width of a Korean character string, for example, exceeds the width of the comparison string then we'd truncate the incoming string accordingly, e.g.,

/* Pseudo code - I HAVEN'T TESTED THIS IN ANY SHAPE OR FORM 😄 */

// The incoming prop value of limit
let limitValue = props.limit;

// Maybe we could put this behaviour behind a flag
if ( ! props.__experimentalUseCharWidth ) {
    // carry on as normal
    return;
}

// Get a base latin-character width.
const baseWidth = getTextWidth( 'z', ...fontStyleProps );

// Max width of the text based on the incoming limit.
const maxWidth = baseWidth * limit;

// Get the width of the incoming string
const incomingStringWidth = getTextWidth( incomingString ...fontStyleProps );

if ( incomingStringWidth > maxWidth ) {
    // Get the single character width from the incoming string.
    const incomingStringSingleCharWidth = getTextWidth( incomingString[0] ...fontStyleProps );

    // Get the max characters that can fit within the limit
    const adjustedCharacterLimit = Math.floor( maxWidth / incomingStringSingleCharWidth );

    // Adjusted limit value taking into account character width
    limitValue = str.substring( 0, adjustedCharacterLimit );
}

The caveat is that we'd have to explicitly state somewhere that the component expect limit values based on latin characters.

@shimotmk
Copy link
Contributor Author

shimotmk commented May 4, 2022

Hmmm.

In the first place, why did 『...』 to put a character limit on block style titles?
Previously, all the characters were displayed like this.

block-style

The design might be more beautiful if all the characters were available, but it might also be difficult to predict the style from the characters if the titles are not all visible initially.

Blocks and block patterns should not be limited to titles either.

Blocks
block

Block Patterns
block-patterns

I would prefer to see all the characters in the title instead of limiting the number of characters.

The screenshot is of WordPress 5.9.3 with the Gutenberg plugin disabled.

Translated with www.DeepL.com/Translator (free version)

@ramonjd
Copy link
Member

ramonjd commented May 4, 2022

In the first place, why did 『...』 to put a character limit on block style titles?

Good question! I think because the block style labels are now contained within buttons, which appear neater when they're all the same dimensions. I understand titles may be more intelligible when they're not truncated though.

The design might be more beautiful if all the characters were available

Possibly. If there were an alternative design that takes into account variable block style label lengths, and balances available space in the sidebar I'd be interested to see it.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label May 4, 2022
@shimotmk
Copy link
Contributor Author

shimotmk commented May 4, 2022

@ramonjd
Thanks for the quick comment.

I made this pull request for the style of title I think you're looking for, if you'd like to take a look.
#40807

@ramonjd
Copy link
Member

ramonjd commented May 4, 2022

cc @shaunandrews

@ndiego
Copy link
Member

ndiego commented May 17, 2022

A solution for this is needed, but unfortunately, we have run out of time to get this into 6.0 RC3 (which is actually being released as I type). Therefore, I have moved this issue and the corresponding PR to the 6.0.1 Project Board. Thank you for submitting this issue @shimotmk and for preparing the PR! 🙌

@shimotmk
Copy link
Contributor Author

Hi @ramonjd
I have reworked this branch to calculate the character limit from the character width.
Could you please take a look at it?
#41455

@mirka
Copy link
Member

mirka commented Jun 22, 2022

Related: #40331

@mirka
Copy link
Member

mirka commented Jun 22, 2022

Suggested a fix in #41455 (comment)

Repository owner moved this from Triage to Done in WordPress 6.0.x Editor Tasks Aug 9, 2022
@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
No open projects
Archived in project
6 participants