Html api/add html processor #6

dmsnell · 2023-03-10T00:47:44Z

Trac ticket:

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.

…he HTML specification

…/add-html-processor

adamziel · 2023-03-10T09:37:23Z

src/wp-includes/html-api/class-wp-html-processor.php

+ while ( $p->next_tag( array( 'tag_closers' => 'visit' ) ) ) {
+ $tag_name = $p->get_tag();
+
+ if ( ! $p->is_tag_closer() ) {


Are void elements closers?

No they're not, that's why this code branch works

as you probably realized, tag closers only exist when the tag name is preceded by a /, which is distinct from the self-closing flag which appears right before the > closing the tag opener

adamziel · 2023-03-10T09:41:30Z

src/wp-includes/html-api/class-wp-html-processor.php

+ $element = WP_HTML_Spec::element_info( end( $this->open_elements ) );
+ // @TODO: Handle self-closing HTML foreign elements: must convey self-closing flag on stack.
+ if ( $element::is_void ) {
+ array_pop( $this->open_elements );


Shouldn't it say if ( ! element::is_void )?

Or not? I don't get why void elements have any interaction with the open elements stack

Oh I see, why are void elements landing on the open elements stack, though?

this is something to document later and it appeared towards the end of the day. I think I've seen this in the HTML spec for tree-building.

the problem is that we have to push the void element to the open stack in order to get proper depth information and climb out of the nesting we're in. this looks funny here because we aren't currently tracking if the last element on the stack requires no closer (is void or self-closing foreign element). that means whenever we approach the next tag we have to immediately remove any void elements because they cannot enclose over other tags.

it's definitely unexpected until you reason through it, or as I did, get reminded by it with failing tests.

According to the spec, popping should happen in the same step as the insertion:

A start tag whose tag name is one of: "area", "br", "embed", "img", "keygen", "wbr"
Reconstruct the active formatting elements, if any.

Insert an HTML element for the token. Immediately pop the current node off the stack of open elements.

Acknowledge the token's self-closing flag, if it is set.

Set the frameset-ok flag to "not ok".

adamziel · 2023-03-10T09:44:52Z

src/wp-includes/html-api/class-wp-html-processor.php

+
+ $starting_depth = count( $this->open_elements );
+
+ while ( $this->next_tag() ) {


We should bale out if between the $starting_depth and the child we see anything with $current_depth <= $starting_depth as that would imply the initial tag was closed

given the ensure_support() function that runs I don't think it's possible for this to happen, but I think I can defer some of the support function. I've added a test to make sure, though I haven't yet added any non-HTML elements in the tests.

adamziel · 2023-03-10T09:48:24Z

src/wp-includes/html-api/class-wp-html-processor.php

+ $current_depth = count( $this->open_elements );
+
+ if ( $this->is_tag_closer() && $current_depth < $starting_depth ) {
+ return true;


Ditto here for the depth check I mentioned above

this is all garbage that will go away

adamziel · 2023-03-10T09:49:55Z

src/wp-includes/html-api/class-wp-html-processor.php

+
+ $stack = array();
+
+ $p = new WP_HTML_Tag_Processor( $this->html );


Smart! I kind of don't like that it throws away the progress we have made so far, though

what do you mean? my vision for this is that it can grow with our added support.

as for running a first-pass through the document I'm not claiming this is certainly needed. I'm sure it's possible to work it in alongside the normal traversal.

for now though it helped quite a bit, and I figured the cost is meager given that it's a read-only scan.

adamziel · 2023-03-10T10:05:04Z

@luisherranz just pointed out that we may not need Bookmark "end" value. We don't seem to use it anywhere, and it generates problems with outer HTML logic. I like that. What do you think @dmsnell

adamziel · 2023-03-10T10:18:00Z

src/wp-includes/html-api/class-wp-html-processor.php

+
+ $self_closes = $element::is_void || ( ! $element::is_html && $this->has_self_closing_flag() );
+ if ( $self_closes ) {
+ $this->open_elements[] = $tag_name;


I don't like this part, it's not really an open element. Even in the parsing spec these are only added to the stack for brevity of the document – they're always immediately popped in the same step and before ingesting the next token

I think it's essential: see above. may not be able to articulate it well yet, but I think one case we might examine is finding the next_sibling() of an <img>. if we don't push void elements onto the stack we'll be looking for a tag that's a parent of the img instead of a sibling.

I think this will matter also for CSS selection. we will need to know the full "tree path" (a term I'm making up here without intending to convey specific information about it) in order to match our current position against the selector.

Btw it's not what the spec says, is it? Depending on the insertion mode and element name, we may or may not have to push it to the stack of open elements. There may be other operations to do before pushing, too, and the order matters.

adamziel · 2023-03-10T10:20:47Z

src/wp-includes/html-api/class-wp-html-processor.php

+ return $this->fully_supported_input;
+ }
+
+ public function next_tag( $query = null ) {


I wish we could make it private in the descendant class. It doesn't even make sense to have it public, it can't support the same set of queries as the parent, and it's a pain to deal with PHP's idea of polymorphism where the parent class will call the child implementation of this method anyway.

funny enough I added _doing_it_wrong() here before realizing that I call it from within this tag processor.

is it the tag processor that's calling this?

the one thing I wish we could do is remove $query but it wouldn't let me. still some thinking about this one to do.

adamziel · 2023-03-10T10:22:00Z

src/wp-includes/html-api/class-wp-html-processor.php

+ }
+ $this->set_bookmark( 'end' );
+
+ $start = $this->bookmarks['start']->end + 1;


Here's one usage of the end bookmark property, but we could do without it by just storing this in a variable a few lines above. (I'm referring to the idea of ditching the end property)

adamziel · 2023-03-10T10:32:14Z

Oh this is great @dmsnell! A few more notes:

seek now needs to rewind the open_elements stack as well as the parsing cursor. Worse, you need to do it before seek calls next_tag – at one point I ended up just copying and pasting the entire seek body from the parent class.

next_tag is public, which I don't like – this class reasons about nodes and exposing a lower-level abstraction seems like a footgun. I don't know how to deal with it, though. We can't just return false and call parent::next_tag because that would break all $this->next_tag() calls in the Tag Processor class. There may be only one right now, but I don't like creating an implicit constraint on introducing more such calls.

adamziel · 2023-03-10T10:33:58Z

src/wp-includes/html-api/class-wp-html-processor.php

+ $end = $this->bookmarks['end']->end + 1;
+ $this->lexical_updates[] = new WP_HTML_Text_Replacement( $start, $end, $new_html );
+ $this->get_updated_html();
+ $this->bookmarks['start']->start = $start;


I've done the same in one of my attempts! Why is this one not getting released in get_updated_html, though? I remember I had to create a new WP_HTML_Text_Span and re-insert it into $this->bookmarks

not quite sure why it isn't released

adamziel · 2023-03-10T10:35:58Z

Oh, one other thing – be sure to test set_outer_content when there are some lexical updates already scheduled. I remember having to flush out everything at the beginning of that method, or else things got messy. It could be caused by my attempt to insert tag closers, though.

dmsnell · 2023-03-21T22:03:03Z

@luisherranz just pointed out that we may not need Bookmark "end" value. We don't seem to use it anywhere, and it generates problems with outer HTML logic. I like that. What do you think @dmsnell

I would love to hear more about this, how it creates problems. it seems inherently essential for separating inside vs. outside of tag tokens, so I'm confused how it isn't necessary.

dmsnell · 2023-03-22T01:09:41Z

seek now needs to rewind the open_elements stack as well as the parsing cursor. Worse, you need to do it before seek calls next_tag – at one point I ended up just copying and pasting the entire seek body from the parent class.

stewing on this I came up with an idea and then realized it's probably the same as your pinned bookmarks 🙃

adamziel · 2023-05-04T17:27:21Z

src/wp-includes/html-api/class-wp-html-processor.php

+ private function enter_element( $element ) {
+ $this->depth++;
+
+ parent::set_bookmark( "{$this->depth}_{$element}" );


also, why set a bookmark for each element?

this is a stub function for the stack of open elements. I think I'm going to rip it out and use a proper stack, but we have to have a bookmark because if we make edits we have to know that the stack might have changed.

We only need a bookmark for the current element, though, right? Not for all the elements seen so far, nor for all parents.

the stack of open elements might actually shift and some may disappear. in any case, if we seek() we have to reset them, so we need a good way to reconstruct the stack when seeking.

in your PR I think you're doing that by storied copies of all the open elements for every bookmark, but also we have to make sure those are bookmarks because we expect changes to be applied to the document which could change things.

the main case that matters I think is wiping out a bookmark, but inserting nodes will shift them all.

<div><p>inside</p></div> ^^^ bookmark unwrap <p>inside</p> ^^^ stack of open elements here is no longer DIV > P but rather only P

…s to escape

adamziel · 2023-05-17T22:23:04Z

src/wp-includes/html-api/class-wp-html-processor.php

+ default:
+ return self::NOT_IMPLEMENTED_YET;
+ }
+ } catch ( Exception $e ) {


Catching all exceptions might hide legitimate errors, it would be safer to have a HTML_Exception class or so

good point. at some point I wanted to go back and special-case known exceptions so we don't hide others, but I didn't do that yet because I wasn't entirely sure how I felt about the escaping mechanism as throwing Exceptions.

adamziel · 2023-05-17T22:25:52Z

src/wp-includes/html-api/class-wp-html-processor.php

+ parent::__construct( $html );
+
+ $this->tag_openers = new WP_HTML_Element_Stack();
+ $this->tag_closers = new WP_HTML_Element_Stack();


When would you push to this stack?

still working on an answer for this 😆

…processor

…errors

TODO: - abstract and optimize "if P in button scope then close P"

…processor

dmsnell added 12 commits March 6, 2023 17:54

WIP: HTML API: Add HTML Spec class to convey information related to t…

805fb55

…he HTML specification

WIP: HTML API: Expose self-closing flag in Tag Processor

84e35ef

Expand documentation

84e25cf

Appease the linting gods

c18a81a

Add extra tests for syntax peculiarities.

af8d49d

Appease the linting gods

72e5de0

Appease the linting gods

095a110

Appease the linting gods

9b3f90d

Merge branch 'html-api/tag-processor-self-closing-flag' into html-api…

09e16b2

…/add-html-processor

Add ensure_support() and tests

396285b

WIP: Work on next_sibling

b2e6856

Add the rest of it

65c9f5b

adamziel reviewed Mar 10, 2023

View reviewed changes

Merge branch 'trunk' into html-api/add-html-processor

f5e4d5e

dmsnell added 3 commits March 21, 2023 16:56

Rename "next_child" to "first_child" to better match its purpose

247966b

Ensure no children are found for void elements.

f1ccd9b

rename next_child to first_child in test names

fb31ea1

adamziel reviewed May 4, 2023

View reviewed changes

dmsnell added 5 commits May 8, 2023 19:18

Merge branch 'trunk' into html-api/add-html-processor

a5284d6

Merge branch 'trunk' into html-api/add-html-processor

74c9e6b

Merge branch 'trunk' into html-api/add-html-processor

a4569cd

Add WIP stack class, trap exceptions in step() to allow nested call…

98180bc

…s to escape

Play with an alternate matching syntax

0e3ada5

dmsnell force-pushed the html-api/add-html-processor branch from d2791d0 to 0e3ada5 Compare May 17, 2023 17:03

adamziel reviewed May 17, 2023

View reviewed changes

dmsnell added 19 commits May 18, 2023 01:40

Add reset_insertion_mode_appropriately

739b4aa

Merge remote-tracking branch 'upstream/trunk' into html-api/add-html-…

2b263b5

…processor

Merge branch 'trunk' into html-api/add-html-processor

e1ec10c

Add custom "Unsupported Exception" class to differentiate from other …

bc61ea8

…errors

Set template insertion mode stack to empty array, not null

6f58b70

Expand SPEC data for IS_SPECIAL and IS_OBSOLETE

d82b146

Docs for SPEC and some minor refactoring

a6e5323

Add algorithms for handling list of active formatting elements.

8a2215e

Add docs for HEAD and FORM pointers.

d96c05a

Generate implied end tags, poorly and naively.

c93d420

More docs, remove something not necessary.

2e0e4f9

Introduce static factory methods

f5c7651

Access private methods and properties of friend class

3956ad9

Merge branch 'trunk' into html-api/add-html-processor

37ce95e

Replace goto:ignored with recursion

613ae1a

Continue adding IN BODY handling.

46e3777

TODO: - abstract and optimize "if P in button scope then close P"

Merge branch 'trunk' into html-api/add-html-processor

a1a095c

Merge remote-tracking branch 'upstream/trunk' into html-api/add-html-…

c2dcc37

…processor

More support

c29570e

dmsnell mentioned this pull request Jun 17, 2023

HTML-API: Introduce minimal HTML Processor. WordPress/wordpress-develop#4519

Closed

7 tasks

dmsnell mentioned this pull request Jul 27, 2023

HTML API: Adjust code styling to Gutenberg's linter's preferences WordPress/wordpress-develop#4918

Closed


		$starting_depth = count( $this->open_elements );

		while ( $this->next_tag() ) {


		$stack = array();

		$p = new WP_HTML_Tag_Processor( $this->html );

Html api/add html processor #6

Are you sure you want to change the base?

Html api/add html processor #6

Conversation

dmsnell commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Mar 10, 2023 • edited Loading

dmsnell commented Mar 21, 2023

dmsnell commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Mar 10, 2023 •

edited

Loading

adamziel commented Mar 10, 2023 •

edited

Loading