MOBILE-4826 core-iframe: Add allow attribute to iframe #4507
Conversation
| </iframe> | ||
| } @else { | ||
| <iframe #iframe class="core-iframe" [attr.id]="id" [ngStyle]="{'width': iframeWidth, 'height': iframeHeight}" [src]="safeUrl" | ||
| [class.core-iframe-loading]="loading"> | ||
| [class.core-iframe-loading]="loading" [allow]="allow()"> |
There was a problem hiding this comment.
This is throwing an error, that's probably why behats are failing:
Angular has detected that the allow was applied as a binding to an <iframe> (used in the 'CoreIframeComponent' component template). For security reasons, the allow can be set on an <iframe> as a static attribute only.
To fix this, switch the allow binding to a static attribute in a template or in host bindings section. Find more at https://angular.dev/errors/NG0910
| </iframe> | ||
| } @else { | ||
| <iframe #iframe class="core-iframe" [attr.id]="id" [ngStyle]="{'width': iframeWidth, 'height': iframeHeight}" [src]="safeUrl" | ||
| [class.core-iframe-loading]="loading"> | ||
| [class.core-iframe-loading]="loading" [allow]="allow()"> |
There was a problem hiding this comment.
With your patch, you let the component to specify the allow string, but we aren't using it anywhere so all the iframes will behave exactly as they did.
If Poodll (the issue reporter) is a site plugin using core-iframe then that's ok because it means they will be able to supply the "allow" information when using core-iframe in their plugin template. But if Poodl is used somewhere else where we use core-iframe ourselves then this solution will not work for them. I don't know how Poodll works so I don't know their use cases, I just thought it was worth mentioning to be sure this solution is fixing the problem.
Even if Poodll use case is covered with this change, maybe we want to allow these permissions in some other cases (e.g. a SCORM?). We might want to discuss this with Juan.
| </iframe> | ||
| } @else { | ||
| <iframe #iframe class="core-iframe" [attr.id]="id" [ngStyle]="{'width': iframeWidth, 'height': iframeHeight}" [src]="safeUrl" | ||
| [class.core-iframe-loading]="loading"> | ||
| [class.core-iframe-loading]="loading" [allow]="allow()"> |
There was a problem hiding this comment.
Finally, the testing instructions of the issue are wrong. The tests done in the testing instructions will work without your patch, because your patch doesn't affect iframes created in LMS content, those iframes don't use the core-iframe component.
If we decide to keep this solution and don't use the "allow" input in our app, only allow using it for plugins, then you will need to create a dummy plugin to test that this works properly.
| @@ -70,6 +71,7 @@ export class CoreIframeComponent implements OnChanges, OnDestroy { | |||
| this.initIframeElement(); | |||
| } | |||
|
|
|||
| readonly allow = input<string>(); | |||
There was a problem hiding this comment.
I wouldn't put this input at the top of inputs, because IMO "src" is the most important input ("src" should probably be a required input, but we can change it later when we migrate this component to use input signals). So IMO "src" should be at the top of the inputs so it's seen first. This "allow" input is optional and will probably be used only in a few use cases (currently it isn't used at all in our app), so IMO it should be in a less important position.
alfonso-salces
force-pushed
the
MOBILE-4826
branch
from
cdfcc4c to
3f59a31
Compare
No description provided.