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

WIP: HTML API: Add support for H1-H6 elements in HTML Processor #5535

Closed
wants to merge 13 commits into from

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Oct 19, 2023

Trac Ticket: Core-60060

Adds support for handling H1 - H6 elements in the HTML Processor.

  • Verified that these heading elements convey no special rules in other parts of the HTML spec. The definitions within IN BODY processing seem sufficient. For example, there's no mention about special handling in another section about them.
  • Write positive-affirming tests for heading element support.

@@ -2409,8 +2410,31 @@ private function matches() {
return false;
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

b/c Coding Standards, see inline GHA warning

@dmsnell dmsnell force-pushed the html-api/add-support-for-h1-h6 branch from e8c4704 to 6b45967 Compare December 13, 2023 12:36
@dmsnell dmsnell marked this pull request as ready for review December 13, 2023 13:28
dmsnell added a commit to WordPress/gutenberg that referenced this pull request Dec 13, 2023
case '-H4':
case '-H5':
case '-H6':
if ( ! $this->state->stack_of_open_elements->has_element_in_scope( '(internal: H1 through H6 - do not use)' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As an alternative to the string literal, we could spell out the whole list

Suggested change
if ( ! $this->state->stack_of_open_elements->has_element_in_scope( '(internal: H1 through H6 - do not use)' ) ) {
if (
! $this->state->stack_of_open_elements->has_element_in_scope( 'H1' ) &&
! $this->state->stack_of_open_elements->has_element_in_scope( 'H2' ) &&
/* etc */
) {

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you. as we discussed, I think this is going to be very difficult to do in practice for now, specifically because of the pop_until() behavior. using each element individually requires looping in a way that we don't need to do. I'd prefer we leave this a bit awkward internally for now until we have a better solution that doesn't tradeoff efficiency to get there.

// @TODO: Record parse error: this error doesn't impact parsing.
}

$this->state->stack_of_open_elements->pop_until( '(internal: H1 through H6 - do not use)' );
Copy link
Contributor

Choose a reason for hiding this comment

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

This one's a bit trickier than the other one; we could change pop_until's signature to accept either a string or an array.

Copy link
Member Author

Choose a reason for hiding this comment

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

see above: if we don't need array passing I'd prefer that so we can avoid creating the array

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Left two notes suggesting an alternative that would avoid the string literal; but since it's only internal, I don't consider it a blocker.

LGTM 👍

@@ -106,7 +106,6 @@ public function current_node() {
*
* @see https://html.spec.whatwg.org/#has-an-element-in-the-specific-scope
*
* @param string $tag_name Name of tag check.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I missed this; this looks accidental, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, thanks. Not sure what happened here.
added back in 6400821

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

*
* @ticket 60060
*
* @covers WP_HTML_Processor::step
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it next_tag we're using here?

Copy link
Member Author

Choose a reason for hiding this comment

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

more specifically this is covering the semantic rules inside step 🤷‍♂️


/**
* Verifies that H1 through H6 elements close an open H1 through H6 element.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing @ticket and @covers:

Suggested change
*
*
* @ticket 60060
*
* @covers WP_HTML_Processor::next_tag
*

Copy link
Member Author

Choose a reason for hiding this comment

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

added in 4772a7d

@ockham
Copy link
Contributor

ockham commented Dec 13, 2023

Committed to Core in https://core.trac.wordpress.org/changeset/57186/.

@ockham ockham closed this Dec 13, 2023
@dmsnell dmsnell deleted the html-api/add-support-for-h1-h6 branch December 13, 2023 17:59
dmsnell added a commit to WordPress/gutenberg that referenced this pull request Dec 14, 2023
dmsnell added a commit to WordPress/gutenberg that referenced this pull request Dec 20, 2023
dmsnell added a commit to WordPress/gutenberg that referenced this pull request Dec 23, 2023
Updates from WordPress/wordpress-develop at 54a09a7ec99c59b8e640bd5aacebfdbf03bb02cc

 - WordPress/wordpress-develop#5535
   Adds support for H1 - H6 elements in the HTML Processor.

 - WordPress/wordpress-develop#5725
   Pause the Tag Processor when reaching the end of the document
   inside an incomplete syntax element.

 - Incorporates linting changes from "TODO:" to "todo"

This patch adds the blanket exclusion from the HTML API compatability
layer after they were removed and started blocking updates from Core.

The PHP files in the compatability layer are merged and maintained in
the Core repo and all changes or updates need to happen first in Core
and then be brought over to Gutenberg as built files. From Gutenberg's
perspective they are no different than NPM packages.

Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
Co-authored-by: Bernie Reiter <96308+ockham@users.noreply.github.com>
dmsnell added a commit to WordPress/gutenberg that referenced this pull request Jan 1, 2024
Updates from WordPress/wordpress-develop at 54a09a7ec99c59b8e640bd5aacebfdbf03bb02cc

 - WordPress/wordpress-develop#5535
   Adds support for H1 - H6 elements in the HTML Processor.

 - WordPress/wordpress-develop#5725
   Pause the Tag Processor when reaching the end of the document
   inside an incomplete syntax element.

 - Incorporates linting changes from "TODO:" to "todo"

This patch adds the blanket exclusion from the HTML API compatability
layer after they were removed and started blocking updates from Core.

The PHP files in the compatability layer are merged and maintained in
the Core repo and all changes or updates need to happen first in Core
and then be brought over to Gutenberg as built files. From Gutenberg's
perspective they are no different than NPM packages.

Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
Co-authored-by: Bernie Reiter <96308+ockham@users.noreply.github.com>
dmsnell added a commit to WordPress/gutenberg that referenced this pull request Jan 13, 2024
Updates from WordPress/wordpress-develop at 54a09a7ec99c59b8e640bd5aacebfdbf03bb02cc

 - WordPress/wordpress-develop#5535
   Adds support for H1 - H6 elements in the HTML Processor.

 - WordPress/wordpress-develop#5725
   Pause the Tag Processor when reaching the end of the document
   inside an incomplete syntax element.

 - Incorporates linting changes from "TODO:" to "todo".

 - WordPress/wordpress-develop#5762
   Handles the "all other tags" rules in IN BODY insertion mode.

 - WordPress/wordpress-develop#5539
   Adds support for list elements in the HTML Processor.

This patch adds the blanket exclusion from the HTML API compatability
layer after they were removed and started blocking updates from Core.

The PHP files in the compatability layer are merged and maintained in
the Core repo and all changes or updates need to happen first in Core
and then be brought over to Gutenberg as built files. From Gutenberg's
perspective they are no different than NPM packages.

Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
Co-authored-by: Bernie Reiter <96308+ockham@users.noreply.github.com>
dmsnell added a commit to WordPress/gutenberg that referenced this pull request Jan 17, 2024
Updates from WordPress/wordpress-develop at 54a09a7ec99c59b8e640bd5aacebfdbf03bb02cc

 - WordPress/wordpress-develop#5535
   Adds support for H1 - H6 elements in the HTML Processor.

 - WordPress/wordpress-develop#5725
   Pause the Tag Processor when reaching the end of the document
   inside an incomplete syntax element.

 - Incorporates linting changes from "TODO:" to "todo".

 - WordPress/wordpress-develop#5762
   Handles the "all other tags" rules in IN BODY insertion mode.

 - WordPress/wordpress-develop#5539
   Adds support for list elements in the HTML Processor.

This patch adds the blanket exclusion from the HTML API compatability
layer after they were removed and started blocking updates from Core.

The PHP files in the compatability layer are merged and maintained in
the Core repo and all changes or updates need to happen first in Core
and then be brought over to Gutenberg as built files. From Gutenberg's
perspective they are no different than NPM packages.

Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
Co-authored-by: Bernie Reiter <96308+ockham@users.noreply.github.com>
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