Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTML API: Add table support #6040

Closed
wants to merge 59 commits into from

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Feb 6, 2024

Trac ticket: Core-61576

Add support for table elements:

  • TABLE
  • THEAD, TBODY, TFOOT
  • TR
  • TD, TH
  • COL, COLGROUP
  • CAPTION

Not all necessary insertion modes are implemented in this PR, so e.g. "in caption" and "in select in table" modes are not implemented in this PR.

HTML5-lib test change (./vendor/bin/phpunit --group html-api-html5lib-tests):

-Tests: 610, Assertions: 275, Skipped: 335.
+Tests: 610, Assertions: 301, Skipped: 309.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

github-actions bot commented Feb 6, 2024

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@sirreal sirreal force-pushed the html-api/add-table-support branch from 759ca17 to 9c59014 Compare July 5, 2024 08:32
@sirreal sirreal force-pushed the html-api/add-table-support branch from 058e995 to 2ded522 Compare July 16, 2024 11:46
Use the method implemented in WordPress#6982
to avoid duplicating the same functionality.
Instead of bailing directly, call the appropriate step_in_ method.
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one's ready for real review and bound for merging!

src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
tests/phpunit/tests/html-api/wpHtmlProcessor.php Outdated Show resolved Hide resolved
src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved
! $this->state->stack_of_open_elements->has_element_in_table_scope( 'THEAD' ) &&
! $this->state->stack_of_open_elements->has_element_in_table_scope( 'TFOOT' )
) {
// @todo Indicate a parse error once it's possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious about this, because these elements are implicitly created on +TABLE, so I think the only case we could get here is with a TEMPLATE

<table>
<td><template>This </table> does not close the outer one</template>
</table>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just following the spec (quoted below). I'm reluctant to deviate from the spec in all but the most obvious places. There are many places the switch modes and reprocess tokens or process tokens following the rules for other modes.

A character token, if the current node is table, tbody, template, tfoot, thead, or tr element
Let the pending table character tokens be an empty list of tokens.

Let the original insertion mode be the current insertion mode.

Switch the insertion mode to "in table text" and reprocess the token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was no hint of asking you to deviate from the spec; all I was doing was observing an interesting oddity since the only elements we're checking are ones that should always be open. I think that TEMPLATE is the only exception. this was just exploring the rules.

if this turns out to be true, it might be possible one day to verify and take advantage of. for now, all it did was seem odd when I saw it, and then I realized there's an escape hatch in almost all of these situations - the TEMPLATE element.

tests/phpunit/tests/html-api/wpHtmlProcessor.php Outdated Show resolved Hide resolved
Assertions from the spec are explicitly optional in implementation.

Remove the assertion, it's checking an invariant condition.
@sirreal sirreal requested a review from dmsnell July 23, 2024 19:29
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sirreal I fixed two logic issues:

  • in IN CELL we have to check if the current node has the same name as the matched token, but we were only checking that it had a name at all.
  • in IN TABLE BODY for -TBODY, … we needed to clear to table body context.

After our short discussion about it I went ahead with insert_virtual_node() because I want that to have a bookmark, and I think it might be important later to add things like namespace and whether it's virtual.

I've made comment and other minor changes, which can all be reverted if you think it's best as it was before.

@@ -765,6 +765,7 @@ public function expects_closer( $node = null ): ?bool {
}

return ! (
( $node->has_self_closing_flag ?? false ) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, @sirreal, this is no longer required, since we moved to processing the event queue. I'm leaving it in for now, though, while pondering the later change which avoids immediately popping the FORM off the stack of open elements.

@@ -2347,10 +2354,12 @@ private function step_in_table(): bool {
) {
return $this->step();
}

// This FORM is special because it immediately closes and cannot have other children.
$this->state->current_token->has_self_closing_flag = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I'm pondering.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for doing it this way instead of adding and popping right here?

The "> …" quoting omitted part of the quoted spec. Include the complete
spec in the quoting.
@sirreal
Copy link
Member Author

sirreal commented Jul 24, 2024

There's an error in the new form test with the change to auto-close the form where the form closer is not visited by next_token. It's fixed with this patch, although I haven't investigated why:

diff --git before/src/wp-includes/html-api/class-wp-html-processor.php after/src/wp-includes/html-api/class-wp-html-processor.php
index 1e6a18389b..0d1c7c1496 100644
--- before/src/wp-includes/html-api/class-wp-html-processor.php
+++ after/src/wp-includes/html-api/class-wp-html-processor.php
@@ -2356,10 +2356,9 @@ class WP_HTML_Processor extends WP_HTML_Tag_Processor {
 				}
 
 				// This FORM is special because it immediately closes and cannot have other children.
-				$this->state->current_token->has_self_closing_flag = true;
-
 				$this->insert_html_element( $this->state->current_token );
 				$this->state->form_element = $this->state->current_token;
+				$this->state->stack_of_open_elements->pop();
 				return true;
 		}
 

I'll push that change and tests should be passing again. It's effectively reverting part of your updates so happy to discuss further.

@sirreal
Copy link
Member Author

sirreal commented Jul 24, 2024

@dmsnell The changes you've pushed look good, thanks for catching a couple of bugs and I do like the method for creating virtual nodes.

The last remaining thing is to sort out the FORM element in table insertion mode that's immediately popped.

@dmsnell
Copy link
Member

dmsnell commented Jul 24, 2024

I'll push that change and tests should be passing again. It's effectively reverting part of your updates so happy to discuss further.

I was considering this because I wanted the code to function as the void tags function, but I can't figure out what's wrong. Along the way I found some bugs in next_token() I introduced when fixing breadcrumbs, but the fix is incoming and easy. We don't close out the remaining open stack elements at the end of the document because they never get popped.

pento pushed a commit that referenced this pull request Jul 24, 2024
As part of work to add more spec support to the HTML API, this patch adds
support for various table-related insertion modes. This includes support
for tables, table rows, table cells, table column groups, etc...

Developed in #6040
Discussed in https://core.trac.wordpress.org/ticket/61576

Props: dmsnell, jonsurrell.
See #61576.


git-svn-id: https://develop.svn.wordpress.org/trunk@58806 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jul 24, 2024
As part of work to add more spec support to the HTML API, this patch adds
support for various table-related insertion modes. This includes support
for tables, table rows, table cells, table column groups, etc...

Developed in WordPress/wordpress-develop#6040
Discussed in https://core.trac.wordpress.org/ticket/61576

Props: dmsnell, jonsurrell.
See #61576.

Built from https://develop.svn.wordpress.org/trunk@58806


git-svn-id: http://core.svn.wordpress.org/trunk@58202 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Jul 24, 2024
As part of work to add more spec support to the HTML API, this patch adds
support for various table-related insertion modes. This includes support
for tables, table rows, table cells, table column groups, etc...

Developed in WordPress/wordpress-develop#6040
Discussed in https://core.trac.wordpress.org/ticket/61576

Props: dmsnell, jonsurrell.
See #61576.

Built from https://develop.svn.wordpress.org/trunk@58806


git-svn-id: https://core.svn.wordpress.org/trunk@58202 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@dmsnell
Copy link
Member

dmsnell commented Jul 24, 2024

Merged in [58806]
5351cd2

@dmsnell dmsnell closed this Jul 24, 2024
@dmsnell dmsnell deleted the html-api/add-table-support branch July 24, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants