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

AMP should respect ⚡️ apart from #25990

Closed
tomayac opened this issue Dec 12, 2019 · 2 comments · Fixed by #26094
Closed

AMP should respect ⚡️ apart from #25990

tomayac opened this issue Dec 12, 2019 · 2 comments · Fixed by #26094

Comments

@tomayac
Copy link
Contributor

@tomayac tomayac commented Dec 12, 2019

Both ⚡️ and look the same, but they're different:

'⚡️'.length;
// 2
'⚡'.length;
// 1
'⚡'.charCodeAt(0) + ' ' + '⚡'.charCodeAt(1);
// "9889 NaN"
'⚡️'.charCodeAt(0) + ' ' + '⚡️'.charCodeAt(1);
// "9889 65039"

AMP respects the but rejects ⚡️, which is confusing since the macOS emoji picker creates ⚡️, that is, the representation with the Unicode Variation Selector-16.

amp-validator

The AMP validator checks for the presence of the representation without Variation Selector-16, but then replaces it with amp, the same happens on the AMP cache.

To avoid this confusion, AMP could potentially allow for both, ⚡️ and , and then just replace them with the textual attribute amp.

More background on this bug: https://blog.tomayac.com/2019/12/12/same-same-but-different-unicode-variation-selector-16/.

@samouri
Copy link
Member

@samouri samouri commented Dec 12, 2019

I'm a fan.

To avoid this confusion, AMP could potentially allow for both, ⚡️ and , and then just replace them with the textual attribute amp.

It would probably be best to accept all variants of the lightning bolt instead of just two of them. That could involved either checking against all 16 possibilities or normalizing the attributes by removing all variant selectors (at least for the purpose of the bolt check).

@honeybadgerdontcare
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare commented Dec 19, 2019

This should hit production by beginning of next week, but due to the holidays it may get delayed till beginning of next year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants