Improve validation of children of invalid elements#1511
Improve validation of children of invalid elements#1511
Conversation
Before, if there is an unaccepted invalid element like <details>, its child elements are not validated. Only on accepting the <details> validation error is the <summary> validated. So this adds elements to $fragment only if the fragment will replace the node. Also, this ensures that the original parent is reported. Before, an invalid node's children replaced the invalid node, and the childrens' parent was reassigned. So this stores the parent in a new property. This might not be the best strategy.
| $child = $node->firstChild; | ||
| while ( $child ) { | ||
| $fragment->appendChild( $child ); | ||
| $this->stack[] = $child; |
There was a problem hiding this comment.
Here, all children were appended to $fragment. Even though $fragment is only used if $should_replace.
So I think this caused the nodes in $this->stack to have no values, and thus not get validated:
As a workaround, this stores the nodes in $children, and only appends them to $fragment if $should_replace.
| $expected_errors[] = array( | ||
| 'node_name' => 'foo', | ||
| 'parent_name' => 'body', | ||
| 'parent_name' => 'divs', |
There was a problem hiding this comment.
It seems like the parent_name should be divs, as that's the parent of <foo>. But feel free to offer a different opinion.
|
Request For Review Hi @amedina and @westonruter, |
| $child = $node->firstChild; | ||
| $children = array(); | ||
| foreach ( $node->childNodes as $child ) { | ||
| $child->originalParent = $node->nodeName; // phpcs:ignore WordPress.NamingConventions.ValidVariableName.NotSnakeCaseMemberVar |
There was a problem hiding this comment.
Without keeping track of the original parent somehow, children of the stripped element will have a different parent reported.
For example, when this is validated:
<div><invalid-element><inner-invalid></inner-invalid></invalid-element></div>This wil strip <invalid-element>:
<div><inner-invalid></inner-invalid></div>So <inner-invalid> will have a reported parent of <div>, even though its original parent was <invalid-element>.
Still, I'm not sure that storing it in $child->originalParent is the right approach.
| ), | ||
| 'nested_invalid_elements' => array( | ||
| array( | ||
| 'source' => '<div><details><summary><p>Example Summary</p></summary><p>Example expanded text</p></details></div>', |
There was a problem hiding this comment.
Now that AMP allows the details element (via ampproject/amphtml#18440), the tests here aren't relevant now.
| $expected_errors[] = array( | ||
| 'node_name' => 'foo', | ||
| 'parent_name' => 'body', | ||
| 'parent_name' => 'divs', |
| ); | ||
|
|
||
| // Both <details> and <summary> are invalid, so there should be errors for both. | ||
| $content[] = '<div><details><summary><p>Example Summary</p></summary><p>Example expanded text</p></details><div>'; |
There was a problem hiding this comment.
As noted above, these elements are no longer invalid. See ampproject/amphtml#18440.
|
Closing in favor of #3243 |
Before
As @amedina reported in #1493, when there is an invalid element like
<details>and the validation error isn't accepted, its child elements are not validated.Copying Alberto's test markup from #1493:
<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>There was only an error for the
<details>, even though the nested<summary>is also invalid:On accepting the
<details>validation error, the<summary>is also validated. But theparent_nameof the<summary>should probably bedetails, as it's nested in<details>:After
Approach
I'm not sure this is the right solution, especially in ensuring the
parent_nameis correct.All of the existing unit tests passed, except for one where the
parent_nameis the original parent. Which I think is the expected result.But adding the
->originalParentto theDOMElement(or DOMText) isn't ideal, as it's not one of its properties.This will of course require full front-end testing in all 3 template modes, like we've done for the other releases.
Closes #1493