Methods for inserting HTML should be exposed on OD_Template_Optimization_Context for before or after all tags have been visited

[westonruter]

Originally discussed in #1919 (comment) and in other comments on that PR.

The OD_HTML_Tag_Processor class includes two methods: ::append_head_html() and ::append_body_html(). The OD_HTML_Tag_Processor instance is exposed on an OD_Tag_Visitor_Context instance passed to the tag visitors, but it is not yet exposed on the OD_Template_Optimization_Context class which is passed to the od_start_template_optimization and od_finish_template_optimization actions which run before and after the document has been iterated over by tag visitors, respectively.

The current use cases for ::append_head_html() are:

• Optimization Detective injecting the LINK HTML markup returned by OD_Link_Collection::get_html().
• Image Prioritizer adding a STYLE tag via a tag visitor to add a style for lazy loaded background images.
• Embed Optimizer adding a STYLE tag via a tag visitor to reduce layout shifts.
• Content Visibility adding a STYLE tag via a tag visitor for CV styles.

The current use cases for ::append_body_html() are:

• Optimization Detective uses this to insert the detect.js script to the page.
• Embed Optimizer adding a SCRIPT to the end of the BODY when there is a lazy-loaded embed on the page.
• Image Prioritizer adding a SCRIPT tag via a tag visitor to lazy load background images.
• Image Prioritizer adding a SCRIPT tag via a tag visitor to lazy load videos.

Allowing insertion of HTML once via the od_finish_template_optimization avoids the need for tag visitors to keep track of whether they inserted or not. They can use the tag visitor callbacks to get a "lay of the land" by looking at all of the tags, and then at the od_finish_template_optimization action they can finalize what they need to insert in the HEAD or the BODY.

This could, for example, allow tag visitors to better optimize stylesheets they insert into the document. Instead of Embed Optimizer inserting a separate STYLE for each embed to reserve space to reduce layout shifts, it could instead insert a single STYLE at od_finish_template_optimization which combines all the style rules in one stylesheet. For example, this would allow Embed Optimizer to better group styles by media query instead of having to output @media (width <= 480px) {} for each embed on the page. Currently for each embed it inserts a STYLE like:

<style>
@media (width <= 480px) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
@media (480px < width <= 600px) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
@media (600px < width <= 782px) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
@media (782px < width) { #embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; } }
</style>
<style>
@media (width <= 480px) { #embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; } }
@media (480px < width <= 600px) { #embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; } }
@media (600px < width <= 782px) { #embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; } }
@media (782px < width) { #embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; } }
</style>
<style>
@media (width <= 480px) { #embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; } }
@media (480px < width <= 600px) { #embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; } }
@media (600px < width <= 782px) { #embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; } }
@media (782px < width) { #embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; } }
</style>

With the od_finish_template_optimization action, it could just print the @media at-rules once for each viewport group rather than duplicating them, and output them all in a single STYLE tag rather than in three separate ones, for example:

<style>
@media (width <= 480px) {
	#embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; }
	#embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; }
	#embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; }
}
@media (480px < width <= 600px) {
	#embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; }
	#embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; }
	#embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; }
}
@media (600px < width <= 782px) {
	#embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; }
	#embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; }
	#embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; }
}
@media (782px < width) {
	#embed-optimizer-6040306707fb51ccaafa915c2da8f412 { min-height: 500px; }
	#embed-optimizer-96ffd32b51748c70b288af1ef0c14c01 { min-height: 500px; }
	#embed-optimizer-f9ff6c9a914366ac3c1aab1994dd8a69 { min-height: 500px; }
}
</style>

See #1923 for a proof of concept for this.

Additionally, for tag visitors that insert scripts at the end of the BODY based on whether certain tags are encountered, this could be done via the od_finish_template_optimization action instead. Otherwise the tag visitor needs to keep track with a added_lazy_script class member variable for whether it inserted the script yet, although the difference here is not as great as with the stylesheets in Embed Optimizer. Example: #1922.

Being able to insert HTML via these actions would also be useful to a plugin like Optimization Detective Admin UI which needs to insert the current response's URL Metric data for rendering in the admin bar. See westonruter/od-admin-ui#11 for an example of how this could be implemented.

Currently when tag visitors call ::append_head_html() or ::append_body_html(), they must be sure to pass raw HTML which is valid in the HEAD and BODY context respectively. If they don't, then they'll cause a parsing error for the browser (e.g. adding an IMG in the HEAD will prematurely open the BODY).

Optimization Detective could continue to use the ::append_head_html() and ::append_body_html(), but for extensions there would need to be new methods like:

• ::append_head_style() to append an inline stylesheet
• ::append_head_script() (but also differentiate between inline vs non-inline?)
• ::append_body_script() (ditto)

We wouldn't need to include an ::append_head_link() since this is what the OD_Link_Collection takes care of, but we may need to add support for rel=stylesheet.

Other potential methods that will be useful:

• ::append_body_stylesheet_link() (not used yet, but useful for adding non-blocking external stylesheets)
• ::append_head_meta() to add a META tag
• ::append_body_comment() to add a comment to the end of the BODY
• ::append_document_comment() to add a comment after the closing </html> tag.

Note that we should perhaps not allow script modules to be inserted in the HEAD because import map scripts (SCRIPT[type="importmap"]) are printed in the footer in Classic Themes, and if a script module appears before an import map then it prevents the import map from working.

If these methods were exposed on both OD_Tag_Visitor_Context and OD_Template_Optimization_Context then the ::append_html_html() and ::append_body_html() methods could be deprecated and eventually not exposed at all.

See also the following from #1546:

We should consider not directly exposing the OD_HTML_Tag_Processor to the tag visitors and template optimization start/finish action handlers since this then exposes the append_head_html() and append_body_html() methods which may not be desirable. This also exposes a lower-level API than which may be appropriate which will complicate switching between using WP_HTML_Tag_Processor and WP_HTML_Processor implementations. Namely, for WP_HTML_Processor there may not be a need to add a get_xpath() method directly to the class, and we may not need to subclass it at all. If we instead passed a wrapper class, then we could expose a get_xpath() on the wrapper subclass which is then able to use the appropriate implementation based on whether it is using WP_HTML_Tag_Processor or WP_HTML_Processor. In fact, this could allow Optimization Detective to use an unsubclassed original WP_HTML_Tag_Processor instance which is provided by core for output-buffer manipulation instead of allowing filtering the underlying string (if that is so desired, per Core-43258).

[felixarntz]

@westonruter Thanks for opening this, and for the detailed description.

I like having methods for specific use-cases rather than allowing any HTML. Not only does this prevent problematic usage, but it also encourages extensions to use these new methods where possible, rather than continuing to use the tag visitor context's tag processor's broader (but therefore also more complicated-to-use) HTML insertion methods.

Some additional thoughts and questions:

• Instead of having methods prefixed with append_head_, append_body_, append_document_, how about we instead introduce some constants / an enum-like class or interface with constants that represents these "locations"? That way we could have fewer methods, and they could be called like e.g. append_style( Location::HEAD, /* stylesheet */ ).
• I'm not sure yet on how we should best name the methods when it comes to inline vs external stylesheets/scripts. If we want to lean on Core's naming, anything just called "style" or "script" would be considered external / not inline, and "inline style" and "inline script" would denote the inline variants. Alternatively, we could use explicit method names where we always refer to "external style" / "external script" / "inline style" / "inline script". The one approach I'm not a fan of is to rely on the "link" term - while technically that's used for an "external style", I think it's unnecessarily low level for the API's intended usage.
• On the PR we discussed potentially requiring an identifier for any such asset added, which would avoid the need for the extension to check whether a specific style or script was already added or not. The underlying implementation could have that baked-in. Alternatively, we could potentially solve it internally even without identifiers, where arguably we could just take the actual script or style string and not inject it if it was already injected, since there's no value in injecting the exact same CSS or JS - in JS there may be theoretical usages like this, but not sure whether that's relevant to consider here.
• It might be a good idea to add these methods on a newly introduced class that wraps the (private) OD_HTML_Tag_Processor instance. An instance of the new class (naming ideas??) could then be made available on OD_Template_Optimization_Context via __get(). This would be in line with how extensions access the tag processor on OD_Tag_Visitor_Context. Eventual usage would be like: $template_context->{new thing}->append_....
• Are there any remaining use-cases where tag visitors need to be able to modify HTML via their own tag visitor callback? Of course they need to retain access to the OD_HTML_Tag_Processor instance to read attributes etc., but I wonder whether we should somehow lock down (or abstract away) the ability to inject arbitrary HTML from the tag visitors as well, to further clarify and harden the API.

Curious to get your thoughts.

[westonruter]

@felixarntz

• Instead of having methods prefixed with append_head_, append_body_, append_document_, how about we instead introduce some constants / an enum-like class or interface with constants that represents these "locations"? That way we could have fewer methods, and they could be called like e.g. append_style( Location::HEAD, /* stylesheet */ ).

Thinking about this some more, I think we can actually be a bit more opinionated about where the HTML is inserted. For example, OD_Link_Collection provides an interface to add links but it doesn't let you control where they're added.

For example, when inserting a script module, it should always be appended to the BODY because otherwise if it is inserted before an import map, this can cause an error. So we can have these methods:

• add_script_module( string $src, array $attrs = array( 'async' => false ) )
• add_inline_script_module( string $text, array $attrs = array( 'async' => false ) )

There's no need to be able to specify defer since they are deferred by default. Also, there's no use case currently for non-inline script modules:

performance/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php

Line 244 in 26442b9

$processor->append_body_html( wp_get_inline_script_tag( image_prioritizer_get_video_lazy_load_script(), array( 'type' => 'module' ) ) );

performance/plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php

Line 237 in 26442b9

$processor->append_body_html( wp_get_inline_script_tag( image_prioritizer_get_lazy_load_bg_image_script(), array( 'type' => 'module' ) ) );

performance/plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php

Line 331 in 26442b9

$processor->append_body_html( wp_get_inline_script_tag( embed_optimizer_get_lazy_load_script(), array( 'type' => 'module' ) ) );

And there aren't any needs so far to insert a non-module script, so perhaps we just not consider these for now.

For STYLE tags, they should generally always be added to the HEAD. The only exception to this is styles for keyframes, which AMP requires to be placed at the end of the BODY. So we could have:

• add_style( $text ) (or add_inline_style( $text )) which always appends to the HEAD
• add_keyframes_style( $text ) when needed, which always appends to the BODY

But we can just go with the first for now.

• Alternatively, we could use explicit method names where we always refer to "external style" / "external script" / "inline style" / "inline script". The one approach I'm not a fan of is to rely on the "link" term - while technically that's used for an "external style", I think it's unnecessarily low level for the API's intended usage.

That said, there is the OD_Link_Collection which could be used for adding stylesheets. This would allow the stylesheets to be loaded even earlier via Link HTTP response headers.

• Alternatively, we could potentially solve it internally even without identifiers, where arguably we could just take the actual script or style string and not inject it if it was already injected, since there's no value in injecting the exact same CSS or JS - in JS there may be theoretical usages like this, but not sure whether that's relevant to consider here.

Yeah, that makes sense to me.

• It might be a good idea to add these methods on a newly introduced class that wraps the (private) OD_HTML_Tag_Processor instance. An instance of the new class (naming ideas??) could then be made available on OD_Template_Optimization_Context via __get(). This would be in line with how extensions access the tag processor on OD_Tag_Visitor_Context. Eventual usage would be like: $template_context->{new thing}->append_....

Yes, this is what I was getting at with what I quoted above from #1546.

The one hiccup here is that in the context of the tag visitor, all of the methods on OD_HTML_Tag_Processor (subclassing WP_HTML_Tag_Processor) would need to be available:

• ::add_class()
• ::change_parsing_namespace() (not sure how this is used)
• ::class_list()
• ::expects_closer()
• ::get_attribute()
• ::get_attribute_names_with_prefix()
• ::get_breadcrumbs()
• ::get_comment_type()
• ::get_current_depth()
• ::get_doctype_info()
• ::get_full_comment_text()
• ::get_modifiable_text()
• ::get_namespace()
• ::get_qualified_attribute_name()
• ::get_qualified_tag_name()
• ::get_tag()
• ::get_token_name()
• ::get_token_type()
• ::get_updated_html() (this need not be exposed)
• ::get_xpath()
• ::has_bookmark()
• ::has_class()
• ::has_self_closing_flag()
• ::is_tag_closer()
• ::next_tag()
• ::next_token()
• ::release_bookmark()
• ::remove_attribute()
• ::remove_class()
• ::seek()
• ::set_attribute()
• ::set_bookmark() (we can prevent tag visitors here from setting reserved bookmark names)
• ::set_meta_attribute()
• ::set_modifiable_text()
• ::subdivide_text_appropriately() (not sure why this would be needed)

However, none of these should be exposed on OD_Template_Optimization_Context since the processor is either not yet started or it is already completed walking over the document.

So I think we'd continue to provide OD_Tag_Visitor_Context::$processor but it wouldn't be direct access to the OD_HTML_Tag_Processor but rather a wrapper which could switch between WP_HTML_Tag_Processor or WP_HTML_Processor in the future. This wrapper could add additional restraints about what methods are allowed to be called. For example, tag visitors must not be able to set or clear internal bookmark names:

• optimization_detective_end_of_head
• optimization_detective_end_of_body
• optimization_detective_current_tag

What OD_Template_Optimization_Context and OD_Tag_Visitor_Context both need is an additional property for handling the insertion of new HTML into the document. For example, it could be it could be an OD_HTML_Document_Inserter class which is exposed as OD_Template_Optimization_Context::$document_inserter and OD_Tag_Visitor_Context::$document_inserter which then exposes these methods:

• add_inline_style()
• add_inline_script_module()

Or these methods could be defined right on OD_Template_Optimization_Context and OD_Tag_Visitor_Context which then call the underlying method on the underlying HTML Processor implementation (which are not present on the processor wrapper exposed on OD_Tag_Visitor_Context).

• Are there any remaining use-cases where tag visitors need to be able to modify HTML via their own tag visitor callback? Of course they need to retain access to the OD_HTML_Tag_Processor instance to read attributes etc., but I wonder whether we should somehow lock down (or abstract away) the ability to inject arbitrary HTML from the tag visitors as well, to further clarify and harden the API.

I think the above handles all the cases currently identified for extensions. Optimization Detective itself would need to be able to insert raw HTML, of course, to be able to insert the link tags, but this needn't be exposed to the extensions.

[westonruter]

As part of this, the optimization loop should directly have access to WP_HTML_Tag_Processor::set_attribute() and not the subclassed version so that it can avoid adding the data-od- meta attributes as seen in #1954. For example, it is currently resulting in:

<meta data-od-replaced-content="optimization-detective 0.0.0" name="generator" content="optimization-detective 0.0.0; url_metric_groups={0: empty, 480: empty, 600: empty, 782: empty}">

When it should instead output:

<meta name="generator" content="optimization-detective 0.0.0; url_metric_groups={0: empty, 480: empty, 600: empty, 782: empty}">

The OD meta attributes are intended for tag visitors to show what changes they made on the document.

[felixarntz]

@westonruter

Thinking about this some more, I think we can actually be a bit more opinionated about where the HTML is inserted. For example, OD_Link_Collection provides an interface to add links but it doesn't let you control where they're added.

For example, when inserting a script module, it should always be appended to the BODY because otherwise if it is inserted before an import map, this can cause an error.

I like that idea, let's start with being opinionated and only supporting script modules, not regular scripts. We could always add support for the latter later, if there proves to be a need.

For STYLE tags, they should generally always be added to the HEAD. The only exception to this is styles for keyframes, which AMP requires to be placed at the end of the BODY. So we could have:

• add_style( $text ) (or add_inline_style( $text )) which always appends to the HEAD
• add_keyframes_style( $text ) when needed, which always appends to the BODY

But we can just go with the first for now.

+1, I think the keyframes requirement is too specific to consider for now. Also, AMP requiring it doesn't necessarily make a compelling argument for supporting it here, since it's neither officially supported by Core nor a standard of some kind.

• Alternatively, we could use explicit method names where we always refer to "external style" / "external script" / "inline style" / "inline script". The one approach I'm not a fan of is to rely on the "link" term - while technically that's used for an "external style", I think it's unnecessarily low level for the API's intended usage.

That said, there is the OD_Link_Collection which could be used for adding stylesheets. This would allow the stylesheets to be loaded even earlier via Link HTTP response headers.

Wouldn't that only allow for external stylesheets? In any case, even if we use OD_Link_Collection under the hood, I think the API would be more intuitive if we exposed consistent methods on our new class for script modules and stylesheets.

The one hiccup here is that in the context of the tag visitor, all of the methods on OD_HTML_Tag_Processor (subclassing WP_HTML_Tag_Processor) would need to be available:

[...]

However, none of these should be exposed on OD_Template_Optimization_Context since the processor is either not yet started or it is already completed walking over the document.

I was thinking that the new class would only be responsible for writing/adding stuff to the HTML output, not reading. I think (at least for now), the tag visitor context could maintain access to the OD_HTML_Tag_Processor instance to get information about the HTML.

So I think we'd continue to provide OD_Tag_Visitor_Context::$processor but it wouldn't be direct access to the OD_HTML_Tag_Processor but rather a wrapper which could switch between WP_HTML_Tag_Processor or WP_HTML_Processor in the future. This wrapper could add additional restraints about what methods are allowed to be called. For example, tag visitors must not be able to set or clear internal bookmark names:

• optimization_detective_end_of_head
• optimization_detective_end_of_body
• optimization_detective_current_tag

Per what I answered above, I think this is worth thinking about, but I think as part of this issue we should limit the scope to focusing on the write interactions only via the new class.

What OD_Template_Optimization_Context and OD_Tag_Visitor_Context both need is an additional property for handling the insertion of new HTML into the document. For example, it could be it could be an OD_HTML_Document_Inserter class which is exposed as OD_Template_Optimization_Context::$document_inserter and OD_Tag_Visitor_Context::$document_inserter which then exposes these methods:

• add_inline_style()
• add_inline_script_module()

See my previous reply, that's roughly what I meant for the new class to have as scope.

I think the above handles all the cases currently identified for extensions. Optimization Detective itself would need to be able to insert raw HTML, of course, to be able to insert the link tags, but this needn't be exposed to the extensions.

👍

[westonruter]

@felixarntz

Wouldn't that only allow for external stylesheets? In any case, even if we use OD_Link_Collection under the hood, I think the API would be more intuitive if we exposed consistent methods on our new class for script modules and stylesheets.

That's right, it would just be for external stylesheets. But considering that one performance pattern is to put stylesheets in the footer (e.g. if they are intended to not be blocking) then this doesn't really fit with OD_Link_Collection. So yeah, probably a separate method like ::add_external_style() should be added (eventually). This would be in addition to ::add_inline_style() for inline styles.

I was thinking that the new class would only be responsible for writing/adding stuff to the HTML output, not reading. I think (at least for now), the tag visitor context could maintain access to the OD_HTML_Tag_Processor instance to get information about the HTML.

If they had access to the OD_HTML_Tag_Processor then they'd also have access to the ::append_head_html() and ::append_body_html() methods. And otherwise, WP_HTML_Tag_Processor (and WP_HTML_Processor) have many other methods for writing HTML, like ::set_attribute() and ::set_modifiable_text(), so it would seem strange to be able to use the processor instance for some writes but then this other HTML writer class for other writes. I think better to add that abstraction now so from the tag visitor's perspective (and the perspective of od_start_template_optimization and od_finish_template_optimization) for them to still have access to the one processor which exposes an interface which contains the desired methods from WP_HTML_Tag_Processor/WP_HTML_Processor with the additional augmented methods for special HTML insertions. This makes sense now as well prior to a 1.0.0 stable version since it is impacting the primary interface used by extensions.

[felixarntz]

@westonruter

So yeah, probably a separate method like ::add_external_style() should be added (eventually). This would be in addition to ::add_inline_style() for inline styles.

Sounds good.

If they had access to the OD_HTML_Tag_Processor then they'd also have access to the ::append_head_html() and ::append_body_html() methods. And otherwise, WP_HTML_Tag_Processor (and WP_HTML_Processor) have many other methods for writing HTML, like ::set_attribute() and ::set_modifiable_text(), so it would seem strange to be able to use the processor instance for some writes but then this other HTML writer class for other writes. I think better to add that abstraction now so from the tag visitor's perspective (and the perspective of od_start_template_optimization and od_finish_template_optimization) for them to still have access to the one processor which exposes an interface which contains the desired methods from WP_HTML_Tag_Processor/WP_HTML_Processor with the additional augmented methods for special HTML insertions.

Sounds good. So basically we would introduce two new classes:

• One for the read interface, basically wrapping WP_HTML_Tag_Processor with a decorator pattern and making it read-only (by exposing only methods that read from the HTML doc).
  • We could either do explicitly expose methods, or use __call(). Since for __call() we would probably still want to include the actual methods names via @method PHPDoc - if so, we may just go with the more explicit route anyway, since either would require manual work per method.
  • It would be nice not to have to do that, but that would go at the cost of DX due to worse method discoverability.
• One for the write interface (also wrapping WP_HTML_Tag_Processor, but only allowing very specific write interactions (instead of arbitrary HTML).
  • So far we would have methods for external style, inline style, external script module right? What about inline script module?
  • I'm still undecided on whether these methods should allow specifying a location (e.g. HEAD, BODY, DOCUMENT) or not. If we don't include this, I think we should implement the methods in a way that it could be added in the future if it became needed, e.g. as an optional new parameter $location. We could still use the most suitable default, depending on what the method is (e.g. style in HEAD by default, script module in BODY by default). Aside: Maybe these identifiers should have more specific names, like HEAD_START, HEAD_END, BODY_START, BODY_END, ... or similar.
  • Didn't we also want to have a method to add HTML comments?

Other question to discuss: Would both the template context and the tag visitor context expose both? Or would only the template context expose the write interface, but both expose the read interface? And what about template context start vs template context finish? What are your thoughts there? I know we discussed this before, but it would be great if we could more clearly define the responsibilities of what tag visitors should do vs what the two hook callbacks for the entire template should do.

[westonruter]

@felixarntz

Sounds good. So basically we would introduce two new classes:
...

• One for the read interface, basically wrapping WP_HTML_Tag_Processor with a decorator pattern and making it read-only (by exposing only methods that read from the HTML doc).
  ...
• One for the write interface (also wrapping WP_HTML_Tag_Processor, but only allowing very specific write interactions (instead of arbitrary HTML).

Actually I think we'd just be introducing one new class which is the OD extension's interface for accessing the underlying tag processor and other relevant methods we expose for writing specific HTML. We could call it OD_HTML_Processor or maybe OD_Extension_HTML_Processor to reflect that it is the processor explicitly used only by extensions to Optimization Detective.

In the end, the tag visitor callback and od_start_template_optimization/od_finish_template_optimization actions would still just access the $context->processor object, and they'd have the same methods exposed on each, for reading and writing. I recant this. This wrapper class would route the calls to the underlying processor instance, whether that uses WP_HTML_Tag_Processor or WP_HTML_Processor under the hood.

• We could either do explicitly expose methods, or use __call(). Since for __call() we would probably still want to include the actual methods names via @method PHPDoc - if so, we may just go with the more explicit route anyway, since either would require manual work per method.
• It would be nice not to have to do that, but that would go at the cost of DX due to worse method discoverability.

For methods that are purely pass-through, yes, we could indeed use __call() with @method phpdoc.

• So far we would have methods for external style, inline style, external script module right? What about inline script module?

Yes, I mention add_inline_script_module() above. That method and add_external_script_module() should always append SCRIPT tags to the end of the BODY because they must come after any importmap script. In contrast, add_inline_style should always append to the HEAD so as to avoid any flash of unstyled content. The only method where I think the location is relevant is add_external_style() where it could either be appended to the HEAD (and be render-blocking) or to the end of the BODY (to not be render blocking). Maybe the specific location shouldn't be specified, the API for external stylesheets can reflect whether it is render-blocking or not. Sometimes external stylesheets are put in the HEAD and are prevented from render blocking by using an onload attribute which changes the media to screen. So that would be an alternative to putting it in the footer. So we could have a parameter for add_external_stylesheet() which is a boolean for whether or not it is blocking, and we can decide which implementation to go with. This said, no extensions to Optimization Detective currently insert external stylesheets.

• Didn't we also want to have a method to add HTML comments?

Yeah, here location is more relevant. It could be:

1. Append to HEAD
2. Append to BODY
3. Insert at the end of the document

However, there aren't any tag visitors that need this yet. So comments could be considered later.

Other question to discuss: Would both the template context and the tag visitor context expose both? Or would only the template context expose the write interface, but both expose the read interface? And what about template context start vs template context finish? What are your thoughts there? I know we discussed this before, but it would be great if we could more clearly define the responsibilities of what tag visitors should do vs what the two hook callbacks for the entire template should do.

Oh, now that you say this, I am reminded that it doesn't make sense for the processor to be exposed in the od_start_template_optimization/od_finish_template_optimization actions because the document either hasn't started being iterated over or it has completely finished being iterated over. This is why in #1919 I had added the append_head_html() and append_body_html() methods to the context object in that case so as to not expose the underlying processor instance (cf. 53a91a2).

So this conflicts with what I had said in #1931 (comment):

If they had access to the OD_HTML_Tag_Processor then they'd also have access to the ::append_head_html() and ::append_body_html() methods. And otherwise, WP_HTML_Tag_Processor (and WP_HTML_Processor) have many other methods for writing HTML, like ::set_attribute() and ::set_modifiable_text(), so it would seem strange to be able to use the processor instance for some writes but then this other HTML writer class for other writes. I think better to add that abstraction now so from the tag visitor's perspective (and the perspective of od_start_template_optimization and od_finish_template_optimization) for them to still have access to the one processor which exposes an interface which contains the desired methods from WP_HTML_Tag_Processor/WP_HTML_Processor with the additional augmented methods for special HTML insertions. This makes sense now as well prior to a 1.0.0 stable version since it is impacting the primary interface used by extensions.

So, all of this to say, yes, I think you're right that there should be two new classes:

1. A processor wrapper class which exposes all of the relevant methods of the underlying processor. This is only exposed in the context provided to the tag visitor callbacks.
2. A document writer class which has an instance of the processor to be able to insert HTML in the HEAD and BODY. This class is exposed on the context objects passed to both the tag visitor callbacks and the od_start_template_optimization/od_finish_template_optimization actions.

😅

[felixarntz]

@westonruter

• We could either do explicitly expose methods, or use __call(). Since for __call() we would probably still want to include the actual methods names via @method PHPDoc - if so, we may just go with the more explicit route anyway, since either would require manual work per method.
• It would be nice not to have to do that, but that would go at the cost of DX due to worse method discoverability.

For methods that are purely pass-through, yes, we could indeed use __call() with @method phpdoc.

What do you think about the trade-off between __call() with @method vs just adding actual one-liner methods? The latter would have the benefit that it could also have actual PHPDoc description etc on it, though my main point for even considering this is that with @method we would have to manually maintain the list of available methods anyway, so to me that makes it less appealing as a shortcut than if we only used __call() and e.g. did something like disallow calling any method on WP_HTML_Tag_Processor that starts with set_*. That alone would be a nice shortcut to not have to cover all method names individually, but then discoverability would be poor.

In contrast, add_inline_style should always append to the HEAD so as to avoid any flash of unstyled content. The only method where I think the location is relevant is add_external_style() where it could either be appended to the HEAD (and be render-blocking) or to the end of the BODY (to not be render blocking).

Why wouldn't the latter apply to inline styles? Of course they don't need to load a file, but isn't processing the CSS still render-blocking? From that perspective, I don't see a fundamental difference in why inline CSS would never need to be at the end of the BODY. Admittedly, it's a nit-pick, and there's a lot more justification to do that for external CSS than inline CSS, but I'm not sure I agree it's only relevant for one but not the other. In any case, see what I'm saying below, we don't really need to discuss this now IMO.

Maybe the specific location shouldn't be specified, the API for external stylesheets can reflect whether it is render-blocking or not. Sometimes external stylesheets are put in the HEAD and are prevented from render blocking by using an onload attribute which changes the media to screen. So that would be an alternative to putting it in the footer. So we could have a parameter for add_external_stylesheet() which is a boolean for whether or not it is blocking, and we can decide which implementation to go with. This said, no extensions to Optimization Detective currently insert external stylesheets.

• Didn't we also want to have a method to add HTML comments?

Yeah, here location is more relevant. It could be:

1. Append to HEAD
2. Append to BODY
3. Insert at the end of the document

However, there aren't any tag visitors that need this yet. So comments could be considered later.

How about for now we go with an implementation that doesn't allow specifying the location at all? We could simply go with the defaults that make the most sense for most cases. If later we come across needs to make it configurable, we can introduce location flags after all - but it seems this right now isn't crucial and is yet another point to figure out. I don't think there is any problem with BC in that regard, since a future location parameter could just be optional, and by default it would use the same behavior we define today.

So:

• add_external_script_module() --> end of BODY
• add_inline_script_module() --> end of BODY
• add_external_stylesheet() --> HEAD
• add_inline_stylesheet() --> HEAD
• add_comment() --> after </HTML>

For the latter, I think there is enough of an established need to add HTML comments from the get-go, e.g. how caching plugins do it. Putting that after the HTML makes most sense to me and is what I'm most commonly seeing. After all, the location of this truly does not matter, and putting it at the very end has, in principle, the least potential to get in the way (although of course you can hardly call an HTML comment render-blocking 😆).

Other question to discuss: Would both the template context and the tag visitor context expose both? Or would only the template context expose the write interface, but both expose the read interface? And what about template context start vs template context finish? What are your thoughts there? I know we discussed this before, but it would be great if we could more clearly define the responsibilities of what tag visitors should do vs what the two hook callbacks for the entire template should do.

So, all of this to say, yes, I think you're right that there should be two new classes:

1. A processor wrapper class which exposes all of the relevant methods of the underlying processor. This is only exposed in the context provided to the tag visitor callbacks.
2. A document writer class which has an instance of the processor to be able to insert HTML in the HEAD and BODY. This class is exposed on the context objects passed to both the tag visitor callbacks and the od_start_template_optimization/od_finish_template_optimization actions.

For the most part, this sounds good to me. I'm pretty sure we discussed it before, but why again does the writer need to be accessible in all three places? Why would an extension want to use a tag visitor to write vs one of the template hooks to write?

[westonruter]

@felixarntz

What do you think about the trade-off between __call() with @method vs just adding actual one-liner methods? The latter would have the benefit that it could also have actual PHPDoc description etc on it, though my main point for even considering this is that with @method we would have to manually maintain the list of available methods anyway, so to me that makes it less appealing as a shortcut than if we only used __call() and e.g. did something like disallow calling any method on WP_HTML_Tag_Processor that starts with set_*. That alone would be a nice shortcut to not have to cover all method names individually, but then discoverability would be poor.

I don't have a strong opinion either way right now. We can try both when implementing to see what works better.

Why wouldn't the latter apply to inline styles? Of course they don't need to load a file, but isn't processing the CSS still render-blocking? From that perspective, I don't see a fundamental difference in why inline CSS would never need to be at the end of the BODY. Admittedly, it's a nit-pick, and there's a lot more justification to do that for external CSS than inline CSS, but I'm not sure I agree it's only relevant for one but not the other. In any case, see what I'm saying below, we don't really need to discuss this now IMO.

Yes, inline styles are render-blocking but since there is no network request to fetch them, the time would be drastically reduced. (This is something else to investigate re: Core-63018.) The vast majority of the time inline stylesheets should be put in the HEAD (and this is so far what tag visitors have done), so I don't see a need to add a way to append inline styles to the end of the BODY now. It could be introduced later as need arises.

How about for now we go with an implementation that doesn't allow specifying the location at all? We could simply go with the defaults that make the most sense for most cases. If later we come across needs to make it configurable, we can introduce location flags after all - but it seems this right now isn't crucial and is yet another point to figure out. I don't think there is any problem with BC in that regard, since a future location parameter could just be optional, and by default it would use the same behavior we define today.

Agreed.

For the latter, I think there is enough of an established need to add HTML comments from the get-go, e.g. how caching plugins do it. Putting that after the HTML makes most sense to me and is what I'm most commonly seeing. After all, the location of this truly does not matter, and putting it at the very end has, in principle, the least potential to get in the way (although of course you can hardly call an HTML comment render-blocking 😆).

SGTM.

For the most part, this sounds good to me. I'm pretty sure we discussed it before, but why again does the writer need to be accessible in all three places? Why would an extension want to use a tag visitor to write vs one of the template hooks to write?

Yes, we discussed before in #1919 (comment) and #1919 (comment) (and others). To quote the second comment, "there are three scenarios for inserting markup:

1. Before the document has processed at od_start_template_optimization. This is where a plugin like OD Admin UI can add styles or scripts that just require information about the OD_URL_Metric_Group_Collection and don't depend on the outcome of tag visitors being invoked (Leverage OD_Template_Optimization_Context westonruter/od-admin-ui#11). However, this could just as well be done at od_finish_template_optimization which is passed the same context. The more important use case for od_start_template_optimization is for tag visitors to do some pre-processing on the URL Metrics to gather up the information they'll need later when walking over the tags, e.g. Leverage od_start_template_optimization action to initialize common LCP external background image data #1921
2. During a tag visitor's callback, which before now has been the only option. This remains a fine option to insert an additional script in the page, for example when a tag visitor comes across a video that should be lazy-loaded, it can append a SCRIPT to the BODY and then set a flag to prevent adding a second script when another lazy-loaded video is encountered. Alternatively, the od_finish_template_optimization action can be used for this instead, although it essentially needs the same flag regardless: Use od_finish_template_optimization action to insert video lazy-load script #1922.
3. After the document has been processed at od_finish_template_optimization. At this point, all tag visitors have run and extensions have an overall lay of the land for what is in the document. They can now compile a single stylesheet for insertion in the HEAD based on the information gathered by the tag visitors, for example, as seen in De-duplicate media queries in Embed Optimizer by merging style rules and inserting after optimizing the document #1923.

[felixarntz]

@westonruter SGTM, except I'm still questioning the following:

2. During a tag visitor's callback, which before now has been the only option. This remains a fine option to insert an additional script in the page, for example when a tag visitor comes across a video that should be lazy-loaded, it can append a SCRIPT to the BODY and then set a flag to prevent adding a second script when another lazy-loaded video is encountered. Alternatively, the od_finish_template_optimization action can be used for this instead, although it essentially needs the same flag regardless: Use od_finish_template_optimization action to insert video lazy-load script #1922.

1. Are there any benefits of inserting HTML in a tag visitor over doing so on od_finish_template_optimization. If not, I don't see a reason to allow it in both places, if only one has benefits over the other, but the other one doesn't.
   • Having the ability to add HTML only on od_finish_template_optimization would tie in nicely with the ability to do so on od_start_template_optimization.
   • Additionally, it would clearly separate responsibilities: Use a tag visitor to read data from the HTML, use one of the two template hooks to amend the HTML.
   • I think that's a more intuitive API than to say "you can amend HTML here or there, but there's no real benefit either way".
2. Regarding setting a flag, I think this would no longer be necessary if we implemented in the new HTML writer class the behavior to never include the same script or stylesheet more than once?

[westonruter]

@felixarntz:

Are there any benefits of inserting HTML in a tag visitor over doing so on od_finish_template_optimization. If not, I don't see a reason to allow it in both places, if only one has benefits over the other, but the other one doesn't.

A benefit for allowing markup insertion in a tag visitor is it allows for a simplified code architecture. See #1919 (comment). While Embed Optimizer can inject cleaner stylesheets if it waits until od_finish_template_optimization to inject the styles (#1923), this doesn't really entail a performance benefit. For the sake of allowing optimization logic to be self-contained in a tag visitor callback closure, I'd like to be able to do the HTML writes from that context as well.

• Additionally, it would clearly separate responsibilities: Use a tag visitor to read data from the HTML, use one of the two template hooks to amend the HTML.

A tag visitor doesn't just read from the HTML, however. It also can write, such as making changes to tag attributes.

Regarding setting a flag, I think this would no longer be necessary if we implemented in the new HTML writer class the behavior to never include the same script or stylesheet more than once?

The HTML writer can indeed handle this de-duplication, but it could involve more work to do so. For example, when adding an inline script module, if the HTML writer were relied on only to de-duplicate, then it would need to iterate over all existing queued inline script modules to see if it already was added, and if so, abort. Granted, this will be fast, but it's slower than checking a boolean flag.

[felixarntz]

@westonruter

A benefit for allowing markup insertion in a tag visitor is it allows for a simplified code architecture. See #1919 (comment). While Embed Optimizer can inject cleaner stylesheets if it waits until od_finish_template_optimization to inject the styles (#1923), this doesn't really entail a performance benefit. For the sake of allowing optimization logic to be self-contained in a tag visitor callback closure, I'd like to be able to do the HTML writes from that context as well.

WFM 👍

A tag visitor doesn't just read from the HTML, however. It also can write, such as making changes to tag attributes.

Good point raising that. So those more specific writes (let's call it updates rather than adding actual HTML) would still happen via regular tag processor methods, correct? Only any kind of additions of HTML / new DOM elements would be prevented and only allowed via our own writer, correct?

Regarding setting a flag, I think this would no longer be necessary if we implemented in the new HTML writer class the behavior to never include the same script or stylesheet more than once?

The HTML writer can indeed handle this de-duplication, but it could involve more work to do so. For example, when adding an inline script module, if the HTML writer were relied on only to de-duplicate, then it would need to iterate over all existing queued inline script modules to see if it already was added, and if so, abort. Granted, this will be fast, but it's slower than checking a boolean flag.

I don't think this would be substantially complex or slow to add. We could simply add a map to look up keys via isset() - no iteration needed. For example:

• For external scripts/stylesheets, something like isset( $queued[ $asset_url ] ).
• For inline scripts/stylesheets, something like isset( $queued[ $asset_code ] ).