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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (13)
#2
@
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.
#3
@
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.
#5
@
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
@
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
@
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.
as per suggested solution, I have added patch.