amp-script: decouple dev modes#27076
Conversation
|
Hey @ampproject/wg-caching, these files were changed: |
honeybadgerdontcare
left a comment
There was a problem hiding this comment.
validation changes look good
| <amp-script layout=container src="https://example.com/foo.js" development></amp-script> | ||
|
|
||
| <!-- Invalid: 'data-ampdevmode' attribute. --> | ||
| <amp-script layout=container src="https://example.com/foo.js" data-ampdevmode></amp-script> |
There was a problem hiding this comment.
What happens when data-ampdevmode="false" here? Is validation of other amp-script rules still skipped?
There was a problem hiding this comment.
Nothing special happens. The fact that data-ampdevmode on the root html node skips validation for all other rules is completely special cased in the validator. It doesn't work that way when on other elements.
One more idea though: I can see us wanting dev mode to be implied by putting it on the root as well. I say this because I can't imagine a situation where a publisher would want to suppress all errors on the page, but wouldn't also want to have amp-script in dev mode.
That would mean enabling amp-script's dev mode on either a root html data-ampdevmode or on the element itself.
There was a problem hiding this comment.
Sorry I meant with both html[data-ampdevmode] and amp-script[data-ampdevmode=false]. I'm guessing it would disable amp-script validation but enforce script hash.
Curious since I think it's been treated as a boolean attribute thus far rather than a value attribute (i.e. hasAttribute vs. getAttribute).
There was a problem hiding this comment.
html[data-ampdevmode]is checked viahasAttribute. If present, any provided value will invalidate the doc and trigger the error suppression.- I figured out how to mimic the same functionality via the protoascii configuration for amp-script (any value, including none, will be a validation error).
|
Something I hadn't thought about: if any developers had left behind a |
Internally we have a means to test for that. I'll run one now with the current rules in the PR. |
|
Seeing this - I'd personally favor the idea you proposed here:
that is, if "suppress validation" also means "not require hashes for any |
As discussed via chat, this now does exactly that 👍 |
| # This attribute should always be invalid. | ||
| name: "data-ampdevmode" | ||
| value: "false" | ||
| blacklisted_value_regex: "false" |
There was a problem hiding this comment.
Nice. Probably can use a different dummy value.
There was a problem hiding this comment.
Discussed offline. Another value could be used but I don't have any issue with this specific value. The comment makes it clear that the intention is for this to be always invalid. Perhaps we could make it more explicit by using a value like invalid-attribute or something. I would suggest providing a link to this PR/Issue in the comment.
This change demonstrates no ill affects to the validity of pages. |
* cl/299411284 Allow links with `tel` scheme in email spec * cl/300054759 github commit msg missing or malformed * cl/300575599 Revision bump for #27098 * cl/300578001 Revision bump for #27132 * cl/300590811 Revision bump for #27027 * cl/300593269 Revision bump for #27170 * cl/300596115 Revision bump for #27134 * cl/300598356 Revision bump for #27076 * cl/300599497 Revision bump for #26912 Co-authored-by: honeybadgerdontcare <sedano@google.com>
data-ampdevmodemust be on both root and the amp-script. Now, placing adata-ampdevmodeon either will do.developmentis outdated anddata-ampdevmodeshould be used.See #26341 (comment)
Todo:
data-ampdevmodeon anamp-scripttag needs to fail validation.