search

pinterest / widgets

JavaScript widgets, including the Pin It button.
Other
211 stars 88 forks source link

Provide for hoverbuttons on src-less img elements with data-pin-media #61

Closed mbaynton closed 8 years ago

mbaynton commented 8 years ago

Pinit.js currently shows hoverbuttons only when the image contains a nonempty src attribute. Made lots of sense in HTML4. In HTML5, to support fetching of appropriately sized image assets for multiple screen sizes, the specification allows for img elements that happily load pictures without an src attribute in sight, either by use of srcset or a parent picture element. Some generators (Wordpress 4.4 for example) are leaving an src attribute in place, thus not breaking pinit.js, but others (Drupal 8 with responsive images enabled for example) have removed src from their markup entirely and added the picturefill shim in order to support responsive image retrieval in legacy browsers as well. This is the only pattern that gives responsive image retrieval capabilities to both modern and legacy browsers, so it is likely to be seen more and more often, but since no src attribute is present it is not compatible with pinit.js.

This pull request, in action @ https://www.tangledupinfood.com/, is the least disruptive solution I came up with after some hours of head scratching. It would be nice to have pinit.js identify the "best" image from these sets for pinning, and thus work out-of-the-box on srcset+picturefill sites, but given the complexity of the picture element and experimental status of DOM interfaces when they are available at all, this level of support doesn't seem very feasible. However, if we require sites using the picturefill pattern to add data-pin-media to their pinnable responsive images, then only these two lines seem to need any changing. It at least gives them a path to having hoverbuttons.

kentbrew commented 8 years ago

Hi, terribly sorry for the trouble but would you mind remaking this PR on the new version of pinit_main.js? I think it's a good change and I'd like to merge it with your name on it.

mbaynton commented 8 years ago

Cool, resynchronized with HEAD. Looks like you've been busy since January.

Full disclosure, my original comment isn't quite accurate -- the WHATWG still considers src to be a required attribute, but modern browsers aren't as strict.

I gather from the commit log that you phase in code changes by serving new versions to a progressively increasing percentage of users? It'd be nice to know when a version of pinit_main.js with this change is guaranteed to be served. Thanks!

kentbrew commented 8 years ago

Thanks for the rewrite! Running this change by some folks with strong opinions about Web standards; should know more by tomorrow.

kentbrew commented 8 years ago

Done and in production now. Thanks again!

mbaynton commented 7 years ago

This is no longer working. Images without an src receive a pinterest hover button, but rather than the image getting pinned, I just get a solid color.

I checked and the latest version of Drupal 8 still ships with no src attribute on the img tag if you configure the site to use responsive images and define more than one breakpoint.

kentbrew commented 7 years ago

May I have an URL to test?

mbaynton commented 7 years ago

My apologies, I should have either closed or revised this. I investigated a bit further and found that the change was relative URLs used to work in data-pin-media, but no longer do. I just modified the Drupal 8 pinterest module to put absolute URLs and life was good again for me. So, there was a breaking change, but I have no vested interest in doing anything about it personally.