Make WordPress Core

Opened 9 years ago

Closed 8 weeks ago

Last modified 8 weeks ago

#31352 closed defect (bug) (fixed)

Media icons are not retina friendly

Reported by: iseulde's profile iseulde Owned by: joedolson's profile joedolson
Milestone: 6.5 Priority: normal
Severity: normal Version: 3.9
Component: Media Keywords: has-patch has-unit-tests needs-dev-note
Focuses: ui, administration Cc:

Description

See screenshot and [27726].

Attachments (14)

Screen Shot 2015-02-16 at 20.20.08.png (9.9 KB) - added by iseulde 9 years ago.
dashicons-media-2x.zip (16.3 KB) - added by melchoyce 9 years ago.
dashicon-media-2x-optimized.zip (3.6 KB) - added by joemcgill 9 years ago.
Optimized dashicon .png files.
31352.diff (4.0 KB) - added by joemcgill 9 years ago.
dashicons-media-svgs.zip (12.7 KB) - added by melchoyce 9 years ago.
media-files-40px-optimized.zip (7.9 KB) - added by joemcgill 9 years ago.
New media files optimized
31352.2.diff (6.1 KB) - added by joemcgill 9 years ago.
31352.3.diff (9.7 KB) - added by joemcgill 9 years ago.
31352.4.diff (7.6 KB) - added by joemcgill 9 years ago.
Replaces bad diff in 31352.3.diff
31352.5.diff (8.7 KB) - added by sabernhardt 2 months ago.
Screenshot 2024-02-11 at 9.20.50 AM.png (567.5 KB) - added by huzaifaalmesbah 2 months ago.
after apply 31352.5.diff looks good.
31352.120x160.diff (8.4 KB) - added by sabernhardt 2 months ago.
edits numbers for viewBox of 0 0 120 160, adjusts some points in audio graphic
31352.pdf-icon.png (14.9 KB) - added by SergeyBiryukov 2 months ago.
31352.follow.diff (4.5 KB) - added by joedolson 2 months ago.
Handle list view; update tests

Download all attachments as: .zip

Change History (73)

#1 follow-up: @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.2

@melchoyce - can we get your comments here, please?

#2 in reply to: ↑ 1 @melchoyce
9 years ago

Replying to wonderboymusic:

@melchoyce - can we get your comments here, please?

Sorry, what's the question? Double checked and yeah, seeing 1x images instead of 2x. I can re-attach the 2x ones here, but we should really try to figure out a way to use something vector here, either the dashicons font directly or svgs.

#3 @wonderboymusic
9 years ago

@melchoyce I will defer to you on the best path forward - I need to go to retina camp and remind myself how these work

#4 follow-up: @melchoyce
9 years ago

Do you remember why we couldn't use dashicons in the first place? I remember there was a good reason, but I don't actually remember what that was anymore. Do you know if it's still an issue?

#5 in reply to: ↑ 4 ; follow-up: @ocean90
9 years ago

Replying to melchoyce:

Do you remember why we couldn't use dashicons in the first place?

See #26650 and [27726]. The issue is that wp_get_attachment_image_src() can only return images.

#6 in reply to: ↑ 5 @melchoyce
9 years ago

Replying to ocean90:

Replying to melchoyce:

Do you remember why we couldn't use dashicons in the first place?

See #26650 and [27726]. The issue is that wp_get_attachment_image_src() can only return images.

Do you know if it considers svgs images?

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


9 years ago

@joemcgill
9 years ago

Optimized dashicon .png files.

@joemcgill
9 years ago

#8 @joemcgill
9 years ago

  • Focuses ui administration added
  • Keywords has-patch ui-feedback needs-testing dev-feedback added; needs-patch removed

31352.diff gets the ball rolling on this by adding @melchoyce's dashicon files (optimized in dashicon-media-2x-optimized.zip) to the /wp-includes/images/media/ directory and referencing them in srcset attributes in media templates by adding an option to views.js when an icon is present.

Concerns:

  • At the moment, the new icon files do not match the 1x versions already in use: https://cloudup.com/cAs9F7XWMAQ. We either need to update both or change the new 2x files to relate to the ones already in core. I would defer to Mel here.
  • Using srcset with x descriptors has pretty good, but not complete browser support: http://caniuse.com/#feat=srcset. If we wanted to provide support for all browsers, we would need to include a polyfill. Not sure we want to add a js dependency here, so I opted not to.
  • There's probably a more robust way to take the name of the 1x icon to create the 2x file name than what I've done in the inital patch, but it's a decent proof of concept.

