WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 weeks ago

#42495 closed defect (bug) (fixed)

Customizer: Widget-embedded playlist: Incomplete preview

Reported by: bpayton Owned by: westonruter
Milestone: 4.9.3 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

When adding a playlist to a text widget via the playlist shortcode, the preview simply shows a white box where the playlist will be. After publishing and reloading the page, the preview renders a full preview of the playlist, but any changes to the text widget cause the playlist preview to revert to the white box.

Attachments (5)

Incomplete playlist preview.png (222.4 KB) - added by bpayton 3 months ago.
Incomplete playlist preview
Playlist preview after publish and refresh.png (234.7 KB) - added by bpayton 3 months ago.
Playlist preview after publish and refresh
42495.0.diff (4.8 KB) - added by bpayton 2 months ago.
42495.1.diff (2.9 KB) - added by bpayton 4 weeks ago.
Same updates but new patch to be SVN friendly
42495.2.diff (5.4 KB) - added by westonruter 3 weeks ago.
Add jsdoc and phpunit tests

Download all attachments as: .zip

Change History (30)

@bpayton
3 months ago

Incomplete playlist preview

@bpayton
3 months ago

Playlist preview after publish and refresh

#1 @westonruter
3 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.9.1

Good catch. We need to unconditionally enqueue the wp-playlist JS statically or lazy-load it when selective refresh happens when playlists are used.

For the static route, we'd need to introduce the enqueue_preview_scripts logic from the media widgets:

https://github.com/WordPress/wordpress-develop/blob/c994a2e/src/wp-includes/widgets/class-wp-widget-media.php#L109-L111
https://github.com/WordPress/wordpress-develop/blob/c994a2e/src/wp-includes/widgets/class-wp-widget-media-video.php#L159-L176

For the dynamic route, it would require more work. We'd need to sniff out whether a playlist is included in the rendered partial and then dynamically add the required JS and CSS. This is done in Jetpack for infinite scroll. I don't think we'd need to go the dynamic route, however.

#3 @johnbillion
3 months ago

  • Milestone changed from 4.9.1 to 4.9.2

#4 @obenland
2 months ago

@bpayton Are Westons comments helpful enough to get started on a fix for this?

#5 @bpayton
2 months ago

@obenland Yeah, I think they're a good place for me to start. I'm going to take the opportunity to understand more about how preview works.

#6 @bpayton
2 months ago

  • Keywords has-patch added; needs-patch removed

I created a PR for this because it has been a good way to collaborate with @westonruter in the past:
https://github.com/xwp/wordpress-develop/pull/303

I'm also attaching a patch with current progress.

@bpayton
2 months ago

#7 @bpayton
2 months ago

Note that this patch doesn't conditionally enqueue playlist scripts based on a filter like we do with wp_video_shortcode_library because AFAICT no similar filter currently exists for the playlist shortcode.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 weeks ago

#9 @westonruter
5 weeks ago

  • Keywords needs-testing added
  • Owner set to westonruter
  • Status changed from new to reviewing

@bpayton anything left here as far as you know?

#10 @bpayton
5 weeks ago

@westonruter, no, I believe it is ready for review and testing. Thanks!

#11 @dd32
5 weeks ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

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


4 weeks ago

#13 @melchoyce
4 weeks ago

Looks like the patch might need a refresh:

Hunk #1 FAILED at 3.
Hunk #2 succeeded at 166 (offset -4 lines).
1 out of 2 hunks FAILED -- saving rejects to file src/wp-includes/js/mediaelement/wp-playlist.js.rej
patching file src/wp-includes/widgets/class-wp-widget-text.php
Hunk #2 succeeded at 393 (offset -4 lines).
Hunk #3 succeeded at 414 (offset -4 lines).
patching file src/wp-includes/widgets/class-wp-widget-text.php
Hunk #1 succeeded at 403 (offset -4 lines).
Hunk #2 succeeded at 416 (offset -4 lines).

#14 @melchoyce
4 weeks ago

  • Keywords needs-refresh added

#15 @afercia
4 weeks ago

It applies cleanly for me :) @melchoyce maybe svn up then check again?

@bpayton
4 weeks ago

Same updates but new patch to be SVN friendly

#16 @bpayton
4 weeks ago

  • Keywords needs-refresh removed

@afercia let me know that patches can be generated in a better way for svn. Does this help?

#17 @afercia
4 weeks ago

@bpayton Git is OK to generate patches and as @jorbin mentioned on Slack, grunt patch takes care of hte format.

#18 @bpayton
4 weeks ago

I was glad to hear about grunt patch. It's helpful to avoid these kinds of issues. The patch was just an easy thing to update. Generating an SVN-friendly patch is about as easy as generating and unfriendly one., so I figured why not? Maybe a reviewer doesn't have grunt and JS deps set up and doesn't want to think about it right now.

Thanks again for your help today.

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


4 weeks ago

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


3 weeks ago

@westonruter
3 weeks ago

Add jsdoc and phpunit tests

#21 @westonruter
3 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 42613:

Customize: Ensure media playlists get initialized after selective refresh; expose new wp.playlist.initialize() API.

In particular allows audio and video playlists to be added to the Text widget and previewed.

Props bpayton, westonruter.
See #40854.
Fixes #42495.

#22 @westonruter
3 weeks ago

  • Keywords commit fixed-major added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#23 @westonruter
3 weeks ago

In 42617:

Widgets: Fix Text widget unit test which broke due to global scope not being cleaned.

Amends [42613].
See #42495.

#24 @bpayton
3 weeks ago

Thank you for adding unit tests, @westonruter. I wasn't sure whether this was something we should unit test, but if so, I didn't want to ask you to write them by default.

Thanks also for the good examples.

Last edited 3 weeks ago by bpayton (previous) (diff)

#25 @SergeyBiryukov
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 42622:

Customize: Ensure media playlists get initialized after selective refresh; expose new wp.playlist.initialize() API.

In particular allows audio and video playlists to be added to the Text widget and previewed.

Props bpayton, westonruter.
See #40854.
Merges [42613], [42617] to the 4.9 branch.
Fixes #42495.

Note: See TracTickets for help on using tickets.