Fix handling of child_tag_name_oneof constraint in tag spec

[westonruter]

In reviewing the Newspack theme (https://github.com/Automattic/newspack-theme/pull/120/files#r309371902), I was seeing something very strange in regards to markup like:

<amp-sidebar id="mobile-sidebar" layout="nodisplay" side="right">
	<nav>
		<ul>
			<li><a href="https://example.com/"></a></li>
		</ul>
		<div class="some-div">
			...
		</div>
	</nav>
</amp-sidebar>

The AMP plugin was marking it as invalid as if it was matching the amp-sidebar > nav tag spec, specifically due to the child_tags constraint:

  child_tags: {
    mandatory_num_child_tags: 1
    child_tag_name_oneof: "OL"
    child_tag_name_oneof: "UL"
  }

But this made no sense because this tag spec should not be considered at all because the nav does not have the toolbar or toolbar-target attributes which are also part of that tag spec:

  attrs: {
    name: "toolbar"
    mandatory: true
    # This tagspec sometimes produces errors for non-sidebar NAVs. This
    # dispatch key helps with this somewhat.
    dispatch_key: NAME_DISPATCH
  }
  attrs: {
    name: "toolbar-target"
    mandatory: true
  }

The reason turns out to be that the DOM was being mutated while checking for candidate tag specs to validate against a given node.

When beginning to process_node, we have to gather up a list of candidate tag specs to validate the node against:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 402 to 415 in 1a3646e

	/*
	* Compile a list of rule_specs to validate for this node
	* based on tag name of the node.
	*/
	$rule_spec_list_to_validate = [];
	$rule_spec_list = [];
	if ( isset( $this->allowed_tags[ $node->nodeName ] ) ) {
	$rule_spec_list = $this->allowed_tags[ $node->nodeName ];
	}
	foreach ( $rule_spec_list as $id => $rule_spec ) {
	if ( $this->validate_tag_spec_for_node( $node, $rule_spec[ AMP_Rule_Spec::TAG_SPEC ] ) ) {
	$rule_spec_list_to_validate[ $id ] = $this->get_rule_spec_list_to_validate( $node, $rule_spec );
	}
	}

This loop is only checking for candidate tag specs; it should not be doing any sanitization. Nevertheless, the validate_tag_spec_for_node() method call there was doing this in its last line:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Line 713 in 1a3646e

return ! ( ! empty( $tag_spec[ AMP_Rule_Spec::CHILD_TAGS ] ) && ! $this->check_valid_children( $node, $tag_spec[ AMP_Rule_Spec::CHILD_TAGS ] ) );

The check_valid_children() method call here was erroneously mutating the document, while checking to see if the nav is a match for that tag spec. The actual tag spec that should only be considered here is the nav spec which has no constraints on the children:

# 4.3.4 The nav element
tags: {
  html_format: AMP
  html_format: AMP4ADS
  html_format: AMP4EMAIL
  html_format: ACTIONS
  tag_name: "NAV"
}

This is a bug that was introduced in v1.1 via #1860.

👉 A side-effect of the change here is the sanitization of AMP components with invalid children will be more draconian. For example, if an amp-story has an invalid child element, then the entire amp-story element will be removed, as opposed to the invalid child alone being removed. Nevertheless, since only AMP components have such validation constraints for children, it should not so common for this to occur in WordPress sites that are just outputting normal HTML. It only will become an issue when starting to use AMP components, and this makes #1420 much more important as it will be needed to explain why the element was removed.

[amedina]

This was a tricky one!