Amp-wp: Unexpected validation error parent element when invalid elements are nested

[amedina]

The <details> HTML tag is not allowed by AMP's specification yet. It will likely be added soon but its current status allows us to see an seemingly unexpected behavior in validation handling. This tag is used as follows:

<details>
  <summary>Copyright 1999-2018.</summary>
   <p>- by Awesome Author. All Rights Reserved.</p>
   <p>All content and graphics on this web site are the property of the author.</p>
</details>

Which results in the following view in the non-AMP version:

On the AMP side the following takes place:

• The plugin correctly flags the HTML error and points out to the <details> tag as the offending markup.
  
  

• Upon accepting the error, the plugin is effectively instructed to remove the details tag

• Upon updating the post, the plugin removes the details tag and flags a new error associated with the <summary> tag

Expected behavior:

• Since the summary tag is a child of the Details tag, the expected behavior is that removing the details tag should also remove the summary tag and therefore no error should be flagged for the latter once the former has been accepted

[westonruter]

At first look, it feels similar to what was fixed in #1461. At the very least the behavior is similar, where accepting one validation error causes another one to take its place. It could be a different underlying cause however.

[amedina]

Why is it that a nested element is not sanitized when the parent is?

[westonruter]

That is the question. I'm guessing it has something to do with the code around here:

https://github.com/Automattic/amp-wp/blob/a3983a0758dc5357592f6461c56e5345b78cacb3/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php#L1817-L1823

This is where that replace_node_with_children method is surely being called when the sanitizer encounters the details element:

https://github.com/Automattic/amp-wp/blob/a3983a0758dc5357592f6461c56e5345b78cacb3/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php#L367-L371

[amedina]

Makes sense;

[kienstra]

Question About Adding To v1.0 Project

Hi @westonruter and @alcurrie,
What do you think about adding this to the v1.0 project, to be part of the 1.0 release?

It'd be good to have this in 1.0. And it's not a new feature, so I think it would be fair to add it in another RC, like RC2.

[alcurrie]

@kienstra - Adding this fix, to the v1.0 project makes sense to me. It is arguably in the grey area between feature improvement and bug fix, but since this is intended to make core functionality (AMP validation) work more completely/correctly it makes sense to me to treat it as a bug fix and add it to another 'RC' for '1.0', but as I'm new to the project interested in @westonruter's opinion.

[westonruter]

It's not a blocker for 1.0. We can have it come in 1.1.

[westonruter]

Testing instructions

1. Create a Custom HTML block and supply it with: <outer-bad><inner-bad>🙈</inner-bad></outer-bad>
2. Hit Save Draft and you should see:
   
3. Click the “Review Issues” link.
4. Reject the outer-bad and accept the inner-bad and Update. The Details column should always have the parent of inner-bad shown as outer-bad, and the parent of outer-bad should always be div (or whatever the theme has). This should not change based on the status of the individual validation errors, whereas before the fix the parent of the inner-bad could be incorrectly div:

Aside: I just noticed that before #3243, the above testing steps causes PHP warning/notice:

PHP Warning: Couldn't fetch DOMElement. Node no longer exists in /app/public/content/plugins/amp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php on line 276
PHP Notice: Undefined property: DOMElement::$parentNode in /app/public/content/plugins/amp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php on line 276

This doesn't happen when the fix is applied.

[kienstra]

Testing Looks Good

Hi @westonruter,
Using your testing instructions, this behaves as expected.

In step 4, the parent of the inner-bad is outer-bad, not div: