Roll forward "Delegate trust check... (#21553)" #21653
Conversation
)" (ampproject#21563)" This reverts commit d96498b.
dreamofabear
changed the title
|
/to @alabiaga |
| * @private | ||
| */ | ||
| function isAmpTagName(lowercaseTagName) { | ||
| return lowercaseTagName.substring(0, 4) === 'amp-'; |
There was a problem hiding this comment.
In the second usage we only have a string (attribute value).
There was a problem hiding this comment.
Extract out of isAmpElementTagName? Either way, you're missing amp-body and amp-sticky-ad-top-padding, which are not AMP elements.
There was a problem hiding this comment.
Semantics for actions are a bit more complex/confusing e.g. <script id="amp-access"> is not an AMP element but we allow invoking actions on it, so I'd rather not conflate the two by calling isAmpElement() or similar.
Maybe it should be called "isAmpTarget" rather than "isAmpTagName" though.
Tangent: I actually think isAmpElement's explicit check for amp-body etc. isn't great. We should probably duck-type check instead e.g. !!element.isLayoutSupported.
There was a problem hiding this comment.
Tangent: I actually think isAmpElement's explicit check for amp-body etc. isn't great. We should probably duck-type check instead e.g. !!element.isLayoutSupported.
It's intentional because we can't trust that the element has been upgraded already.
Most action handlers are registered on the custom element. This is only for non-CE extensions e.g. form, access. See the comment I added on |
Roll forward #21553 with type and test fixes.