Update image dimensions inside of a timeout to ensure image replacement is rendered on screen before measuring #48
Conversation
The dimension calculations were calculating image dimensions before the image is rendered on the page, which gives incorrect element dimensions, leading to a misproportioned image. This PR attempts to fix that issue by recalculating dimensions on a Mutation event, when the src is first replaced, rather than immediately on setup, to better calculate the space the <img> element takes up in the document. (I'm torn between this approach and just doing it in a timeout... I think the timeout would be better performance, but at the risk of not being as accurate.)
goldenapples
force-pushed
the
mutation-observer
branch
from
9f9bd81
to
ca13105
Compare
goldenapples
changed the title
assets/gaussholder.js
Outdated
| render(canvas, element.dataset.gaussholder.split(','), final, function () { | ||
| // Load in as our background image | ||
| element.style.backgroundImage = 'url("' + canvas.toDataURL() + '")'; | ||
| element.style.backgroundRepeat = 'no-repeat'; | ||
| setTimeout( function() { calculateDimensionStyle( element ); }, 200 ); |
There was a problem hiding this comment.
Could we use the image element load event perhaps? This still seems like it could be brittle
There was a problem hiding this comment.
This is already happening in the image's onload event handler. The timeout here is just trying to work around some issues where the dimensions weren't calculated properly, because apparently the "onload" event fires when the file load is complete, but before the DOM rendering is necessarily complete.
I could probably remove the 200ms timeout and just leave it in a timeout or a requestAnimationFrame to make sure the image renders on screen before measuring it. You're right, this does feel brittle - I'll clean it up and see what I can do to make the callback happen as soon as possible.
There was a problem hiding this comment.
Ah ok, so if this is really just adding a small extra buffer right now before the blurred image is getting swapped out then I think it's ok. I'll test it out on local and see what happens to see if I have any other ideas.
I'll run I'd also love to have someone with a front-end focus (pinging @joeleenk to get it on your radar possibly?) to follow this up with some tweaks to the timing of the whole image replacement and blur process to make this feel as slick as we can get it - I think the lazyloading here is a really nice touch to offer as a platform feature but it feels a little slow and uneven right now. |
Brings this file more in line with our coding standards. Note that I had to turn off the "no-var" rule to avoid conflicts with grunt uglify, which for some reason doesn't support let and consts yet.
|
Just a note here - there are some updates along with WP 5.5 that might mean we don't need to do this. I actually fully removed the image dimensions stuff in #55 - WP 5.5 adds width and height attributes to all images now and in smart media we prevent Tachyon from removing the image dimensions as well. Lastly in #55 this moves towards relying on the browser's |
|
@roborourke @goldenapples I think this same issue is happening in Orgvue site: https://github.com/humanmade/concentra/issues/950. I've disabled it in production but kept the Gaussholder enabled in development. The problem seems to be appearing exclusively in Safari, not Chrome or Firefox so you should see the images in Chrome but not in Safari because their heights have been set to 0.7px. When trying to debug, this problem doesn't appear as it seems to be a timing issue. |
|
There's a new fix in here: #57 that avoids timeouts and also use the IntersectionObserver API to detect images in the viewport. It includes several other improvements like webpack bundling. I think we can close this one. |
The dimension calculations were calculating image dimensions before the image is rendered on the page, which gives incorrect element dimensions, leading to a misproportioned image.
This PR attempts to fix that issue by setting a 200ms timeout after setting an image src, in order to better calculate the space the![]()
element takes up in the document.