Consider allowing input[type=button] and input[type=image]

westonruter commented 4 years ago

I'm coming across some WordPress themes that are using input[type=button] and input[type=image]. Are there specific reasons for why are not allowed in AMP?

https://github.com/ampproject/amphtml/blob/a81762bd82cac406c3341a02908e45ed9448c560/validator/validator-main.protoascii#L4509-L4517

I'm wanting to know if either AMP should allow them or otherwise if the WordPress AMP plugin should try to convert them to <button> and <button><amp-img></amp-img></button> respectively.

westonruter commented 4 years ago

/cc @ampproject/wg-security-privacy per @honeybadgerdontcare in Slack.

westonruter commented 4 years ago

Per @Gregable in the Slack thread:

My memory may be completely corrupt here, but I vaguely recall something about wanting to avoid someone creating a document which looked like a Google login page to be used for Phishing.

It seems that with the dynamic elements that AMP supports these days, that this ship sailed long ago. Therefore, if my memory is correct, then this can be relaxed in the validator.

The type=password is what makes me think of this, but I really could be wrong.

dvoytenko commented 4 years ago

The issue with <input type="image">, AFAIR, that we can't control its loading time it jumps the layout once loaded. And at this point we can't even apply layout rules to it. Is there a discernable difference between using <input type=image src="..."> vs <input type=submit><amp-img src="..."></input>?

westonruter commented 4 years ago

An input[type=image] can have a width/height, so that would prevent layout shifting.

The only thing that input[type=image] doesn't support is loading=lazy, so that would prevent deferring of the image loading.

Is there a discernable difference between using <input type=image src="..."> vs <input type=submit><amp-img src="..."></input>?

With styling it doesn't appear so.

<code>input[type=image]</code> 👉
<input type="image" src="https://amp.dev/static/img/favicon.png" width="50" height="50">

<button style="border: none; padding: 0; margin: 0; background: transparent; cursor: pointer; -webkit-appearance: none; -moz-appearance: none;"><amp-img src="https://amp.dev/static/img/favicon.png" width="50" height="50"></amp-img></button>
👈 <code>button &gt; amp-img</code>

So transforming input[type=image] to button > amp-img seems like a viable option if it remains invalid AMP.

Gregable commented 4 years ago

Sounds like the conclusion is:

Add button / file to the supported list.
Modify the AMP transformers to rewrite images found in <input src> using the same logic as <img src>.
After the above has been released, create a new validator rule for <input type=image> that also requires src, width, and height.

It doesn't sound like there is demand for type=password, so for simplicity perhaps continue to disallow it until there is a use case.

cc @ssantosms to be aware of the image rewrite.

cc @ampproject/wg-amp4email for thoughts on which of these should apply to the email spec.

@lannka for thoughts on which of these should apply to the ads spec.

@choumx for thoughts on which of these should apply to the actions spec.

dvoytenko commented 4 years ago

