fix(firefox): handle data uris #209
Conversation
There was a problem hiding this comment.
Thanks for the PR! I know we don't have a lot of stuff to help you get this set up on new projects, so let me know if you want any guidance there. I have integrated it with GitHub and Travis on a project and would be happy to share some of our scripts if you would find that helpful. I'd like to make it a lot easier to get set up at some point too (#143).
| return new Promise((resolve, reject) => { | ||
| // No need to wait for the image to be loaded. | ||
| if (url.indexOf('data:image') === 0) { |
There was a problem hiding this comment.
I think we might want to be a little more specific with this search. In my experience this was working alright with JPEG data URIs and was actually needed in order for those to be fully rendered by the browser (although I wonder if we should switch to something like createImageBitmap instead). I think this might only be a problem when the data URI is an SVG.
I also wonder if this should be in the promise or if we should add a filter before this function call. Thoughts?
In the project you were trying to add this to, did you encounter this with an SVG data URI or something else?
There was a problem hiding this comment.
I have been digging a bit more in the issue. It seems to be a char encoding issue. I haven't went down the rabbit hole yet. Are you saying that even with not network latency, it's preferable to load the image first?
There was a problem hiding this comment.
Ah interesting.
Are you saying that even with not network latency, it's preferable to load the image first?
Yes, because image rendering is non-instant and non-blocking, so screenshots can actually be taken in the middle of image rendering, which makes it so you end up with a lot of visual diffs for components that use images because the screenshots are taken at different moments while the image was rendering. Here's some more info: #162 (comment)
| @@ -0,0 +1,13 @@ | |||
| import { waitForImageToLoad } from '../waitForImagesToRender'; | |||
|
|
|||
| const image = "data:image/svg+xml;utf8,<svg%20viewBox='0%200%2010%2011'%20xmlns='http://www.w3.org/2000/svg'><g%20stroke='%23bfc7d8'%20stroke-width='1.5'%20fill='none'%20fill-rule='evenodd'%20stroke-linecap='round'><path%20d='M5%201.5v8M9%205.5H1'/></g></svg"; | |||
There was a problem hiding this comment.
I think you are missing a > on the end here.
|
I have found a better solution closer to the root of the issue. I have noticed that Firefox escape the var url1 = window.getComputedStyle(document.querySelector(X)).getPropertyValue('background-image')
// Firefox: url1 = url("data:image/svg+xml;utf8,<svg%20viewBox=\'0%200%2010%209\'%20xmlns=\'http://www.w3.org/2000/svg\'><path%20d=\'M1%204.88l2.378%202.435L9.046%201.6\'%20stroke-width=\'1.6\'%20stroke=\'%23FFF\'%20fill=\'none\'%20fill-rule=\'evenodd\'%20stroke-linecap=\'round\'%20stroke-linejoin=\'round\'/></svg>")
// Chrome: url1 = url("data:image/svg+xml;utf8,<svg viewBox='0 0 10 9' xmlns='http://www.w3.org/2000/svg'><path d='M1 4.88l2.378 2.435L9.046 1.6' stroke-width='1.6' stroke='%23FFF' fill='none' fill-rule='evenodd' stroke-linecap='round' stroke-linejoin='round'/></svg>") |
Thanks for asking. I'm gonna try to integrate it with Travis CI, however, I'm planning on hijacking his original purpose to only takes the screenshots with Happo and to let Argos-CI handle the diffs and review process. Finally, I feel like it would be really interesting to build a selenium grid target. We have been using vrtest on Material-UI to take the screenshots in a docker hosting a selenium grid, chrome & firefox, that works pretty well. |
|
Now I have a reproduction example for that issue: algolia/react-instantsearch#130. I have managed to make the 82 cases passed with the fix of the PR. |
| @@ -5,7 +5,9 @@ export default function findBackgroundImageUrls(string) { | |||
| let match; | |||
| // eslint-disable-next-line no-cond-assign | |||
| while (match = URL_PATTERN.exec(string)) { | |||
| result.push(match[1]); | |||
| // We need to unescape the simple quote. | |||
There was a problem hiding this comment.
This comment could be expanded a little to better explain why it's here. How about
Inlined images of the form
data:imagecan contain embedded svg. To avoid failing to load this image, we need to unescape escaped single quotes.
I'm hesitant to open this up to be customized because I'm not sure how useful it would be for most people. However, we have been discussing changing how we approach uploading in #178. Do you think something like that would fit your use-case? |
|
Thanks guys :).
@lencioni I can understand, I ended up writing a normalisation script. Without looking too much in details, I don't think that #178 would answer my issue. |
|
This was released in version |
Closes #207.
Come into that issue trying to add happo to the storybook of react-instantsearch.