WordPress.org

Make WordPress Core

Opened 7 weeks ago

Last modified 12 days ago

#53801 new defect (bug)

Block-based Widgets Screen does action wp_footer after each Widget

Reported by: MadtownLems Owned by:
Milestone: 5.8.2 Priority: normal
Severity: major Version: 5.8
Component: Widgets Keywords: has-patch needs-testing
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 7 weeks ago.
widget-1.png (13.3 KB) - added by ravipatel 7 weeks ago.
widget-2.png (34.5 KB) - added by ravipatel 7 weeks ago.
53801_css01.diff (740 bytes) - added by costdev 3 weeks ago.
53801_css02.diff (1.2 KB) - added by costdev 2 weeks ago.
CSS #02: Hide root-level text nodes, non-widget elements and pseudo-elements.
53801_css03.diff (1.2 KB) - added by costdev 10 days ago.
Remove width. Add !important to transform.

Download all attachments as: .zip

Change History (43)

#1 @knutsp
7 weeks ago

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

#2 @sabernhardt
7 weeks ago

  • Milestone changed from Awaiting Review to 5.8.1

@ravipatel
7 weeks ago

@ravipatel
7 weeks ago

#3 @MadtownLems
7 weeks 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
7 weeks 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.


4 weeks ago

  • 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
4 weeks 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
4 weeks 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
4 weeks 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, right?

Last edited 4 weeks ago by costdev (previous) (diff)

#9 @prbot
3 weeks ago

draganescu commented on PR #1602:

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

#10 @prbot
3 weeks ago

costdev commented on PR #1602:

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 weeks 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?

#12 @prbot
3 weeks ago

draganescu commented on PR #1602:

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.

#13 @prbot
3 weeks ago

Clorith commented on PR #1602:

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.

#14 @prbot
3 weeks ago

costdev commented on PR #1602:

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 weeks 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 weeks ago by costdev (previous) (diff)

#16 @eatingrules
3 weeks 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.

#17 @prbot
3 weeks ago

draganescu commented on PR #1602:

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

#18 @prbot
3 weeks ago

costdev commented on PR #1602:

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.

#19 @prbot
3 weeks ago

costdev commented on PR #1602:

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 weeks 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 weeks 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 weeks ago by zieladam (previous) (diff)

#22 @eatingrules
3 weeks 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 weeks 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 weeks ago by costdev (previous) (diff)

@costdev
3 weeks ago

#24 @costdev
3 weeks ago

  • Keywords needs-testing added

#25 @zieladam
3 weeks 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 weeks ago by zieladam (previous) (diff)

#26 @desrosj
2 weeks 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
2 weeks 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
2 weeks ago

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

#28 follow-up: @costdev
2 weeks 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 2 weeks ago by costdev (previous) (diff)

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


2 weeks ago

#30 follow-up: @Hareesh Pillai
2 weeks 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.


2 weeks ago

#32 in reply to: ↑ 30 @costdev
2 weeks 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 2 weeks ago by costdev (previous) (diff)

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


2 weeks ago

#34 @ryokuhi
2 weeks 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
13 days 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
13 days 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
12 days ago

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

@costdev
10 days ago

Remove width. Add !important to transform.

Note: See TracTickets for help on using tickets.