[wp-trac] [WordPress Trac] #59443: Block Supports: Re-use instance of Tag Processor when adding layout classes

[WordPress Trac]

#59443: Block Supports: Re-use instance of Tag Processor when adding layout classes
---------------------------------------+----------------------------
 Reporter:  dmsnell                    |       Owner:  isabel_brison
     Type:  enhancement                |      Status:  reopened
 Priority:  normal                     |   Milestone:  6.4
Component:  Editor                     |     Version:  trunk
 Severity:  normal                     |  Resolution:
 Keywords:  gutenberg-merge has-patch  |     Focuses:  performance
---------------------------------------+----------------------------

Comment (by dmsnell):

 thanks for your continued diligence in this @spacedmonkey - I do suggest
 first though that you revert the patch and run your benchmarks against
 that revert, as I've asked several times already, as that will give us a
 clear answer to the questions about this particular refactor and you won't
 be spending your time trying to refactor if that doesn't turn up any
 results.

 I'm still confused why we keep discussing this when we don't have any
 comparisons before and after this patch applies to demonstrate that this
 is a problem or introduced a regression.

 on your other questions though:

 > Can I understand why this function was changed and no unit tests seem to
 have been added?

 As stated in the [https://github.com/wordpress/gutenberg/pull/54075
 original description of the changes], this function was updated to address
 some hygiene issues with the HTML API in Core and to prevent future
 breakage because of the way this was written. I didn't catch it when the
 Tag Processor was initially introduced and I am trying to ensure that Core
 sets proper examples because people will be copying them. In the
 description above I also repeated that this change is necessary to prevent
 a breakage to Core when an oddity in the Tag Processor is fixed later on.

 As a refactor with no functional changes I was relying on existing tests,
 and also didn't know what should be tested or how. My goal was to make
 things clearer and remove an obvious inefficiency.

 > Still trying to understand the point of this ticket. This seems like a
 code refactor. Considering there are rules around this, see the docs…What
 new functionality or bug does this code fix?

 These have been repeated in multiple places, so what's still unclear? I'm
 sorry I'm having trouble clearly communicating this.

 > WP_HTML_Tag_Processor is done on inner content in a while loop
 > While loop here.

 Ah okay, well this isn't creating new tag processor instances here. It's
 scanning through the HTML tags to try and find the wrapping HTML element
 for the inner blocks. As noted in this patch, after spending much effort
 to figure out what the original purpose of the code was, this is not a
 reliable or particularly great way to do this, and something better surely
 exists.

 Don't shoot me: I only tried to make it clearer after my own
 investigation.

 Also please take note that before this change it was normative to create
 //three// instances of the Tag Processor and afterwards there's only
 //one//.

 ---

 @hellofromTonya I opened #59544 as part of addressing the overall
 performance issues in the 6.4 branch, but unrelated to this ticket. I'm
 still waiting to see evidence that this ticket introduces a performance
 regression. It addresses the rendering pipeline, where I have been seeing
 extra time spent during the render of the homepage for `twentytwentyfour`.

 For the big changes to this function, around which this discussion is
 taking place, introduced in #57584, there's an obvious oddity in how it's
 trying to find the inner block wrapper, assuming it exists. That's a great
 question that maybe @isabel_brison or @flixos90 can help elucidate. We're
 scanning HTML on render to find a place that we ostensibly could indicate
 directly at a different stage of the render process. I'm sure this is hard
 stuff and other options were tried. Before #57584 the inherent logic was
 simpler, so it's not a surprise to me that we're seeing different
 performance characteristics afterwards.

 This being said, I have some ideas for how to address this more broadly,
 but probably not in a way that we can address it for 6.4. I think there's
 a reasonable idea in there somewhere to mark block boundaries during
 render and then remove them on final output for pages. There are some
 opportunities with Unicode ranges that allow us to do this in a safe and
 benign way, but any broad fix is going to need lots of input and vetting
 before pushing it out; not a beta-timeline process IMO.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/59443#comment:32>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform