Fix gray border on image for alt with no src bug on lightbox #12379
Conversation
cathyxz
changed the title
src/image-viewer.js
Outdated
| } else { | ||
| // Most browsers show a gray border for images with alt but no src | ||
| // If we did not set the src, remove the alt attribute | ||
| this.image_.removeAttribute('alt'); |
There was a problem hiding this comment.
How does it get set initially? Is it only through updateSrc_?
There was a problem hiding this comment.
It gets set initially in the init function here (this code is in the init function) IFF the image has already loaded. Then in updateSrc it gets set again. The basic idea is that you set the already loaded image src in init, and then you upgrade it to the best fit version from srcSet in updateSrc. updateSrc is also called after every measure(), since measure updates the height and width of the image. But if the image wasn't loaded, then you shouldn't be setting the src here.
But this behavior definitely raises the question of a perf bug... all of these images should have already been loaded by the time you click on them to load the lightbox. Although there are alternate ways of triggering the lightbox, which makes the code here reasonable, there's almost definitely something wrong with the way we're detecting whether an image has loaded here... =.="
There was a problem hiding this comment.
Yeah, actually it's because we are not passing in the sourceImage parameter to init. This was an oversight in my original port of the implementation. Testing in progress, fix incoming.
|
Retested this on all devices--the initial load speed for the first 3 images in carousel is now a lot better. The grey box problem is solved by setting the image to 0 width and height. Using this implementation because this also solves the problem of images with a loaded src being initialized in the wrong position, since at |
|
Excellent, nice job assessing and improving the performance! Double LGTM |
|
Good stuff! |
…ect#12379) * Fix safari gray border bug on lightbox * Add special case for iOS * Actually this bug applies to most mobile browsers and desktop safari * Set image source properly, initialize height width to zero
…ect#12379) * Fix safari gray border bug on lightbox * Add special case for iOS * Actually this bug applies to most mobile browsers and desktop safari * Set image source properly, initialize height width to zero
Fixes #12366.
Problem:
Multiple browsers gives images with
altbut notsrcattribute a gray border. OnImageViewerinitialization, we set all the aria attributes includingalt, but not thesrcattribute unless the image has loaded. Therefore when we open the lightbox for the first time, before the image loads, safari shows an ugly gray border. This happens every time we scroll to a new image for the first time. Applies to: mobile safari, mobile chrome, mobile firefox, desktop safari.Proposed solution:
Special case safari to not set the
altattribute until after we setsrc.There are multiple hack solutions to this problem, including setting opacity to 0 until we set src, setting a tiny transparent src, but nothing elegant comes to mind. If an immediate better solution comes to mind, I'm very open to suggestions. I think we could also revisit this after reviewing Lightbox for performance because depending on how we refactor the order of doing things, this bug might no longer be applicable by then.
Testing: