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

transition:name can't use Chinese、Japanese and Korean, etc. #9803

Closed
1 task
liruifengv opened this issue Jan 24, 2024 · 14 comments · Fixed by #9822
Closed
1 task

transition:name can't use Chinese、Japanese and Korean, etc. #9803

liruifengv opened this issue Jan 24, 2024 · 14 comments · Fixed by #9822
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope)

Comments

@liruifengv
Copy link
Member

Astro Info

Astro                    v4.1.0
Node                     v20.9.0
System                   Windows (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             @astrojs/tailwind
                         @astrojs/react
                         @astrojs/sitemap
                         astro-expressive-code
                         @astrojs/mdx

If this issue only occurs in one browser, which browser is a problem?

Chrome

Describe the Bug

Set a element transition:name to transition:name={name}
The name is Chinese text.

Read Astro code:

// Ensure animationName is a valid CSS identifier
function toValidIdent(name: string): string {
	return name.replace(/[^a-zA-Z0-9\-\_]/g, '_').replace(/^\_+|\_+$/g, '');
}

This will cause the Chinese name to be replaced with empty, resulting in multiple duplicates of the name.

image

My website as example:

https://liruifengv.com/tags/

image

I think the same will be true for Japanese and Korean, etc.

What's the expected result?

Expect Chinese to be used correctly as transition:name.

CSS identifiers can be Chinese、Japanese and Korean, etc.

Link to Minimal Reproducible Example

https://liruifengv.com/tags/

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Jan 24, 2024
@liruifengv liruifengv changed the title transition:name can't use Chinese transition:name can't use Chinese、Japanese and Korean, etc. Jan 24, 2024
@liruifengv
Copy link
Member Author

This is causing me a lot of problems and causing my site animations to fail.

I checked the commit history and found that this PR did a replacement process for transition:name.

Why do this, I think it's a break change.

#8207

@martrapp martrapp added - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs triage Issue needs to be triaged - P3: minor bug An edge case that only affects very specific usage (priority) labels Jan 24, 2024
@delucis
Copy link
Member

delucis commented Jan 24, 2024

It looks like the replace logic there was intended to ensure valid transition names as those are supposed to be CSS <custom-ident> strings, but didn’t account for Unicode character support (which requires escaping):

// Ensure animationName is a valid CSS identifier
function toValidIdent(name: string): string {
return name.replace(/[^a-zA-Z0-9\-\_]/g, '_').replace(/^\_+|\_+$/g, '');
}

@martrapp
Copy link
Member

martrapp commented Jan 24, 2024

As you can see, I'm having trouble choosing the right label here. This is definitely not an edge case, but it also does not contradict the documented behavior.

The browser only accepts a limited range of characters for view-transition-names.
The only valid unescaped characters are A-Z, a-z, 0-9, and \-_.

@liruifengv
Copy link
Member Author

Maybe we can encode it using encodeURIComponent before.

@martrapp
Copy link
Member

martrapp commented Jan 24, 2024

Even if the intention is good, it's not so nice to change the characters without notice :-)
We also replace "\" which would be a legal character and necessary for escaping.
The correct way to define 开源 as a view transition name would be "\5F00\6E90" which is currently not possible due to the missing \)

Maybe we can do better on auto escaping, but I think we should definitively issue warning when changing names.

@liruifengv
Copy link
Member Author

Yeah. We need give a warning when change the user passed value.

@martrapp
Copy link
Member

martrapp commented Jan 24, 2024

Maybe we can encode it using encodeURIComponent before.

This would yield %E5%BC%80%E6%BA%90 which is also illegal, but I know what you mean and would agree to auto-escaping if we find a robust way to do so.

@delucis
Copy link
Member

delucis commented Jan 24, 2024

I took a quick look and didn’t immediately spot a definitive CSS ident escaping tool, but yes I’d be +1 on auto-escaping if possible and definitely warning in case someone is using the name elsewhere in their CSS.

@martrapp martrapp self-assigned this Jan 24, 2024
@martrapp
Copy link
Member

This might be a bit bumpy, but I'll take a look.
Or will someone else volunteer?

@liruifengv
Copy link
Member Author

I'll also take a look.

@ccbikai
Copy link

ccbikai commented Jan 24, 2024

Is it possible to use encodeURIComponent('中文').replaceAll('%', '_')?

Although longer, they are all legal characters

@martrapp
Copy link
Member

Is it possible to use encodeURIComponent('中文').replaceAll('%', '_')?

true, but it would be rather inconvenient to decode. When using \5F00\6E90, you get the original text in the browsers development tools:
image

@lilnasy lilnasy added - P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope) labels Jan 24, 2024
@liruifengv
Copy link
Member Author

I just made a PR. Can you help me with it? @delucis @martrapp

#9822

@martrapp martrapp removed their assignment Jan 25, 2024
@martrapp
Copy link
Member

Great! Thanks a lot, happy to do so!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants