Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#46540 new defect (bug)

Admin bar CSS shouldn't be added in the customizer

Reported by: nabil_kadimi's profile nabil_kadimi Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.5
Component: Customize Keywords: has-patch needs-testing 2nd-opinion
Focuses: administration Cc:

Description

I was playing with the Customizer selective refresh and noticed that the admin bar class WP_Admin_Bar doesn't check if the user is in the Customizer, thus it may add the 46px top margin and whatnots even if there is no admin bar.

So to reproduce the bug, create a partial where the render_callback has wp_head() in it.


Attachments (1)

46540.diff (661 bytes) - added by dlh 5 years ago.

Download all attachments as: .zip

Change History (12)

#1 @nabil_kadimi
5 years ago

I'm using this workaround:


add_action( 'customize_register', function() {
        show_admin_bar( false );
} );

Version 1, edited 5 years ago by nabil_kadimi (previous) (next) (diff)

@dlh
5 years ago

#2 follow-up: @dlh
5 years ago

  • Focuses ui removed
  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 5.2
  • Version changed from 5.1 to 4.5

Thanks for your report, @nabil_kadimi!

This issue can be fixed, at least superficially, by including a customize_messenger_channel parameter in the selective-refresh request, which activates essentially the same logic in \WP_Customize_Manager::setup_theme() as in the workaround: https://github.com/WordPress/wordpress-develop/blob/6d2f78d9bacf931fb9d4ba031e135c4eb5b17713/src/wp-includes/class-wp-customize-manager.php#L559-L562

However, I confess that I'm not sure what the messenger_channel is for or why it might have been excluded from the partial-refresh requests so far.

@westonruter, are you able to comment on the merits of 46540.diff?

Also, @nabil_kadimi , what's the use case for a partial that includes wp_head()?

I'm very gingerly adding this to the 5.2 milestone just in case the fix really is only to add the one line and could be easily shepherded into a release.

#3 @nabil_kadimi
5 years ago

Thanks for reviwing this @dlh

Also, @nabil_kadimi , what's the use case for a partial that includes wp_head()?

There's a theme I'm working one whose stylesheet URI may change depending on the user customizations, I was trying to find a way to update the stylesheet URI without doing a full refresh, the smallest "partial" I could target was the header.

#4 in reply to: ↑ 2 ; follow-up: @westonruter
5 years ago

Replying to dlh:

@westonruter, are you able to comment on the merits of 46540.diff?

This seems sensible to me.

But in general I would think that associating an entire partial with wp_head() will cause other problems as well.

@nabil_kadimi Can't you register a partial with the selector for the link element that has the stylesheet?

#5 in reply to: ↑ 4 ; follow-up: @nabil_kadimi
5 years ago

@nabil_kadimi Can't you register a partial with the selector for the link element that has the stylesheet?

No, unless I hardcode the style HTML code instead of using wp_enqueue_style.

But in general I would think that associating an entire partial with wp_head() will cause other problems as well.

Yes I believe. I switched to 'postMessage' => 'refresh'. But I'm thinking that the customizer shouldn't make any difference between HTML in body or head.

#6 in reply to: ↑ 5 @westonruter
5 years ago

Replying to nabil_kadimi:

But I'm thinking that the customizer shouldn't make any difference between HTML in body or head.

I think it would still be problematic because things in the head are generally special in controlling the page.

I think actually actually this isn't a good fit for selective refresh. Since everything is in an external stylesheet, the only thing needed is to update the URL of the stylesheet, no? So then what if you used postMessage and then added some JS to the preview like this:

wp.customize( 'my-custom-stylesheet', ( setting ) => {
    setting.bind( () => {
        const link = document.querySelector( 'link#my-stylesheet' );
        const url = new URL( link.href );
        url.searchParams.set( 'cache-bust', Math.random() );
        link.href = url.href;
    } );
} );

So this would just update the URL to the stylesheet whenever the relevant setting changes.

#7 @nabil_kadimi
5 years ago

Since everything is in an external stylesheet, the only thing needed is to update the URL of the stylesheet, no?

I appreciate your help @westonruter, actually, the CSS is kinda dynamic (Bootstrap with customizable variables), before knowing the CSS, I need to know the variables, then create the CSS that uses them and give the file a name that contains these variables md5 (for unicity).

I tried passing the variables using code similar to yours with AJAX in it, unhappily, the AJAX response was always -1 and I was not able to get the new CSS link using that method, from there I moved to selective_refresh where I had that error with the the admin bar CSS, then finally I reverted to refresh.

Thanks again for your help, I don't want to waste your time on this, 'postMessage' => 'refresh' works for me.

Last edited 5 years ago by nabil_kadimi (previous) (diff)

#8 @westonruter
5 years ago

Oh yes, of course. Another reason why it doesn't work is that the changeset UUID is not included in the stylesheet URL, so that is why the Customizer state is not applied to the requested stylesheet.

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


5 years ago

#10 @audrasjb
5 years ago

  • Milestone changed from 5.2 to Future Release

As per today's bug scrub, we are going to move this one to Future Release since beta 3 and RC are approaching.

#11 @celloexpressions
3 years ago

  • Keywords 2nd-opinion added

As noted previously, it's generally not a good idea to set the entire wp_head() as a partial. I haven't seen any use cases noted that would require that approach instead of more-targeted partials.

It sounds like 46540.diff may fix this issue, but it's not clear whether that's a necessary change.

Note: See TracTickets for help on using tickets.