Alternate solutions considered:

  • Simply replace the current icon files with the 2x versions and serve oversized images to those who don't need them. Not ideal, but less markup. I wouldn't recommend this, but it's an option.
  • Using SVGs. I couldn't find any prior art in the admin for using inline SVGs in the UI. There are several places where we use SVGs as background images via CSS, but not inline. Ultimately, this would be an ideal case for using SVGs but there have been concerns expressed with doing so.

#9 @melchoyce
9 years ago

Attached the icons as svgs. Let's use these.

This ticket was mentioned in Slack in #design-dashicons by melchoyce. View the logs.


9 years ago

#11 @joemcgill
9 years ago

There is a desire to update dashicons so that they could be used as inline SVGs instead as background images via CSS. Once the dashicon SVGs are updated (see previously linked Slack log). If we want to wait for an SVG solution, those updates would need to happen before this issue could be resolved.

Otherwise, we can add 2x png files for now and look at updating them to SVG once the dashicon improvements are merged.

This ticket was mentioned in Slack in #design-dashicons by ryelle. View the logs.


9 years ago

@joemcgill
9 years ago

New media files optimized

@joemcgill
9 years ago

#14 @joemcgill
9 years ago

I've compressed the dashicon files from @melchoyce and colored them to match what was already there (hex: 888888) in media-files-40px-optimized.zip. These files will need to overwrite (and add to) the existing files in src/wp-includes/images/media/ as shown in 31352.2.diff.

By the way, If there's a better way to deal with media files in diffs, let me know.

@joemcgill
9 years ago

@joemcgill
9 years ago

Replaces bad diff in 31352.3.diff

#15 @joemcgill
9 years ago

I found a view that I missed when updating these media icons. 31352.4.diff adds support in the media list view (ignore 31352.3.diff). Just a reminder, you will still need to manually add the images from media-files-40px-optimized.zip.

This still isn't a complete patch, because we need to account for times where the icon directory has been filtered and the 2x versions do not exist. Not sure the best way to check if the there is a way to see if a filter has been set on the icon directory within the media/views.js, so it may be better to extend wp_get_attachment_image_src() to return a valid src and src-2x. On the other hand, if we were to just use SVGs instead of PNGs here, we could avoid adding a srcset at all and simplify the whole process.

Would be interested in feedback on the preferred way forward.

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


9 years ago

#17 @helen
9 years ago

  • Milestone changed from 4.2 to Future Release

Not yet on final approach here, too late for 4.2.

#18 @joemcgill
8 years ago

  • Milestone changed from Future Release to 4.5
  • Owner set to joemcgill
  • Status changed from new to accepted

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


8 years ago

#20 @chriscct7
8 years ago

  • Milestone changed from 4.5 to Future Release

Punting per discussion in Slack.

#21 @melchoyce
8 years ago

Would really love to see this get revisited soon.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


7 years ago

#23 @melchoyce
7 years ago

@liljimmi, @empireoflight — This would be a good candidate for using SVGs.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


7 years ago

#25 @boemedia
6 years ago

  • Keywords has-ui-feedback needs-design added; ui-feedback removed

Totally agree this would be better in svg. Maybe @cathibosco or someone else from the dashicons team can take a peak? I'll ping them on Slack.

This ticket was mentioned in Slack in #design-dashicons by boemedia. View the logs.


6 years ago

#27 @boemedia
6 years ago

I mentioned this in the #design-dashicons channel on Slack yesterday, and according to @melchoyce , this now needs a developer to make a patch using the Dashicons svgs.

#28 @karmatosed
5 years ago

  • Keywords has-ui-feedback needs-design removed

As this needs a developer to make a patch, for now removing the keyword as think that's the right step. I can add it back in if this does still need design.

#29 @joedolson
6 months ago

  • Milestone changed from Future Release to 6.5
  • Owner changed from joemcgill to joedolson

@sabernhardt
2 months ago

#30 @sabernhardt
2 months ago

I took the collection from dashicons-media-svgs.zip, used Adobe Illustrator to move the contents to the upper left corner of the image, exported at 500 by 500, resized the viewBox to 300 by 400, and ran the graphics through optimization.

Note:

  • The fill color is #888888.
  • The path numbers could be reduced for a smaller viewBox, but I would want to make sure the rest of the patch works well first.
  • I updated the default graphic's extension in wp-playlist.js.

