[wp-trac] [WordPress Trac] #32417: Add new core media widget

[WordPress Trac]

#32417: Add new core media widget
-------------------------------------------------+-------------------------
 Reporter:  melchoyce                            |       Owner:  melchoyce
     Type:  feature request                      |      Status:  assigned
 Priority:  normal                               |   Milestone:  4.8
Component:  Widgets                              |     Version:  4.3
 Severity:  normal                               |  Resolution:
 Keywords:  needs-unit-tests has-patch needs-    |     Focuses:  ui,
  refresh                                        |  administration
-------------------------------------------------+-------------------------
Changes (by afercia):

 * keywords:  needs-unit-tests has-patch => needs-unit-tests has-patch
     needs-refresh


Comment:

 > "Open link in a new tab or window" doesn't make sense for something
 without a link.
 Yep, a bit pointless even when the media is not embeddable, since clicking
 on the link will just download the file and `target="_blank"` either gets
 ignored by browsers or opens a new tab/window just to download a file...

 Also, the usage of `target="_blank"` is under discussion and some work has
 already been done in that regard, admittedly mainly for the admin. Maybe
 opening new tabs/windows shouldn't be encouraged.
 https://core.trac.wordpress.org/query?keywords=~target-blank

 A few things noticed while checking the 2 latest patches:
 - Accessibility: the "Select Media" button is not even focusable, it's
 just an `a` element without a `href` attribute: `<a data-id="wp-media-
 widget-2" class="button select-media widefat">Select Media</a>`; it should
 be a real button element
 - Usability: there are no instructions that, when clicking on an image
 preview, users can change the media; see for example how the featured
 image meta box solves this with a hint "Click the image to edit or update"
 which is targeted by `aria-describedby` on the preview link. However, when
 the media preview is a video or audio, clicking the preview plays the
 audio or video so... the description (and the aria-describedby) should be
 used conditionally depending on the media type
 - Both a11y and usability: in the preview, not embeddable audio/video
 media get a link with a `href="#"` attribute that serves no purpose (?);
 from an accessibility perspective it should be avoided since it's a non-
 link; from an usability perspective, maybe better to just display the
 title and a message below, something like "this media type can't be
 embedded in the page, a link to the file will be used instead" ?
 - Usability: the media modal doesn't show the select to filter media by
 type, not sure if it can be added easily
 - Usability: in the modal, I can actually select a .pdf, .doc, .zip, or
 other files types and nothing happens: ideally, the media modal should
 filter the attachments in order to show only the allowed ones. Or, at
 least use a warning when trying to insert: "this media type is not
 allowed"
 - JavaScript: not translatable strings, e.g. 'Use as widget', 'Select
 Media'. Not sure about the usage of 'Change Media' as sort of fallback
 - JavaScript (minor): many properties name don't need quotes, only
 reserved names and names with hyphens need them, e.g. `'class'` or `'data-
 id'`
 - PHP: most of the uses of `extract()` were removed from core, no other
 widgets uses it. I'd recommend to avoid to introduce new ones. See #22400.
 - PHP: not sure why hidden fields should be inside a `<fieldset>` element,
 which is a semantic element used in this case for  elements that are not
 perceivable
 - PHP: escaping should be consistent, for example why `_e( 'Add an image,
 gallery, video, or audio to your sidebar.' )` and then `esc_html_e(
 'Select Media' )`?
 - PHP: not sure ternaries can be used inside translation functions:
 `esc_html_e( $attachment ? 'Change Media' : 'Select Media' );` while this
 may work in PHP, I'm not sure such strings can be correctly extracted by
 the i18n tools /cc @ocean90
 - Coding Standards: there's the need for some love, minor issues like
 spaces (see for example all the occurrences of `if(` and `('`. Also
 `(function ( $ ) {` should be `( function( $ ) {`
 - Minor: maybe this comment should be removed? `// @todo Variable is not
 used.`
 - Reminder: @since notations should be updated

--
Ticket URL: <https://core.trac.wordpress.org/ticket/32417#comment:97>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform