Try to integrate the amp-hulu tag

[dawehner]

This PR tries to integrate the amp-html by checking HULU embedds.
It seems to be though that we have to update the generator spec file first ...

[sebastianbenz]

Looks like there is a check missing whether the iframe is actually a hulu embed.

[dawehner]

@sebastianbenz Oh that is a good point. Thank you for the feedback.

To be honest I got stuck with the validation. Just this new pass, causes a disallowed amp-hulu tag. Given that I thought one has to update the spec file, by updating the forked amphtml repo, merging in the changes from google upstream, and finally building the validator-generated.php, see Lullabot/amphtml#1

This though causes some of those assertion errors:

52) AmpTest::testFiles with data set "tests/test-data/full-html/validator-amp-soundcloud.html" ('tests/test-data/full-html/val...d.html', true)
assert(): assert(!isset($this->tag_specs_by_dispatch[$dispatch_key])) failed

/Users/dawehner/Projects/amp/src/Validate/TagSpecDispatch.php:44
/Users/dawehner/Projects/amp/src/Validate/ParsedValidatorRules.php:120
/Users/dawehner/Projects/amp/src/Validate/ParsedValidatorRules.php:66
/Users/dawehner/Projects/amp/src/AMP.php:137
/Users/dawehner/Projects/amp/tests/AmpTest.php:32

As far as I see this problem can be seen in the spec file:

+  $o_12 = new TagSpec();
+  $o_12->tag_name = 'LINK';
+  $o_12->spec_name = 'link rel=';
+  $o_17 = new TagSpec();
+  $o_17->tag_name = 'LINK';
+  $o_17->spec_name = 'link rel=canonical';
+  $o_21 = new TagSpec();
+  $o_21->tag_name = 'LINK';
+  $o_21->spec_name = 'link rel=manifest';
+  $o_21->mandatory_parent = 'HEAD';

As far as I understand this should actually though use PropertySpecList and PropertySpec. I would be super happy about some pointer, but I'm still investigating.

[sebastianbenz]

@dawehner have a look at the vimeo sample: https://github.com/Lullabot/amp-library/blob/master/src/Pass/IframeVimeoTagTransformPass.php#L47

They match the iframe URL to a regex to check if the iframe is a vimeo embed. You'd need to do something similar for hulu.

[dawehner]

Thank you for letting me know.

This is blocked until we don't get the new generated validator code, because the one in HEAD doesn't yet support <amp-hulu>.

[dawehner]

@sebastianbenz It would be super cool if you could help me out onhttps://github.com/Lullabot/amphtml/pull/1

[dawehner]

Note: I managed to generate the validator and tweak it a little bit. It outputs all uppercase tags, which doesn't work with all the test coverage.

There are remaining failures left, but I guess we can fix them by tuning the validator a bit more? To be honest that sounds wrong.

[sebastianbenz]

Tuning the validator sounds wrong. What's needed to make it work?

Thanks btw for going through the pain of migrating to a new validator version! To be honest - I'm no real help here as I haven't been involved in developing amp-library.

[dawehner]

Tuning the validator sounds wrong

He, I did not expected anything else :)

What's needed to make it work?

The version which is in master of the amp-library doesn't include the 'amp-hulu' tag yet as allowed tag. The new exported validator though then contains uppercase tags, so I had to manually tune them to no longer do that. Still, there are a lot of test failures.

Do you think that manually putting in the amp-hulu tag and its JS would cut it?

[sebastianbenz]

Regarding the uppercase tags - is this something we can handle in the php validator runtime? The other test failures - can they be fixed by updating the amp-library transformations?

…

On Fri, Feb 17, 2017 at 4:57 PM Daniel Wehner ***@***.***> wrote: Tuning the validator sounds wrong He, I did not expected anything else :) What's needed to make it work? The version which is in master of the amp-library doesn't include the 'amp-hulu' tag yet as allowed tag. The new exported validator though then contains uppercase tags, so I had to manually tune them to no longer do that. Still, there are a lot of test failures. Do you think that manually putting in the amp-hulu tag and its JS would cut it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#162 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXOOAxxR6xAQydIWdLrD-KXJI2UF-fuks5rddFsgaJpZM4L66Fh> .

[dawehner]

Regarding the uppercase tags - is this something we can handle in the php
validator runtime?

I guess we could alter the initialized objects when we create them.

The other test failures - can they be fixed by updating the amp-library
transformations?

I haven't looked into them too much. One thing I noticed is that it no longer stripped out submit buttons.

[SGudbrandsson]

@dawehner I can see that changing the spec breaks a lot of functionality in the library.

When you generated the spec file, what version of the amphtml project did you use?

I think it'd be good to use the current running version of the AMP library - https://github.com/ampproject/amphtml/tree/1487358851767

That way, we know that the code is stable.

What I see pop up a lot is amp4ads, which is still a draft and should only be used for creatives (ads).

1. Amp4ads is expected in many places now. It shouldn't be expected since this is only for creatives.
   What is triggering this?
   Can we remove this trigger?
2. The tag 'style amp-custom' appears more than once in the document. is no longer triggered
   The code needs to remove the second amp-custom
   File: tests/test-data/full-html/duplicate_unique_tags_and_wrong_parents.html
3. The amp-youtube javascript isn't being included in tests/test-data/full-html/mandatory_dimensions.html.
   It needs to be included if there is amp-youtube tags in the document.

TL;DR

I think the biggest win for you would be to start by fixing the amp4ads. That's going to remove most of the failures, allowing you to focus on what's left.

[dawehner]

Thank you for the guidance!

I think it'd be good to use the current running version of the AMP library - https://github.com/ampproject/amphtml/tree/1487358851767

This is what I did. I merged the master of amphtml into the fork from lullabot, see dawehner/amphtml@bc0812e

I'll try to work on the failures. Thank you

[dawehner]

I fixed the amp4ads validation as well as many other instances of wrong expactations. We are down to 16 failing tests now.

[dawehner]

I fixed a couple of more failures. There are a couple of items, like the youtube failures which are at the moment a bit challenging. There are other ones I'm not sure what to do about them, like nested noscript tags. That sounds like a super edge case feature for me.

[renzit]

How i can help for get this done? i think we all need re make the validator.php for other integrations and contribute our needs of new components and the validator branch is very far behind master of amphtml. If we can solve all the issues here or in a new issue it would be very nice to keep updated this library. @sigginet or @sebastianbenz can we talk about it?

[SGudbrandsson]

I'm looking through this..

1. A second <style amp-custom> is being passed through in the <head>. It should not get passed through. See https://www.ampproject.org/docs/reference/spec#cust
2. <input type="submit" value="submit"> is being passed through. It should not get passed through unless we include amp-form js. See https://www.ampproject.org/docs/reference/components/dynamic/amp-form
3. Some cleanup needs to happen in the output because of the validator update. (new links, changed output, etc.)
4. There are some youtube validation errors. We need them to fail correctly and succeed correctly. I don't have time to check them out right now (I'm on a holiday)
5. Nested noscript tags aren't removed (they should be)
6. JS script tags aren't removed anymore. This seems to be an issue with the amp4ads stuff .. this library doesn't support amp4ads .. we would have to create a specific amp4ads PR and an option to enable it.

I need to head out now, but I hope this helps a bit to move this PR forward :)

[dawehner]

Sorry for not continueing the work here, other priorities got set. I hope I can get back to it