#31 @joedolson
2 months ago

This seems to work well to me. Do you want to adjust the path numbers, @sabernhardt? Up to you.

@sabernhardt
2 months ago

edits numbers for viewBox of 0 0 120 160, adjusts some points in audio graphic

#32 @sabernhardt
2 months ago

I divided the values by 2.5 to get a viewBox of 0 0 120 160. When I tried reducing again to get 48 by 64, the images seemed a little blurry at 500% zoom (plus the audio and video SVG file sizes were larger because of the decimals). The 0 0 120 160 images still look crisp and reduce the file sizes from 31352.5.diff.

I also adjusted a few points in the audio SVG to make the first eighth note (quaver) slightly more rounded.

#33 @bosskhj
2 months ago

Test Report

Patch tested: 31352.120x160.diff

Environment

  • WordPress: 6.5-alpha-56966-src
  • PHP: 8.2.15
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.15)
  • Browser: Chrome 121.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Four 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.1.0

Actual Results

  1. ✅ Issue resolved with patch.

Screenshot after applying the patch.

https://i.ibb.co/sK06gnf/Screenshot-2024-02-13-at-1-09-15-AM.png

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


2 months ago

#35 @joedolson
2 months ago

  • Keywords commit added; needs-testing dev-feedback removed

This looks good and has a second test, so let's get it moving.

#36 @joedolson
2 months ago

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

In 57638:

Media: Replace media icon images with SVG.

Replace the .png files in use for media library type icons with .svg versions. Improves sharpness, especially on high resolution screens and devices.

Props iseulde, melchoyce, joemcgill, sabernhardt, huzaifaalmesbah, wonderboymusic, ocean90, karmatosed, boemedia, bosskhj, joedolson.
Fixes #31352.

#37 @SergeyBiryukov
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for the commit! I noticed that wp_mime_type_icon() tests now fail when run separately:

phpunit --filter test_wp_mime_type_icon

...

There were 2 failures:
1) Tests_Post_Attachments::test_wp_mime_type_icon
Failed asserting that 'http://example.org/wp-includes/images/media/default.svg' contains "images/media/default.png".

S:\home\wordpress.test\develop\tests\phpunit\tests\post\attachments.php:511

2) Tests_Post_Attachments::test_wp_mime_type_icon_video
Failed asserting that 'http://example.org/wp-includes/images/media/video.svg' contains "images/media/video.png".

S:\home\wordpress.test\develop\tests\phpunit\tests\post\attachments.php:520

For some reason, they pass when running the whole test suite, so this needs further investigation.

Additionally, the text icon is no longer displayed for PDF files after [57638] (31352.pdf-icon.png is the before/after screenshot), so something else is missing here, perhaps there's a MIME type mismatch somewhere in the function.

Last edited 2 months ago by SergeyBiryukov (previous) (diff)

#38 @joedolson
2 months ago

The test failure sounds odd; I'll investigate that.

I checked my local dev, and I can see why I missed the PDF icon - my local renders the PDF previews, instead. Didn't cross-check.

#39 @joedolson
2 months ago

Found it. In the list view, the image is only displayed if it has a width and a height found by wp_getimagesize().

#40 @sabernhardt
2 months ago

  • Keywords commit removed

Should this be reverted?

Other icons do not print in List mode either. I had only checked Grid mode and the audio playlist (using Classic Editor/block).

Last edited 2 months ago by sabernhardt (previous) (diff)

@joedolson
2 months ago

Handle list view; update tests

#41 @joedolson
2 months ago

Updated patch attached. The tests are odd, though. In looking at the tests, I couldn't see any sound reason that the tests that failed should have passed when run in total; and I also found an additional test that I feel *should* have failed.

Somebody with better knowledge of the test suite architecture might need to look at those.

#42 @joedolson
2 months ago

@sabernhardt No, I don't see a need to revert. The tests are strange, but not blockers, I expect.

This ticket was mentioned in PR #6131 on WordPress/wordpress-develop by @joedolson.


2 months ago
#43

  • Keywords has-unit-tests added

#44 @joedolson
2 months ago

The updated tests fail running the test suite. I'm wondering if there's a cache in play somewhere, but I don't know where it might be.

#45 @joedolson
2 months ago

  • Keywords 2nd-opinion added

The problem is that both sets of files are still present, so both sets of files are added to the icon_files array. *Which* file is used depends on the order the files are iterated in. We still have to support the possibility of .png files, because these could be customized, but need to prioritize the SVG in core.

It's probably an OS or filesystem thing, but I didn't investigate that further once I determined the root problem.

In the PR, I added a function argument to prefer the svg extension, defaulted to svg. We could also default to png, but add the parameter to uses of this function. As I have it, it'll be a potential problem for anybody passing a custom icon directory that does not contain SVG files. There are a lot of plugins that filter this, so having this as a default is probably not a great plan. Core doesn't call the function frequently, so it's not a major change to just add a parameter where needed.

Requesting 2nd opinions.

This ticket was mentioned in PR #6138 on WordPress/wordpress-develop by @joedolson.


2 months ago
#46

This is an alternate path for 31352 that changes the arguments in core functions but not the default return for the function.

Trac ticket: https://core.trac.wordpress.org/ticket/31352

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


2 months ago

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


8 weeks ago

#49 @antpb
8 weeks ago

  • Keywords 2nd-opinion removed

I think the change to default .svg is safe enough given there is a path to updating if they need to define .png explicitly. The deprecated logic is good too. Risk seems low enough that the alternative approach is a good one.

@joedolson commented on PR #6131:


8 weeks ago
#50

Closing in favor of the alternate patch in PR https://github.com/WordPress/wordpress-develop/pull/6138

#51 @joedolson
8 weeks ago

  • Keywords commit added

#52 @joedolson
8 weeks ago

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

In 57687:

Media: Ensure wp_mine_type_icon() returns expected file type.

Add an argument to wp_mime_type_icon() to control the file type returned. Following [57638], there are two file formats in the media icons directory. Different systems would pull up different files by default dependent on the order loaded into the cached array, causing intermittent test failures and unpredictable behavior.

Function update allows core usages to always return the .svg while maintaining backwards compatibility for any extended usage that expects a .png. Follow up to [57638].

Also handles a missed case in media list view.

Props SergeyBiryukov, sabernhardt, joedolson, antpb.
Fixes #31352.

@joedolson commented on PR #6138:


8 weeks ago
#53

in r57687

#54 @sabernhardt
8 weeks ago

  • Keywords commit removed

I'm a little late to comment on the pull request.

  1. Update documentation with since note and "Optional":
    • * @since 6.5.0 Added `$preferred_ext` argument.
    • * @param string $preferred_ext Optional. File format to prefer in return. Default .png.
  2. I think some people might miss the period at the beginning of the parameter value. I tried adding a conditional statement after the if ( ! is_numeric( $mime ) ) {...} check (using all three conditions and ensuring lowercase is probably excessive but it could cover more mistakes):
    if ( ! empty( $preferred_ext ) && is_string( $preferred_ext ) && ! str_starts_with( $preferred_ext, '.' ) ) {
    	$preferred_ext = '.' . strtolower( $preferred_ext );
    }
    
  3. We probably should add more unit tests in post/attachments.php for the new parameter (test_wp_mime_type_icon_video_with_preferred_ext(), perhaps in a future release/ticket).

Also, directory searches found wp_mime_type_icon\( in 445 plugins and 27 themes.

Last edited 8 weeks ago by sabernhardt (previous) (diff)

#55 @joedolson
8 weeks ago

  • Keywords needs-dev-note added

#56 @joedolson
8 weeks ago

I did look at a lot of the directory results referencing this function, and didn't see anything of concern with this pattern. If you see something specific, though, please mention it.

I'm not super concerned with people missing the dot in the argument. It would just fail, and most likely somebody using the function would notice that? Can enhance it, though; not a major concern.

Re: the @since reference. I know I had that in a previous version; I got myself into some trouble by having multiple PRs, I think, and I failed to copy some changes between them. Sigh.

#57 @sabernhardt
8 weeks ago

The directory searches probably belonged in a separate comment; I did not check the plugins and themes for problems.

Maybe the mistake-correcting enhancement and extra unit tests (items 2 and 3) could be considered later on a separate ticket.

#58 @joedolson
8 weeks ago

The existing tests cover both the 'is an svg' and the 'no argument' cases, so I think it's OK for now; but obviously doesn't hurt to expand test coverage. But I think that can be a follow-up ticket; at least, I don't think we need to reopen this one for those.

#59 @SergeyBiryukov
8 weeks ago

In 57701:

Docs: Add a @since note for $preferred_ext parameter in wp_mime_type_icon().

Follow-up to [57687].

See #31352.

Note: See TracTickets for help on using tickets.