After the above has been released, create a new validator rule for <input type=image> that also requires src, width, and `height.

Yep. Exactly. As long as we rewrite the src, we can allow <input type=image> with the required width and height values.

Gregable commented 4 years ago

One other FYI. In addition to not supporting loading=lazy, we also lose support for srcset (at least I didn't find any reference to <input srcset>). The AMP cache applies this automagically to images. The cache and transformers will need to simply rewrite src only. I think this is a reasonable tradeoff.

Gregable commented 4 years ago

The cache transforms were filed internally at Google as b/148083520.

dvoytenko commented 4 years ago

@Gregable, is there no srcset in <input type=image>?

Gregable commented 4 years ago

I didn't test it, but quick reference checks didn't see a reference to it, such as here, here, or here.

dvoytenko commented 4 years ago

I suppose it doesn't make me like <input type=image> anymore than I already didn't like it. But I just checked, it's not deprecated, so let's do.

fstanis commented 4 years ago

Some quick thoughts from email PoV:

type="button" sounds OK.
type="image" would add another resource that can potentially leak requests and that'd need to be proxied, unless there's a strong reason, this is probably not worth it.
type="password" and type="file" aren't currently allowed in any capacity (as emails only support forms with action-xhr and not action) for different reasons compared to websites, so we'd probably need a separate discussion on whether to change this.

This is just my opinion. We'll discuss in the next WG meeting and report back.

Unrelated to that, does amp-form actually support serializing files when making an XHR? We may need to confirm that before enabling it. I suspect it'd just not include file data into the POST request, but haven't checked.

Gregable commented 4 years ago

@fstanis if possible, it would be nice to support proxying this for consistency in formats as much as we can. It should follow the exact same code path as <img src>, which makes me think implementation would be straightforward.

fstanis commented 4 years ago

I agree, but the issue is that:

We need to make sure all email clients supporting AMP are (1) aware of this and (2) are appropriately handling it. We can use the WG meeting to coordinate this effort, but I don't want to block this issue on it, so it's best to just skip email support for now.
More things to proxy raises the bar to support AMP for new email clients.

Gregable commented 4 years ago

@fstanis I don't understand your earlier comment on type=password and how it relates to action vs. action-xhr. My understanding is that the only difference between type=text and type=password is the way that the browser displays the characters as the user enters them, where password obscures the input.

fstanis commented 4 years ago

Oh, I just misinterpreted our docs when I read that:

I thought this meant type=password is OK for <form method=POST action> (but not <form method=POST action-xhr>), but later realized it's the opposite (and also that <form method=POST action> is actually not allowed anyway).

TLDR: Ignore my previous comment regarding password, it's not allowed in emails for different reasons (phishing concerns).

westonruter commented 4 years ago

Another example of input[type=image] causing an issue. The PayPal donation form snippet apparently uses this, causing an unexpected result in AMP. Support forum topic: https://wordpress.org/support/topic/paypal-form-in-reader-mode-not-visible/

Gregable commented 4 years ago

FYI, support for input[type=button] was added in https://github.com/ampproject/amphtml/pull/26845

Aside: I've actually noticed a small number of documents that appear to have autoconverted input[type=button] to button, likely due to this. This is error-prone because input[type=button] has no closing HTML tag, while button does, so some of these conversions have broken the page in a major way with unclosed button tags.

input[type=image] still requires additional work, and I'm disappointed that it will end up a allowed in AMP, but not AMP Email.

input[type=password] is not considered acceptable for AMP Email due to phishing concerns. There are also no examples shared here where there is demand for this, it appears mentioned only out of completeness. I'm inclined to leave this one disallowed in all formats for the time being.

westonruter commented 4 years ago

input[type=password] is not considered acceptable for AMP Email due to phishing concerns. There are also no examples shared here where there is demand for this, it appears mentioned only out of completeness. I'm inclined to leave this one disallowed in all formats for the time being.

Disallowed in all formats? It is currently allowed inside form[method=post] for the AMP format exclusively, right?

https://github.com/ampproject/amphtml/blob/d32bf3b3751b1184b1d46b20e67a3f569cd93313/validator/validator-main.protoascii#L4558-L4576

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

westonruter commented 3 years ago

This is still relevant.

molnarg commented 3 years ago

type=password was enabled for non-email formats in #14747 (which links to a security discussion doc I don't have access to yet).

In general, if a format allows using <amp-script>, then it doesn't make much sense (it's futile) from a security PoV to disallow password fields. And if a format disallows amp-script, then the question boils down to what context are the AMP documents displayed in (how much we care about phishing, and how much a password field helps phishers) - in the email case, we should definitely keep it disallowed.

Gregable commented 2 years ago

Split off #37443 to track work on input[type=image], since there is a lot of discussion here and that should simplify the feature request.

The other part here input[type=password] which has already been allowed for regular AMP, but will not be allowed for email. So, I'm closing this request.

ampproject / amphtml

Consider allowing input[type=button] and input[type=image] #26364