-
Notifications
You must be signed in to change notification settings - Fork 531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Translated content images aren't loading in Portuguese lessons #4667
Comments
@KolliAnitha are you able to consistently repro the image not loading issue? What are the steps to do so? |
Confirming that state does seem to have an issue. When in Portuguese:
It looks like something is incompatible with this SVG. This may be a problem as we don't currently have a filter for knowing which SVGs are incompatible, so I think I'll need to somehow add that to the asset pipeline. That being said, I'm not actually sure what to do about this yet as it's an issue in AndroidSVG (the library we use for parsing SVGs). |
What's interesting about the failure above is that it seems there is a transformation that includes an embedded base 64 PNG image. Per the SVG spec on transforms, this does not seem to be valid SVG (though it's supported by at least Chrome). I can't be sure since I'm having difficulty finding an SVG validator to check it, but it seems that we may inadvertently relying on hidden browser functionality in these images actually showing up. Now, one example image of this is: img_20220224_115706_jbqb2fgun6_height_300_width_400.svg with source visible in this Gist: https://gist.github.com/BenHenning/df53efba2a7eb61a09c4413610e55f7a?short_path=1215fb9. One thing to notice is that there are actually 5 embedded images within just that SVG, so this isn't a simple case of find-and-replace. These images need to be actually loaded and properly rendered in order for the final result to look correct. So I've had a chance to scan the entire corpus of images (for English + Portuguese) to get a comprehensive list of all SVGs that will fail to load in the app. 14 images in all fail, all of which are failing because of this embedding issue. Here's the list:
It seems that because of the nature of this issue, it's probably not feasible to fix it as it will require fairly extensive changes to at least the SVG loading library (and possibly Oppia Android if we want to make sure that the image is properly loaded through our image pipeline). Given that this may also have browser support issues (if it is indeed not per-spec SVG), perhaps it would be best to fix this in the lessons? |
So Sean and I realized that what's actually happening is AndroidSVG is having issues with xlink:href references to the base 64 PNGs, and it's incorrectly stripping them out causing a malformed SVG to be produced (hence why it can't continue parsing). That means these SVGs are valid, but AndroidSVG isn't handling them correctly. Nevertheless, we don't actually want embedded images in these SVGs since it makes it much harder to make them offline compatible. The fix is to replace the SVGs with PNGs in the meantime, and then prohibiting embedded links moving forward for all SVGs. |
Issue filed on Web for disallowing embedded links in images: oppia/oppia#16573 |
This issue should now be fixed (no code changes were needed in Android, just a redeployment of the binary). It won't be able to be verified until RC02 lands. |
This is due to an issue with the lesson importer where it doesn't properly catalog non-English images prior to downloading. This has resulted in ~2k images being omitted from the release.
The text was updated successfully, but these errors were encountered: