WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#19695 closed defect (bug) (invalid)

Links widget does not generate width or height attributes when showing an image

Reported by: mbijon Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

It's a best practice to include width and height attributes in image tags to prevent reflows when the browser renders the page. Both PageSpeed and YSlow include rules that measure this.

_walk_bookmarks() in /wp-includes/bookmark-template.php does support adding images to the output of the Links widget. However, it doesn't output the width or height attributes.

It is possible to size images with CSS instead of by tag attributes (and PageSpeed rules consider this acceptable). But it doesn't seem ideal to force users to set dimensions in CSS, when the Link edit page doesn't accept CSS directly. Plus, I think it's best to have all the meta info about something in one place, not to set an image in a WP link, and then the dimensions in a theme's CSS file.

Attachments (3)

19695.diff (1.2 KB) - added by mbijon 6 years ago.
Uses PHP's getimagesize() to detect image size and output the dimensions
19695.2.diff (1.3 KB) - added by mbijon 6 years ago.
No more PHP Notice when getimagesize() returns false
19695.3.diff (1.7 KB) - added by ericmann 6 years ago.
Cleaning up the code slightly to allow for situations where the function getimagesize() doesn't work (i.e. GD Image Library isn't installed with PHP).

Download all attachments as: .zip

Change History (12)

@mbijon
6 years ago

Uses PHP's getimagesize() to detect image size and output the dimensions

@mbijon
6 years ago

No more PHP Notice when getimagesize() returns false

#1 @mbijon
6 years ago

I don't like that my method relies on getimagesize() and URL-based images that Links has. getimagesize() is used elsewhere in WP, but usually with a path to the image instead of the URL.

Would it be better to enhance the edit page for Links, instead of my method in _walk_bookmarks() ? This URL method increases roundtrips when the images are hosted remotely.

#2 @mbijon
6 years ago

  • Keywords has-patch added

#3 @ericmann
6 years ago

I like the idea of it, actually. We really should be applying height/width attributes wherever we can, and this will definitely help with some content flow issues when building responsive layouts.

@ericmann
6 years ago

Cleaning up the code slightly to allow for situations where the function getimagesize() doesn't work (i.e. GD Image Library isn't installed with PHP).

#4 @mbijon
6 years ago

Thanks Eric. Per your Twitter reference to /wp-includes/media.php, that's what I started from. My main change is using the 4th value returned by getimagesize() instead of building a string.

I'm not sure about throwing an error if GD Lib isn't installed (in attachment:ticket:19695:19695.3.diff). I don't like doing that because GD isn't a requirement for installing WP, http://wordpress.org/about/requirements/. My method failed past that on purpose.

Note that PHP downgraded getimagesize() read errors to E_NOTICE in 5.2. I think that's the right approach, and that delivering content instead of an error msg is more important than having correctly-set image attributes.

#5 @ericmann
6 years ago

GD isn't a requirement, but this is the same way we handle it elsewhere. And, coming from having accidentally installed PHP at least once without GD, I can tell you that having notices like this in there is a lifesaver. It took me about 2 weeks to figure out why thumbnails weren't being auto-generated.

I mirrored my patch after the other patterns we follow when using getimagesize() in core for consistency. If one of the core leads wants to step in with an opinion, I'd be fine going either way, really.

#6 @dd32
6 years ago

I don't like this one bit, for the simple reason that getimagesize() is called on every request, quite potentially on a remote file, and due to the output not being scaled, it really provides little benefit for the performance hit it takes.

I'd prefer to see it support the width/height attributes if the item was added from the media library, otherwise just skip them. Of course, the Links stuff doesn't support the media library though, sounds like a great enhancement there for the day that links is moved from core to a plugin..

#7 @mbijon
6 years ago

Good point about this not being performant. What do you think about doing getimagesize() once on-save and then storing the values?

Adding media library now definitely isn't good. I just don't like leaving this unfixed up until links are finally moved to a plugin.

#8 @chriscct7
3 years ago

  • Keywords dev-feedback 2nd-opinion removed
  • Resolution set to invalid
  • Status changed from new to closed

Links are no longer shown in WordPress core since #21307, this ticket is now invalid.

#9 @DrewAPicture
3 years ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.