Enable amp-autocomplete in AMP4Email with doc level opt-in #27174

caroqliu · 2020-03-10T15:30:26Z

Related to: #24881

Requires data-amp-autocomplete-opt-in in the document <html> tag for email documents to mimic experiment language
Adds validation rules

amp-owners-bot · 2020-03-10T15:30:34Z

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
validator/engine/validator_test.js
validator/testdata/amp4email_feature_tests/amp-autocomplete.html
validator/testdata/amp4email_feature_tests/amp-autocomplete.out
validator/validator-main.protoascii

extensions/amp-autocomplete/validator-amp-autocomplete.protoascii

validator/testdata/amp4email_feature_tests/amp-autocomplete.out

extensions/amp-autocomplete/0.1/amp-autocomplete.js

Gregable · 2020-03-16T17:23:56Z

I'm OK with this change for the validator. However, I'd like to loop in @kristoferbaxter on the addition of a new attribute to the <html> tag. He expressed some concerns about this recently, I think he has more context than myself.

zhangsu · 2020-03-18T15:05:03Z

Just one thought (sorry that it came late): what documentation do we have to avoid confusing developers when they discover that they can start using amp-autocomplete in the playground but nothing seems to work, both in the playground and in any email client?

caroqliu · 2020-03-19T14:39:59Z

@zhangsu Thanks for bringing this up. Talked with @sebastianbenz and there is currently no support for format-specific experimental flagging. For now I've added a warning at the top of the documentation file that will only be displayed for email.

sebastianbenz

Doc update LGTM

zhangsu

Thanks! @bill97819 FYI

kristoferbaxter · 2020-03-19T19:29:04Z

When adding new attributes that are not part of the existing HTML specification, we should prefix them to avoid making it harder for the HTML specification to change.

In this case we should look at using a data- attribute prefix, as described here.

kristoferbaxter

Moving to a data- attribute will avoid making it harder for the HTML specification to change.

caroqliu · 2020-03-19T19:41:43Z

@kristoferbaxter @Gregable Would it be possible to use a data- prefixed attribute that is explicitly disallowed in the validator? All data-* attributes are validated and this attribute should not be.

dreamofabear · 2020-03-19T20:06:02Z

@samouri did this recently in amp-script:

amphtml/extensions/amp-script/validator-amp-script.protoascii

Lines 196 to 199 in 0acc235

    
           # This attribute should always be invalid. See https://github.com/ampproject/amphtml/pull/27076 
        
           name: "data-ampdevmode" 
        
           value: "false" 
        
           blacklisted_value_regex: "false"

Gregable · 2020-03-26T17:14:14Z

validator/validator-main.protoascii

+    # This attribute should always be invalid. See https://github.com/ampproject/amphtml/pull/27174
+    name: "data-amp-autocomplete-opt-in"
+    value: "false"
+    blacklisted_value_regex: "false"


This is a clever solution. Can you leave a comment here explaining what this is doing and why, because I suspect it'll be confusing to someone reading it in isolation.

This amp-autocomplete-opt-in only applies to AMP4email even though it is defined on the html element shared across the various AMP flavors, correct?

It is only relevant to the AMP4Email format, yes.

samouri · 2020-03-26T18:20:22Z

extensions/amp-autocomplete/0.1/amp-autocomplete.js

@@ -215,6 +216,14 @@ export class AmpAutocomplete extends AMP.BaseElement {

  /** @override */
  buildCallback() {
+    const doc = this.element.ownerDocument;
+    const isEmail = doc && isAmp4Email(doc);


When will doc be falsy?

It could be null if this.element is itself a document. The syntax is more for the signature of isAmp4Email, so we would still need a type assertion:
isAmp4Email(/** @type {!Document} */ (doc))

Requested changes have been addressed

caroqliu added 2 commits March 10, 2020 11:19

Add doc-level opt-in flag

eed0ff5

Add validation rules and tests

cc91e2e

googlebot added the cla: yes label Mar 10, 2020

amp-owners-bot bot requested a review from tomokinat March 10, 2020 15:30

caroqliu requested a review from dreamofabear March 10, 2020 15:31

Rename doc level flag to "amp-autocomplete-opt-in"

3575b80

zhangsu reviewed Mar 10, 2020

View reviewed changes

extensions/amp-autocomplete/validator-amp-autocomplete.protoascii Outdated Show resolved Hide resolved

extensions/amp-autocomplete/validator-amp-autocomplete.protoascii Outdated Show resolved Hide resolved

Gregable requested changes Mar 10, 2020

View reviewed changes

validator/testdata/amp4email_feature_tests/amp-autocomplete.out Outdated Show resolved Hide resolved

extensions/amp-autocomplete/0.1/amp-autocomplete.js Outdated Show resolved Hide resolved

Gregable added the WG: caching label Mar 10, 2020

caroqliu added 2 commits March 10, 2020 12:37

s/list/autocomplete

c899a37

Remove duplicate allowance of "template"

08f855e

caroqliu requested a review from Gregable March 11, 2020 15:37

caroqliu added 2 commits March 12, 2020 09:46

Update out

e4fc7fd

Allow amp-autocomplete in email validator test

3045ea9

Gregable requested a review from kristoferbaxter March 16, 2020 17:24

Gregable added the Validator: Approved label Mar 16, 2020

caroqliu requested a review from zhangsu March 18, 2020 14:36

caroqliu added 2 commits March 19, 2020 10:11

Update assertion message.

24e0ccd

Add warning to email documentation

191c4d5

caroqliu requested a review from sebastianbenz March 19, 2020 14:43

sebastianbenz approved these changes Mar 19, 2020

View reviewed changes

zhangsu approved these changes Mar 19, 2020

View reviewed changes

dreamofabear approved these changes Mar 19, 2020

View reviewed changes

kristoferbaxter previously requested changes Mar 19, 2020

View reviewed changes

data- prefix opt-in attr

14ed163

caroqliu requested a review from kristoferbaxter March 20, 2020 13:45

Gregable reviewed Mar 26, 2020

View reviewed changes

Add comment explaining validator rules

b4d87b4

caroqliu requested a review from Gregable March 26, 2020 18:20

samouri reviewed Mar 26, 2020

View reviewed changes

Gregable approved these changes Mar 26, 2020

View reviewed changes

caroqliu merged commit d5b64c4 into ampproject:master Mar 27, 2020

banaag added a commit to banaag/amphtml that referenced this pull request Mar 31, 2020

cl/303432005 Revision bump for ampproject#27174

039da88

banaag mentioned this pull request Mar 31, 2020

cl/303432005 Revision bump for #27174 #27506

Merged

banaag added a commit that referenced this pull request Mar 31, 2020

cl/303432005 Revision bump for #27174 (#27506)

06ff72e

ampprojectbot added the PR use: In Beta / Experimental label Mar 31, 2020

westonruter mentioned this pull request Apr 20, 2020

Update allowed tags/attributes from spec in amphtml 2004142326360 ampproject/amp-wp#4548

Merged

5 tasks

caroqliu mentioned this pull request Jun 11, 2021

Allow amp-autocomplete in email format by default #34848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable amp-autocomplete in AMP4Email with doc level opt-in #27174

Enable amp-autocomplete in AMP4Email with doc level opt-in #27174

caroqliu commented Mar 10, 2020 •

edited

Loading

amp-owners-bot bot commented Mar 10, 2020 •

edited

Loading

Gregable commented Mar 16, 2020

zhangsu commented Mar 18, 2020

caroqliu commented Mar 19, 2020

sebastianbenz left a comment

zhangsu left a comment

kristoferbaxter commented Mar 19, 2020

kristoferbaxter left a comment

caroqliu commented Mar 19, 2020

dreamofabear commented Mar 19, 2020

Gregable Mar 26, 2020

westonruter Apr 20, 2020

caroqliu Apr 20, 2020

samouri Mar 26, 2020

caroqliu Mar 26, 2020 •

edited

Loading

Enable amp-autocomplete in AMP4Email with doc level opt-in #27174

Enable amp-autocomplete in AMP4Email with doc level opt-in #27174

Conversation

caroqliu commented Mar 10, 2020 • edited Loading

amp-owners-bot bot commented Mar 10, 2020 • edited Loading

Gregable commented Mar 16, 2020

zhangsu commented Mar 18, 2020

caroqliu commented Mar 19, 2020

sebastianbenz left a comment

Choose a reason for hiding this comment

zhangsu left a comment

Choose a reason for hiding this comment

kristoferbaxter commented Mar 19, 2020

kristoferbaxter left a comment

Choose a reason for hiding this comment

caroqliu commented Mar 19, 2020

dreamofabear commented Mar 19, 2020

Gregable Mar 26, 2020

Choose a reason for hiding this comment

westonruter Apr 20, 2020

Choose a reason for hiding this comment

caroqliu Apr 20, 2020

Choose a reason for hiding this comment

samouri Mar 26, 2020

Choose a reason for hiding this comment

caroqliu Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

caroqliu commented Mar 10, 2020 •

edited

Loading

amp-owners-bot bot commented Mar 10, 2020 •

edited

Loading

caroqliu Mar 26, 2020 •

edited

Loading