Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#41489 closed defect (bug) (fixed)

Customize Preview: do not show `cursor: not-allowed` on audio/video track titles within playlists

Reported by: celloexpressions's profile celloexpressions Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch commit
Focuses: ui Cc:

Description

In the customize preview, external links get a cursor: not-allowed. Core audio playlists link individual tracks within the playlist list to the individual audio file (.mp3, etc.). This is likely the case as a fallback for no-js users. Since the customize preview requires JS, clicking these links will not actually navigate the customize preview off of the site, and they will instead operate the playlist. Users should be encouraged to interact with playlists in the preview, as they may be adding them within content via Customize Posts or in a widget via a plugin such as Featured Audio.

Suggested solution: add something along the lines of if ( element.hasClass( 'wp-playlist-caption' ) { return: false; } somewhere in wp.customize.isLinkPreviewable() in wp-includes/js/customize-preview.js.

This needs an actual patch and more thorough testing. There may be other similar situations where we shouldn't use cursor: not-allowed that could be grouped into this ticket if desired.

Attachments (4)

41489.patch (850 bytes) - added by mitraval192 8 years ago.
as per suggested solution, I have added patch.
41489.2.patch (603 bytes) - added by scott.deluzio 7 years ago.
Prevent customize-unpreviewable class from being added to links in playlists
41489.3.diff (1.5 KB) - added by westonruter 7 years ago.
41489.3.patch (1.7 KB) - added by scott.deluzio 7 years ago.

Download all attachments as: .zip

Change History (13)

@mitraval192
8 years ago

as per suggested solution, I have added patch.

#1 @mitraval192
8 years ago

  • Keywords needs-testing added; needs-patch removed

#2 @westonruter
8 years ago

@mitraval192 I think the change would make more sense up after the “Ignore links with href="#", href="#id", or non-HTTP protocols (e.g. javascript: and mailto:)” conditional, or even made a part of that conditional.

@scott.deluzio
7 years ago

Prevent customize-unpreviewable class from being added to links in playlists

#3 @scott.deluzio
7 years ago

The 'if' line in the patch added by @mitraval192 should be this:

if ( element.hasClass( 'wp-playlist-caption' ) ) { 

not this (note missing ending parenthesis):

if ( element.hasClass( 'wp-playlist-caption' ) { 

That patch also seems to add errors to the customize preview:

Uncaught TypeError: element.hasClass is not a function
    at Function.prepareLinkPreview (customize-preview.js?ver=4.9-alpha-40870-src:349)
    at HTMLAnchorElement.<anonymous> (customize-preview.js?ver=4.9-alpha-40870-src:236)
    at Function.each (jquery.js?ver=1.12.4:2)
    at jQuery.fn.init.each (jquery.js?ver=1.12.4:2)
    at Function.addLinkPreviewing (customize-preview.js?ver=4.9-alpha-40870-src:235)
    at HTMLDocument.<anonymous> (customize-preview.js?ver=4.9-alpha-40870-src:670)
    at i (jquery.js?ver=1.12.4:2)
    at Object.fireWith [as resolveWith] (jquery.js?ver=1.12.4:2)
    at Function.ready (jquery.js?ver=1.12.4:2)
    at HTMLDocument.K (jquery.js?ver=1.12.4:2)
prepareLinkPreview @ customize-preview.js?ver=4.9-alpha-40870-src:349
(anonymous) @ customize-preview.js?ver=4.9-alpha-40870-src:236
each @ jquery.js?ver=1.12.4:2
each @ jquery.js?ver=1.12.4:2
addLinkPreviewing @ customize-preview.js?ver=4.9-alpha-40870-src:235
(anonymous) @ customize-preview.js?ver=4.9-alpha-40870-src:670
i @ jquery.js?ver=1.12.4:2
fireWith @ jquery.js?ver=1.12.4:2
ready @ jquery.js?ver=1.12.4:2
K @ jquery.js?ver=1.12.4:2

My proposed patch uses element.classList.contains instead of element.hasClass, which does not output the errors found in the previous patch.

It is inserted after the "Make sure links in preview use HTTPS if parent frame uses HTTPS." check. This ensures that HTTPS can be applied to the links if the parent frame uses HTTPS rather than returning before that check as was proposed by @westonruter.

It still returns before adding the customize-unpreviewable class.

#4 @scott.deluzio
7 years ago

  • Keywords has-patch added

@westonruter
7 years ago

#5 @westonruter
7 years ago

  • Keywords good-first-bug removed

@scott.deluzio The hasClass method is for a jQuery object. The element here is an HTMLAnchorElement, so that is why classList.contains() works instead. However, elsewhere in the function it's already doing $( element ) multiple times, so it makes sense to just create a $element and then hasClass can be used. See 41489.3.diff.

#6 @westonruter
7 years ago

One former reason to use jQuery's hasClass() instead of HTML5's classList.contains() is that the latter is only supported as of IE11. It's true that WordPress no longer supports versions older than that, but there's no reason why we have to intentionally break it here since jQuery addClass() is already being used in this same function.

#7 @scott.deluzio
7 years ago

@westonruter - agreed on using hasClass() instead of classList.contains(). I didn't realize it's support was limited to IE11+. And you're right, there's no need to intentionally break things for something especially if there's an easy way to maintain compatibility with older versions.

I added a new patch with the hasClass() method.

I kept the check for playlist links after the HTTPS check though, as links in the playlist could need to use that check.

The href="#" or href="#id" links should correctly use HTTP or HTTPS based on the protocol used on the page, so the HTTPS check is unnecessary. That check is OK to come before the HTTPS check.

The check for links in a playlist should come after this, but before the customize-unpreviewable class can be added.

#8 @westonruter
7 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to westonruter
  • Status changed from new to accepted

#9 @westonruter
7 years ago

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

In 41338:

Customize: Do not show cursor: not-allowed on audio/video track titles within playlists in preview.

Props scott.deluzio, mitraval192, westonruter.
See #31517.
Fixes #41489.

Note: See TracTickets for help on using tickets.