Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 22 months ago

#53801 closed defect (bug) (reported-upstream)

Block-based Widgets Screen does action wp_footer after each Widget

Reported by: madtownlems's profile MadtownLems Owned by:
Milestone: 5.9 Priority: normal
Severity: major Version: 5.8
Component: Widgets Keywords: has-patch has-testing-info
Focuses: Cc:

Description

I've reported this on Github already (https://github.com/WordPress/gutenberg/issues/33580), but I'm really not sure the right place to post issues these days so figured I'd post here as well to make sure it gets looked at before 5.8.1

WordPress 5.8's Block-based Widgets screen does action wp_footer in between each widget. This action has historically been reserved for only the front-end of the site (as admin_footer is the back-end version). It's where many plugins output things in the footer.

This can result in a very broken and ugly widgets screen, with all sorts of unintended content showing up. (In our specific case, we noticed because a bunch of standardized legal text and brand images were showing, but I've read other reports of GPDR notices or "chat with us" implementations as well.)

Sample code for proof of concept:

<?php
add_action( 'wp_footer', 'wp_footer_proof_of_concept' );

function wp_footer_proof_of_concept() {
echo "Why is this being output on the Widgets screen?";
}

Attachments (6)

126395788-1f75c085-0ffd-4181-8905-63df71863354.png (36.8 KB) - added by MadtownLems 3 years ago.
widget-1.png (13.3 KB) - added by ravipatel 3 years ago.
widget-2.png (34.5 KB) - added by ravipatel 3 years ago.
53801_css01.diff (740 bytes) - added by costdev 3 years ago.
53801_css02.diff (1.2 KB) - added by costdev 3 years ago.
CSS #02: Hide root-level text nodes, non-widget elements and pseudo-elements.
53801_css03.diff (1.2 KB) - added by costdev 3 years ago.
Remove width. Add !important to transform.

Download all attachments as: .zip

Change History (62)

#1 @knutsp
3 years ago

  • Severity changed from normal to major
  • Version set to 5.8

#2 @sabernhardt
3 years ago

  • Milestone changed from Awaiting Review to 5.8.1

@ravipatel
3 years ago

@ravipatel
3 years ago

#3 @MadtownLems
3 years ago

@ravipatel : Can you please confirm what the Appearance->Widgets screen looks like in that situation? widget-2.png appears to show the front-end, where this bug is about extraneous output in the backend.

thanks!

#4 @mukesh27
3 years ago

  • Keywords needs-patch added

Yes. It's a bug for the admin widget page.

I quickly check the issue and found that wp_footer called in the https://github.com/WordPress/WordPress/blob/master/wp-includes/blocks/legacy-widget.php#L123 file so it was loading all footer action in the WordPress Widgets screen.

If we remove/comment wp_footer from that location it will fix the issue. Let's wait for the Gutenberg team's feedback on this.

This ticket was mentioned in PR #1602 on WordPress/wordpress-develop by costdev.


3 years ago
#5

  • Keywords has-patch added; needs-patch removed

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

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

#6 @costdev
3 years ago

I've submitted a patch so that the change is ready for code review and commit - should the final word be to remove wp_footer() from legacy widget previews.

I see that @peterwilsoncc has noted on the Github issue that wp_head() is also called on each legacy widget preview, which when hooked via add_action( 'wp_head' ..., also outputs for each legacy widget.

What are your thoughts on removing this as well?

#7 @peterwilsoncc
3 years ago

I am not sure what the best solution here is.

Some widgets will depend on assets included in wp_head() and wp_footer() for either styling or JavaScript needed for rendering the widget preview. For example the Jetpack's social icons widget alongside several other plugins with widgets in the most popular plugins.

The problem stems from unrelated plugins resuming the front-end for the actions a historically that's been the case and outputting features there as a result, be it analytics code or something that is displayed.

Some possible solutions are:

  • Remove the front-end actions and use new wp_widget_preview_* in their place -- this will require all themes to enqueue default styles especially for the widget previews
  • Leave them in place -- requiring plugins to check is_admin() when enqueueing/adding anything not needed for the widget preview

Both of these seem like bad choices, so I welcome any other suggestions.

As I said, I'm not sure what the best solution is as either seem like a backward compatibility break but it's already been broken with using the formerly front-end only actions in the admin.

#8 @costdev
3 years ago

I agree that both options risk some bc breaks, but getting all ideas out on the table is exactly what we need!

A thought regarding the styling or Javascript needed by legacy widgets. We could get the enqueued styles and scripts for the legacy widget, and output them to the legacy widget preview iframe, thereby skipping over any other output. We should be able to get those specific scripts/styles from the queue, as that's the key function that wp_head() and wp_footer() are carrying out in this scenario?

Version 0, edited 3 years ago by costdev (next)

draganescu commented on PR #1602:


3 years ago
#9

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

costdev commented on PR #1602:


3 years ago
#10

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?

#11 @eatingrules
3 years ago

So I came across this today since I noticed our script was being loaded multiple times in the widgets.

Proof of concept... This will load /test.js once for each legacy widget in the editor.

add_action( 'wp_enqueue_scripts', 'enqueue_test_script' );

function enqueue_test_script() {
	wp_enqueue_script( 'enqueue-test', '/test.js' );
}

Hiding the output of wp_footer with display:none would not have solved this, since our JavaScript is a third-party script that displays a help widget in the lower-right corner of the screen. So it was loading in the bottom corner of each legacy widget.

I also saw this on a clients' site today. She has an email opt-in popup -- it popped up and obscured every widget on the page. Made it impossible to edit anything, of course.

As a temporary workaround in our plugin, we're going to add a check for is_admin(), as @peterwilsoncc suggested above... but that definitely seems a bit clunky.

I think continuing to output JavaScript is likely to cause strange behaviors or other errors (especially if that JavaScript does anything to modify the content). And, it's being loaded multiple times which also isn't ideal.

Perhaps a better solution is for a "Legacy Widget" to actually look and behave like an old-style widget, with no actual preview?

draganescu commented on PR #1602:


3 years ago
#12

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 commented on PR #1602:


3 years ago
#13

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 commented on PR #1602:


3 years ago
#14

I thought 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 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:
  • - <script *...src='*' *...>
  • - <link *...href='*' *...>
  • - <script *...>*</script>
  • - <style *...>*</style>
  1. Save the parsed wp_head() and wp_footer() data to separate strings
  2. The aim is to do all of the above only once instead of per widget.
  3. 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.

#15 @costdev
3 years ago

Actually, my comment above on the PR will still result in the email opt-in case that @eatingrules raised. I'm not sure that type of case is resolvable via Core and would likely need plugin/theme developer action - a situation we're trying to avoid since we're specifically dealing with Legacy widgets.

The proposed solution by @eatingrules to remove previews for legacy widgets might be the best option for now.

To quickly see what this will look like in your own installs:

  1. Go to Appearance > Widgets.
  2. Open DevTools (F12).
  3. In the console, paste the following and press enter:
const legacy = document.querySelectorAll( '.wp-block-legacy-widget' );

legacy.forEach( ( widget ) => {
    const form    = widget.querySelector( '.wp-block-legacy-widget__edit-form' ),
          preview = widget.querySelector( '.wp-block-legacy-widget__edit-preview' );

    form.removeAttribute( 'hidden' );
    preview.style.display = 'none';
});

Note: Don't hook the code above into a production site - clicking on and off a legacy widget will try to return to the preview (which the code above has now hidden). The code above is for a visual example of legacy widget rendering as per @eatingrules proposed solution.

Last edited 3 years ago by costdev (previous) (diff)

#16 @eatingrules
3 years ago

Ooh, that's a neat trick in the console, @costdev! 😎

I just tested that on my clients' site that has all the legacy widgets with the opt-in popups. This is a much better solution than trying to make Legacy Widgets have a preview!

In addition to solving the compatibility issues with wp_head/wp_footer, it has the benefit of reinforcing their "Legacy" status. It makes the distinction between the new/legacy widgets much more clear, which will actually help people understand the difference and see the benefit in moving to new block widgets.

I think this change actually makes for a better editing experience as well. It's jarring to click on the widget and have it completely change to the form view -- clicking a legacy widget triggers an unexpected behavior/state shift. On top of that, the size the form usually changes, so things move around on the page, which is disorienting (it's especially bad for a widget with a lot of options/form fields).

It's also worth noting that the popular Genesis Framework has completely disabled the new Block Widget Editor (due to compatibility issues, I presume), and I know of at least one other theme that has had significant issues with the new editor as well. So this solution may allow them to restore the functionality for their users and help further the adoption of the new editor.

draganescu commented on PR #1602:


3 years ago
#17

@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 👏🏻

costdev commented on PR #1602:


3 years ago
#18

See [this comment https://core.trac.wordpress.org/ticket/53801#comment:15] on Trac for the reason why the possible solution I'd posted that @draganescu referenced above won't be suitable for all cases and a proposal to remove legacy widget previews, instead showing only the form for the legacy widget, until such time as we reach a better solution.

costdev commented on PR #1602:


3 years ago
#19

See this comment on Trac for the reason why the possible solution I'd posted that @draganescu referenced above won't be suitable for all cases and a proposal to remove legacy widget previews, instead showing only the form for the legacy widget, until such time as we reach a better solution.

#20 @andraganescu
3 years ago

I think, we need to make the best compromise in core between the problems at hand. The widget previews are a feature that the block editor finally makes possible and it is a fundamental part of the experience enhancement it brought: the ability to preview before save, unlike live editing of theme content in the classic screen.

Disabling previews should not be the result of a few special case plugins that use the widget system to display some very specific UI that has to be rendered fixed.

I still believe that @costdev 's idea of parsing through wp_footer output for styles and scripts, is a great balance of removing content unrelated to the previewed widget, that happens to be output in wp_footer, while also keeping the previews functional and styled. It is prefferable for the plugins providing the widgets that can't be contained in the previews, even after this optimisation (if it works), to update their approach, rather than change core to fit this specific situation.

#21 follow-up: @zieladam
3 years ago

Using wp_head and wp_footer in the preview may indeed add some unexpected content there. At the same time, not having the preview seems like a broken experience. Either of these seems imperfect and I don't think there is a solution that could make things 100% right.

My two cents are that we probably can't solve all the cases here, but we likely can solve most of them. To get more practical: We want to hide any extra content a) rendered by PHP and b) added in the runtime by javascript. Assuming iframed preview, I wonder if this rule would already account for most cases: body > *:not(.allowlisted_preview_div) { display: none !important; pointer-events: none !important; opacity: 0 !important; } .

Sure, it is possible to write a script that will overcome that. But how often does that actually happen?

Last edited 3 years ago by zieladam (previous) (diff)

#22 @eatingrules
3 years ago

I agree that with any new Block Widgets the live preview is an improvement. But if widget previews don't display properly and reliably (with at least a close approximation of the front end), it's not an improvement it's an impediment. This will lead to user frustration and will hurt adoption.

Don't get me wrong, I agree it would be nice if Legacy Widget previews would work well...but with so many variables here, I can't imagine that will ever happen reliably enough to make it worth it. Even the suggestions above came with acknowledgements that they won't be 100% reliable.

From what I've seen on sites so far, the preview for Legacy Widgets is an absolute disaster -- even if scripts/styling from wp_head or wp_footer are not interfering. I have yet to open the widget editor on a production site and have anything even close to a good experience.

Here's a screenshot of one site I own, for a quick example: https://share.nrdp.rs/4gul2G0W

(There's an error at the top of the page, Sucuri Firewall is blocking some simple text widgets, others don't display at all, and the one that does display, is wider than the front end and the images are not aligned properly.)

Looking at this from a non-developer user's perspective (which are all of my clients!), the experience of clicking on a legacy widget's preview, and then having the state change where it turns into a form field, is quite jarring. It's a mode-shift that is not user-friendly, not expected, and not intuitive for most users.

Removing the preview on legacy widgets (since everyone seems to agree the preview will never be 100% reliable/accurate) will accelerate the transition/adoption of block widgets, while making them more reliable and more user-friendly in the meantime.

Further, if Legacy Widgets do not have a preview, then it will encourage developers to update their widgets (or modern replacements will be created by others).

It's far better to have an consistent/reliable experience in the block widget editor, than for users to get frustrated and install the Classic Widgets plugin.

Having said all that, if you're determined to keep Legacy Widget Previews, how about adding an editor setting to disable the legacy widget previews? And maybe also a filter that can be added to disable them? (I'd certainly add this to our clients' sites immediately!)

Sorry that got a bit long-winded. Let me sum up:

  1. If legacy widgets can't display an accurate, or close-to-accurate, preview ~99% of the time, it's better not to have the preview.
  1. Mode switching between the preview and the classic/form-field editor is jarring/confusing.
  1. Not having previews on legacy widgets will encourage developers to update/create new block widgets.
  1. Inaccurate/problematic previews on legacy widgets will likely encourage users to install the Classic Widgets plugin, further slowing adoption of the new editor.
  1. How about a block editor setting, and also a filter for developers, to disable the Legacy Widget previews?

#23 in reply to: ↑ 21 @costdev
3 years ago

Replying to zieladam:

Using wp_head and wp_footer in the preview may indeed add some unexpected content there. At the same time, not having the preview seems like a broken experience. Either of these seems imperfect and I don't think there is a solution that could make things 100% right.

My two cents are that we probably can't solve all the cases here, but we likely can solve most of them. To get more practical: We want to hide any extra content a) rendered by PHP and b) added in the runtime by javascript. Assuming iframed preview, I wonder if this rule would already account for most cases: body > *:not(.allowlisted_preview_div) { display: none !important; pointer-events: none !important; opacity: 0 !important; } .

Sure, it is possible to write a script that will overcome that. But how often does that actually happen?

In the interests of furthering this issue towards a workable resolution, I'm happy to submit patches that follow the solutions that are up for consideration. By applying the patches and experiencing the results within your own installs (non-production please!), we can get more well-rounded feedback.

To that end, I'm about to add a patch with a CSS solution (similar concept to yours @zieladam, but it also handles loose text nodes). While still possible to overcome this solution, I believe you'd need to add content specifically to the .widget itself to do so. However, test, test, test.

EDIT

It's worth noting that:

  1. Non-widget content with inline styles containing !important will overcome even the most robust CSS solution.
  2. A script that has a delay for any reason, such as a slow request or setTimeout(), before it adds non-widget content will overcome even the most robust JS solution. That is, unless the JS solution had a setInterval() that checked for non-widget content, which... is an idea that I don't think any of us has had enough to drink tonight to support.

As it stands, the CSS solution above is promising, but we'd have to add a number of CSS rules to make overcoming it feel like unnecessary/improbable effort.

e.g.

body > *:not(#page),
#page > *:not(#content),
#content > *:not(.widget),
body > *[style]:not(#page),
#page > *[style]:not(#content),
#content > *[style]:not(.widget) {
   display:        none !important;
   font-size:      0 !important;
   height:         0 !important;
   left:           -9999px !important;
   max-height:     0 !important;
   max-width:      0 !important;
   opacity:        0 !important;   
   pointer-events: none !important;
   position:       absolute !important;
   top:            -9999px !important;
   transform:      translate(-9999px, -9999px) !important;
   visibility:     hidden !important;
   width:          0 !important;
   z-index:        -999 !important;
}
Last edited 3 years ago by costdev (previous) (diff)

@costdev
3 years ago

#24 @costdev
3 years ago

  • Keywords needs-testing added

#25 @zieladam
3 years ago

@costdev I just tried that patch and, after adding !important to all the new CSS rules, it does the trick for me without affecting the preview. Thank you!

As it stands, the CSS solution above is promising, but we'd have to add a number of CSS rules to make overcoming it feel like unnecessary/improbable effort.

Adding some additional rules to target width, height etc. makes a lot of sense to me. I am not sure how I feel about the proliferation of #page > :not(#content) rules – it seems a bit excessive.

Either way, I would say now it's all about finding out whether or not we can find a popular plugin or a script that would unintentionally break it. My guess is "no", but I'm happy to be wrong here.

Last edited 3 years ago by zieladam (previous) (diff)

#26 @desrosj
3 years ago

  • Milestone changed from 5.8.1 to 5.8.2

While this is something I'd love to see fixed as soon as possible in 5.8.1, I am very hesitant to include this.

I think once there's a solution everyone feels is going in a good direction, it should be called out in Dev Chat for further feedback, and it should sit in trunk for a while for more testing before it gets backported and included in a minor release.

Punting to 5.8.2 with 5.8.1 RC in less than 24 hours.

#27 @seedsca
3 years ago

It seems to me that the best way to deal with this is not enabling this feature when there are legacy widgets active and show a notice of how much better the widget editing workflow would be if the site was using the Block-based Widgets instead.

Slow, but safe.

@costdev
3 years ago

CSS #02: Hide root-level text nodes, non-widget elements and pseudo-elements.

#28 follow-up: @costdev
3 years ago

I've added an updated patch that contains the additional rules. A typo in the description - I removed pseudo-element coverage before uploading, as it's something we haven't discussed the likelihood of this or a need to target them.

@zieladam

I am not sure how I feel about the proliferation of #page > :not(#content) rules – it seems a bit excessive.

I agree, however both #page and #content could have content added to them by scripts, so this is part of strictly focusing on "Hide any extra content a) rendered by PHP and b) added in the runtime by javascript".

Either way, I would say now it's all about finding out whether or not we can find a popular plugin or a script that would unintentionally break it. My guess is "no", but I'm happy to be wrong here.

Absolutely. For anyone reading this, please apply this patch and let us know if you have any unexpected additional content in your legacy widget previews. If you know what plugin or theme is adding the extra content, please let us know!

@seedsca

It seems to me that the best way to deal with this is not enabling this feature when there are legacy widgets active and show a notice of how much better the widget editing workflow would be if the site was using the Block-based Widgets instead. Slow, but safe.

Now that legacy widget previews have already been released, I think removing them is no longer an option. However, an unintrusive or one-time notice may be worth adding at some point. I'm not sure that that's within the scope of this ticket, but if you do open a new ticket, I'd be happy to contribute to that discussion.

Last edited 3 years ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core-css by costdev. View the logs.


3 years ago

#30 follow-up: @Hareesh Pillai
3 years ago

Defining both width and max-width as 0 seems redundant. One of them can be removed.

This ticket was mentioned in Slack in #accessibility by costdev. View the logs.


3 years ago

#32 in reply to: ↑ 30 @costdev
3 years ago

Replying to Hareesh Pillai:

Defining both width and max-width as 0 seems redundant. One of them can be removed.

Indeed, thanks!

To all, running changes for the next iteration are:

  1. Remove width: 0 !important, as max-width: 0 !important will override this anyway.
  2. Add !important to transform: translate(-9999px, -9999px).

I've also raised this ticket in CSS and Accessibility channels. Pending further discussion, I'll be aiming to upload the next iteration on Monday. In the meantime, please test the current patch and let us know if you find any plugins/themes that successfully inject content into the legacy widget previews.

Only test existing plugins/themes - We're all experienced enough to know how to break this patch. We're specifically focusing on how likely a break is in the wild.

Last edited 3 years ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


3 years ago

#34 @ryokuhi
3 years ago

Thanks @costdev for the feedback request. We discussed this ticket today during the weekly Accessibility Team's bug-scrub.

We had a look at the latest patch: if its purpose is to hide non-widget elements not only visually, but also to assistive technologies, then the display: none; rule should be enough.
No team member has tested the legacy widget preview screen yet: a round of testing using keyboard only navigation and with a screen reader might help in pointing out other possible accessibility issues, but at the moment we don't have specific concerns.

A possible unexpected result of hiding non-widget elements is that, in case the widget requires some content located in the footer (for example, if the widget triggers a modal contained in wp_footer), it would be impossible to replicate that behaviour in preview mode. We don't know of any specific example of such an implementation and this wouldn't be an accessibility issue, but it's worth taking in consideration such a possibility.

#35 in reply to: ↑ 28 @eatingrules
3 years ago

Replying to costdev:

I've added an updated patch that contains the additional rules.

I just tested the patch on a site that was having these issues and it worked well. It hid three different script-based items that were getting in the way.🎉

Nevertheless, the previews still looked pretty bad since the block width is much wider than the actual sidebar width. (I realize this is a separate issue...I searched trac briefly but didn't find an open ticket on that yet?)

My preference would still be to avoid legacy widget previews altogether, for reasons I stated earlier in this thread... especially the "mode shifting" concerns, which I still find jarring.

Now that legacy widget previews have already been released, I think removing them is no longer an option.

I don't see a problem with going back on the legacy widget previews, since it won't break anything to stop showing previews.

Alternatively, an option to disable the legacy widget preview would be a good "failsafe" so that if the proposed patch doesn't catch all situations, at least then the user could toggle the previews off.

#36 @costdev
3 years ago

@ryokuhi Thanks a lot for this feedback!

A possible unexpected result of hiding non-widget elements is that, in case the widget requires some content located in the footer (for example, if the widget triggers a modal contained in wp_footer), it would be impossible to replicate that behaviour in preview mode. We don't know of any specific example of such an implementation and this wouldn't be an accessibility issue, but it's worth taking in consideration such a possibility.

I agree that there are a number of edge cases that may be encountered - This presents a negative both in the existing release of Core (a modal appearing within the legacy widget preview may be larger than the preview container) and in the fix for the issue this ticket raises (no visible modal at all).

@eatingrules

I just tested the patch on a site that was having these issues and it worked well. It hid three different script-based items that were getting in the way.🎉

Excellent!

I think that the remainder of your comment highlights where the scope of this ticket and legacy widget preview feedback diverge. The current patch, with some adjustments as mentioned in an earlier comment I posted, appears to present a workable solution to the issue that this ticket raises. Our edge cases are for now hypothetical in the wild - however, I'm itching to find one! - I think this ticket is possibly coming to a close soon after some additional testing.

With regards to the future of legacy widget previews, I think an option to disable legacy widget previews is the best way forward. However, this should be raised in another ticket, probably as an enhancement, tagging ui, accessibility and admin to get some well-rounded input on this. If you're interested in raising the ticket and want to discuss anything about it beforehand, feel free to DM me on Slack.

#37 @eatingrules
3 years ago

Thanks @costdev -- I just opened #54075 to split off the conversation about disabling the legacy widget previews.👍😁

@costdev
3 years ago

Remove width. Add !important to transform.

#38 @EkoJr
3 years ago

@peterwilsoncc I would be more in favor with the is_admin() option (2nd option). Also, there's admin_enqueue_scripts and wp_enqueue_scripts, and plugins can check the WP_Screen object if enqueue scripts hook or wp_enqueue_script() needs to be used (either to avoid or include on widgets id).

It will drop performance much like adding admin_enqueue_scripts on every admin screen w/o checking the screen id, as well as not checking is_admin() on/for wp_enqueue_scripts. Technically, this situation already occurs with some plugins, but Option 2 would increase the potential & severity of it occurring. However, it would be the least amount of risk to breaking, and would leave optimizations up to the plugin authors.

If option 2 ends up being the decision, and currently is the option to use to avoid the issue, it would be a good idea to make plugin authors aware of the performance risks. At least until a solution is added, because it can cause a significant loss in performance.

Meanwhile, anyone having the same issue can implement this concept for enqueuing scripts & styles.

function proof_of_concept() {
	if ( is_admin() ) {
		$screen = get_current_screen();

		if ( 'widgets' === $screen->id ) {
			wp_enqueue_script( 'dev-admin-widgets' );
			// OR.
			add_action( 'admin_enqueue_scripts', 'dev_admin_widgets_enqueue_func' );
		} else {
			wp_enqueue_script( 'dev-admin-other' );
			// OR.
			add_action( 'admin_enqueue_scripts', 'dev_admin_other_enqueue_func' );
		}
	} else {
		wp_enqueue_script( 'dev-frontend' );
		// OR.
		add_action( 'wp_enqueue_scripts', 'dev_frontend_enqueue_func' );
	}
}
add_action( 'current_screen', 'proof_of_concept' );
Last edited 3 years ago by EkoJr (previous) (diff)

#39 @costdev
3 years ago

@EkoJr Option 2 raises several concerns for me.

  1. Performance impact. The CSS solution doesn't have this - the * selector, while general by intent, has negligible performance impact, especially with the various :not() modifiers.
  2. Option 2 is a breaking change which requires updates by both extenders and website administrators. Without automatic plugin updates enabled, and with third party premium plugins often only being purchased for a single annual licence thereby removing the possibility to update the plugin, any website using plugins that require these updates will break if automatic Core updates are enabled.
  3. While the CSS solution isn't a 100% guarantee, we've been unable to locate a real-world example where the CSS solution fails or causes breaking changes.

For the reasons above, I believe that the latest patch is our best course of action to resolve this ticket's issue while preventing breaks.

#40 @costdev
3 years ago

PR update

CI is failing on Ensure version-controlled files are not modified or deleted, as the modified file doesn't match @wordpress/widgets/src/blocks/legacy-widget/index.php.

I've opened an issue on the Gutenberg repo here to request changes to this file so that CI can pass.

While this discussion leads to majority support for the CSS solution after testing and accessibility feedback, I've also welcomed further comment from the folks in the Gutenberg repo. For those watching this ticket, I'd suggest you keep an eye on the issue there as well to stay fully updated, as this may require changes in both repos.

Last edited 3 years ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core by costdev. View the logs.


3 years ago

#42 @hellofromTonya
3 years ago

  • Keywords has-testing-info added

A Test Report and step-by-step instructions to test are provided here in the issue on the Gutenberg repo.

This ticket was mentioned in Slack in #core-test by justinahinon. View the logs.


3 years ago

#44 @circlecube
3 years ago

@costdev

I've opened an issue on the Gutenberg repo here to request changes to this file so that CI can pass.

Seeing this ticket is closed now, has it been merged upstream? No other updates or status on that ticket other than it being closed soon after it was opened: https://github.com/WordPress/gutenberg/issues/35168.

If it's ready we'd love to include it in 5.8.2 RC which is due today, if not, I'm afraid we'll need to punt to future.

#45 @desrosj
3 years ago

  • Milestone changed from 5.8.2 to Future Release

It looks like GB-35168 was superseded by GB-33580, which is still open so I'm going to punt this one.

#46 @andraganescu
3 years ago

  • Keywords commit added; needs-testing removed
  • Milestone changed from Future Release to 5.9

Given the CSS in GB1602 only affects the legacy widget preview iframe and that nobody was able to find any issues with it, I think this is good to go for 5.9 as a fix for the problem.

While the rules look like a sledgehammer the fact they only apply to the iframes in the widgets editor makes them less impactful than introducing logic in how wp_footer and enqueueing work.

The rules to hide any extra content are really overwhelming, but I guess that's what you have to do to cover all possibilities.

hellofromtonya commented on PR #1602:


3 years ago
#47

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 commented on PR #1602:


3 years ago
#48

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.

#49 @hellofromTonya
3 years ago

  • Keywords commit removed

As this needs to be fixed upstream in Gutenberg, removing the commit keyword.

@noisysocks Is the fix for this issue planned for 5.9?

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

hellofromtonya commented on PR #1602:


3 years ago
#53

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.

#54 @costdev
3 years ago

  • Resolution set to reported-upstream
  • Status changed from new to closed

The GB PR has now been merged and brought over to Core via a @wordpress package update in 52277.

#55 @okvee
3 years ago

I'm currently testing with WordPress 5.9-RC2-52567 but the wp_footer still appears on admin widgets management & customizer widgets management pages.
What I have to do?

#56 @domainsupport
22 months ago

Seeing as the issue has not yet been fixed in Gutenberg, what is the recommended method for preventing the issue for plugin developers? I have come up with this ... ?

<?php
add_action( 'wp_footer', 'wp_footer_proof_of_concept' );

function wp_footer_proof_of_concept() {

if (!(defined('REST_REQUEST') && REST_REQUEST)) {

echo "Why is this being output on the Widgets screen?";

}

}

Please confirm.

Oliver

Note: See TracTickets for help on using tickets.