Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#42495 closed defect (bug) (fixed)

Customizer: Widget-embedded playlist: Incomplete preview

Reported by: bpayton's profile bpayton Owned by: westonruter's profile 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 7 years ago.
Incomplete playlist preview
Playlist preview after publish and refresh.png (234.7 KB) - added by bpayton 7 years ago.
Playlist preview after publish and refresh
42495.0.diff (4.8 KB) - added by bpayton 7 years ago.
42495.1.diff (2.9 KB) - added by bpayton 7 years ago.
Same updates but new patch to be SVN friendly
42495.2.diff (5.4 KB) - added by westonruter 7 years ago.
Add jsdoc and phpunit tests

Download all attachments as: .zip

Change History (30)

@bpayton
7 years ago

Incomplete playlist preview

@bpayton
7 years ago

Playlist preview after publish and refresh

#1 @westonruter
7 years 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
7 years ago

  • Milestone changed from 4.9.1 to 4.9.2

#4 @obenland
7 years ago

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

#5 @bpayton
7 years 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
7 years 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
7 years ago

#7 @bpayton
7 years 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.


7 years ago

#9 @westonruter
7 years 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
7 years ago

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

#11 @dd32
7 years 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.


7 years ago

#13 @melchoyce
7 years 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
7 years ago

  • Keywords needs-refresh added

#15 @afercia
7 years ago

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

@bpayton
7 years ago

Same updates but new patch to be SVN friendly

#16 @bpayton
7 years 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
7 years ago

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

#18 @bpayton
7 years 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.


7 years ago

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


7 years ago

@westonruter
7 years ago

Add jsdoc and phpunit tests

#21 @westonruter
7 years 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
7 years ago

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

#23 @westonruter
7 years ago

In 42617:

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

Amends [42613].
See #42495.

#24 @bpayton
7 years 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 7 years ago by bpayton (previous) (diff)

#25 @SergeyBiryukov
7 years 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.