Avoid iframes having to deal with asset proxy/lazy load
What does this MR do and why?
Part of <iframe> embeds in Markdown content (#282443).
Right now, iframes emitted by GitLab's IframeLinkFilter (i.e. entirely under our control, 100% safe!) still have to do a lot of backflipping to avoid getting wrecked by two other components:
-
AssetProxyFilter, which applies to images if the asset proxy is configured and enabled. Iframes look like images before being transformed byIframeLinkFilter, so they get "asset proxied" and we have to account for this possibility in the frontend. (This actually slipped our collective mind during the first roll-out of this FF'd feature, and was part of the first post-deploy fix of this to GitLab.com to get it actually working — see last paragraph of the "What does this MR do and why?" section of !215529 (merged).) -
ImageLazyLoadFilter, which applies to all images. We have to account for this too, while also accounting for the possibility that it may be disabled on a self-managed instance.
This ends up complicating the filter landscape a fair bit — there are 4 combinations of these two filters being enabled or disabled — and makes building additional functionality on the iframe loading (as proposed in #282443) quite difficult. This is all mostly a consequence of where in GfmPipeline the IframeLinkFilter was initially placed — it's handled after all these other filters, so it has no choice but to deal with the consequences of these elements having already been handled without any inkling they might be handled as iframes later.
This MR shifts the "iframe recognition" filter, IframeLinkFilter, up before these other two. This means successful iframe candidates are marked with js-render-iframe before the other filters run, and the other filters can then ignore such elements. As a result, the frontend doesn't have to deal with the possibility of the correct iframe src being one of three (!) different attributes.
We also have to bring AttributesFilter up — IframeLinkFilter depends on it having already run, so we need to preserve its relative ordering to IframeLinkFilter. (Opened Have Banzai filters declare their dependency or... (#591853) to automate the validation of this requirement.)
References
Past work:
- Add Banzai filter for detecting potential `ifra... (!204952 - merged) was the preliminary work for potential iframe recognition
- Allow iframe embeds in markdown from allowed sr... (!200864 - merged) carried the initial implementation
- Push iframe feature flag to frontend based on p... (!215529 - merged) fixed two bugs that stopped it from being usable on GitLab.com; since this MR the feature is configured and usable on GitLab.com, but you need to enable the FF for your group (we have it enabled in groupknowledge)
Present work: a series of 4 stacked MRs for %18.10 to bring this closer to GA:
- Pass iframe dimensions through if supplied (!224624 - merged)
- Avoid iframes having to deal with asset proxy/l... (!224625 - merged) — You are here
- Treat `<iframe>` as `<img>` (!224626 - merged)
- Transform `<iframe>` URLs according to a registry (!223870 - merged)
How to set up and validate locally
This is entirely a refactor, with zero observable behaviour difference. Here's how it should work:
- Check out the branch.
- Enable the
allow_iframes_in_markdownfeature on your GDK. - Enable embedding of
www.youtube.comfrom/admin/application_settings/general#js-iframe-settings, and ensureEnable embedded contentis turned on. - It may be wise to restart the GDK Rails instance here, for best chances of success.
- Using the plain-text editor, insert content like seen in the screenshot above to a wiki page, issue description, MR comment, etc.:
Here's my YouTube video: {width=800 height=600} - It should look like a big embedded YouTube video!
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.