Adding id as a valid attribute for amp-iframe #1147
Conversation
| @@ -135,6 +136,7 @@ private function filter_attributes( $attributes ) { | |||
| case 'sandbox': | |||
| case 'class': | |||
| case 'sizes': | |||
| case 'id': | |||
There was a problem hiding this comment.
Instead of just allowing id we should probably obtain all attributes that are allowed on amp-iframe, including data-* ones. For non-data-* attributes, the list of attributes can be obtained via AMP_Allowed_Tags_Generated::get_allowed_tag( 'amp-iframe' ). The list of globally-allowed attributes can be obtained via AMP_Allowed_Tags_Generated::get_allowed_attributes()...
On second thought, we it would be better to just copy all of the attributes over from the iframe because then the whitelist sanitizer can then report when invalid attributes are present, as opposed to silently stripping them out.
…er will do the job - remove scrolling from allowed attribute in amp-iframe
|
Following your advice I am just normalizing specific attributes in amp-iframe and letting AMP_Tag_And_Attribute_Sanitizer do the rest. |
| 'scrolling' => array( | ||
| 'value_regex' => 'auto|yes|no', | ||
| ), | ||
|
|
There was a problem hiding this comment.
Why is this change here? Any changes to this generated file should be done via amphtml-update.sh
|
@westonruter I believe it's ready to be merge now 👍 |
| @@ -161,7 +153,11 @@ private function filter_attributes( $attributes ) { | |||
| } | |||
| break; | |||
|
|
|||
| case 'scrolling': | |||
| break; | |||
There was a problem hiding this comment.
@juanchaur1 I don't think this is right because scrolling is allowed on amp-iframe:
In any case, it would be better to let the whitelist sanitizer remove it so that a validation error would be reported.
The id attribute of any iframe is being removed in the amp-iframe-sanitizer. I believed it's save to pass it as it's done with the classes, size, etc.