Skip to content

Conversation

@costdev
Copy link
Contributor

@costdev costdev commented Aug 18, 2021

handle_legacy_widget_preview_iframe() called wp_footer() on each widget on the Appearance > Widgets screen.
This patch removes the call to wp_footer().

PR Update
Following discussion on Trac, a CSS solution is simpler and suggests equal results. The updates to this PR remove the initial changes and implement a CSS based solution instead.

Discussion on Trac included feedback from the accessibility team who have no specific concerns at this moment. Some keyboard only navigation and screen reader testing of the legacy widget preview screen is desirable.

Trac ticket: https://core.trac.wordpress.org/ticket/53801

@draganescu
Copy link

How about if we hide what these hooks output here in a display none div?

@costdev
Copy link
Contributor Author

costdev commented Aug 24, 2021

While this could work for wp_footer(), it would return invalid markup for wp_head().

As the docs specifically state that both functions are intended for output to the frontend, we would ideally not call these functions in the widget editor.

However, to reduce BC breaks, we can output enqueued scripts and styles to ensure that the widgets' functionality and styling is maintained in the widget editor.

Thoughts?

@draganescu
Copy link

Yes, I think that could work. Using the global $wp_scripts, global $wp_styles we could just output these. Could you move this PR in that direction?

Hopefully not using wp_footer will not break other things.

@Clorith
Copy link
Member

Clorith commented Aug 26, 2021

Some plugins and themes print styles and JS directly within those action hooks, when they find adding extra assets to be overkill, so just trying to print enqueued scripts or styles may have a negative impact on those scenarios.

I don't have the solution either, but all these scenarios need to be mentioned and considered.

@costdev
Copy link
Contributor Author

costdev commented Aug 26, 2021

I've been thinking about this as well @Clorith.

To the best of my knowledge, we don't currently have a way to get widget-specific additions to wp_head() and wp_footer(). If this is correct, we have at least two options:

  1. "WP... finds a way." - Dr. Ian Malcolm - I'm considering a potentially non-breaking option to get widget-specific additions at the moment. Will update if it pans out.
  2. Don't be focused on being widget-specific and think instead about reducing the output of wp_head() and wp_footer() on the Widget Block Editor screen to just scripts and styles.

Based on the discussion so far and current limitations, my thinking for my next update to this PR is:

  1. Capture output for wp_head() and wp_footer() using ob_*() functions.

  2. Parse for:

    both wp_head() and wp_footer():

    • <script *... src='*' *...>
    • <script *...>*</script>

    wp_head() only:

    • <link *... href='*' *...>
    • <style *...>*</style>
  3. Save the parsed wp_head() and wp_footer() data to separate strings.

    • The aim of this is to do (1) and (2) only once instead of per widget.
  4. Output the data at the top or bottom of each widget's iframe based on which function called it.

Naturally, in addition to any extras (do we care about any meta tags for example?) performance is a crucial element.

Reducing the calls to wp_head(), wp_footer() and the parsing process above to just one call each is one part of reducing the performance impact of this.

@draganescu
Copy link

@costdev I like that output buffering + "parsing" option. It is not very "elegant" but it serves very well the need to preview only the widget but also make sure all the styles and scripts are present 👏🏻

@hellofromtonya
Copy link
Contributor

The e2e tests are failing on the Ensure version-controlled files are not modified or deleted job. Isn't the file being modified part of the published @wordpress/widgets package? If yes, should it be modified in Gutenberg? cc @costdev @draganescu

@costdev
Copy link
Contributor Author

costdev commented Nov 3, 2021

The e2e tests are failing on the Ensure version-controlled files are not modified or deleted job. Isn't the file being modified part of the published @wordpress/widgets package? If yes, should it be modified in Gutenberg? cc @costdev @draganescu

That's right, it will need a change upstream. Noted here and at the bottom of this comment.

I'm not 100% sure if it will require the changes in both repos or if the change upstream will be enough and the build process will update the legacy-widget.php file in Core. I think an upstream change will be enough.

@hellofromtonya
Copy link
Contributor

Closing this PR as the fix needs to be done in Gutenberg, the package published, and then brought over to Core via a @wordpress package update.

Here's the GB issue and PR that will resolve the issue.

@costdev costdev deleted the #53801 branch September 19, 2022 21:07
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.

4 participants