✨amp-experiment 1.0: Allowed all class changes #22679
Conversation
| @@ -68,6 +69,9 @@ export const attributeMutationAllowList = { | |||
| 'style': { | |||
| '*': new DefaultStyleAllowedAttributeMutationEntry(), | |||
| }, | |||
| 'class': { | |||
There was a problem hiding this comment.
I believe there're a bunch of internal class names that are only reserved for AMP only. For example everything starts with i-amphtml-*. Could you please double check? Thank you
There was a problem hiding this comment.
Oh my you are completely correct, I was only thinking about security not validation 😂 Thank you!
Note to self: CSS Validation is in validator/validator-main.protoascii. And we should try to piggy back off of an existing sanitizer in AMP.
|
@zhouyx Made requested changes, this is good to go! 😄 |
|
So, this PR will be merged once I get a response from the Runtime team ( @jridgewell ) on whether we need class restrictions on certain elements (especially AMP Components) other than But timezones so let's figure this out Monday 😄 |
|
I think just blocking |
|
|
||
| // Don't allow the .i-amphtml class | ||
| // See `validator/validator-main.protoascii` | ||
| if (value.match(/(^|\\W)i-amphtml-/)) { |
There was a problem hiding this comment.
Can we declare a constant for this instead of inlining the regex
There was a problem hiding this comment.
Talked offline, since we are having a refactor, and the current code is like this. We went ahead and added some more comments, and will address this in the refactor 😄
relates to #20225
relates to #21705
Pretty small change, but allows any class to be applied as an attribute mutation 😄
Example