Skip to content
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

Resize working randomly with embedded H5P content #314

Open
anieminen opened this issue Nov 8, 2019 · 34 comments · May be fixed by #458
Open

Resize working randomly with embedded H5P content #314

anieminen opened this issue Nov 8, 2019 · 34 comments · May be fixed by #458

Comments

@anieminen
Copy link

Hi,

Embedded H5P content auto-resizing seems to work very randomly with MS browsers. Is this something that could be fixed in H5P Moodle plugin or PHP library?

Here is a couple of screenshots from my tests using Edge. When opening a Moodle Page activity with embedded H5P Interactive video in a small browser window, at first it doesn't resize the content:
Edge_embed_test1

But after refreshing the page a few times it does resize eventually:
Edge_embed_test2

@Urpokarhu1
Copy link

any progress on this @thomasmars

@Senio-123
Copy link

Any progress on this - I'm getting the same issue

@anrichp
Copy link

anrichp commented Sep 9, 2020

I am experiencing a similar issue in Moodle when inserting an H5P resource into a page. On most loads, the resource loads with the incorrect height. After reloading the page a few times the height of the resource loads correctly.

@dnutzz
Copy link

dnutzz commented Sep 17, 2020

We are experiencing the same problem on our instance when embedding H5P Content. Sometimes it does resize correctly but sometimes not. We could recreate the problem with several different Browsers (Chrome, Firefox and Safari).

H5P Version: 1.21.0
Moodle Version: 3.5.13+

For the case the video itself doesn't resize properly, we found that on the iframe object the style properties width and height are missing. When it is resizing correctly, it seems the resizer code adds the style attribute with width and height.
Furthermore we found a console error message regarding a postMessage function from h5p embed code:

Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided (‘https://www.youtube.com’) does not match the recipient window’s origin (‘https://ourexampleserver.com). embed.php.

In addition I have to mention, that we were not able to reproduce this on a local development system, only on a running server over web. We should maybe consider due to inconsistency of the bug, that we're maybe facing a timing problem of the function calls in JS.

@dnutzz
Copy link

dnutzz commented Oct 7, 2020

@thomasmars do you have a hint or a clue what could be the problem here?

@anieminen anieminen changed the title Resize working randomly with embedded H5P content on Edge & IE11 Resize working randomly with embedded H5P content Oct 7, 2020
@anieminen
Copy link
Author

We have also seen this happening with several different browsers lately.

@thomasmars
Copy link
Member

@dnutzz Yes, it seems likely that this is a timing issue. Do you have a public URL where this can be reproduced, the steps to reproduce it and the browsers and browser versions this can be seen in ? Or alternatively, can you share the H5P where you can reproduce this ?
I'm not able to reproduce this locally or on our Moodle test server.

@dnutzz
Copy link

dnutzz commented Oct 7, 2020

@dnutzz Yes, it seems likely that this is a timing issue. Do you have a public URL where this can be reproduced, the steps to reproduce it and the browsers and browser versions this can be seen in ? Or alternatively, can you share the H5P where you can reproduce this ?
I'm not able to reproduce this locally or on our Moodle test server.

Unfortunately we have no public courses (in terms of accessing them without an account). But all our courses are free, so I can only invite you to create a free accout (which you can delete afterwards) here and then visit this course.
Here you can find randomly resizing of the embedded h5p videos. In the screenshot you can see above the correct resized video from the course (should use 100% width of the wrapping container) and below a video, which has the described failure. It's happening on the latest Firefox, Chrome and Safari Browsers. You might need to F5 sometimes and click through all the videos and then you might be able to catch a faulty one.

image

@otacke
Copy link
Contributor

otacke commented Oct 7, 2020

@dnutzz Are you by any chance using some custom H5P iframe integration that's similar to the one that oncampus (formerly mooin) is using for their platform? That one had bugs which could lead to two competing resize functions on the iframe which a) could result in flickering and b) could lead to strange resize behavior.

@anrichp
Copy link

anrichp commented Oct 7, 2020

@thomasmars We experience this issue specifically with H5P built into core Moodle.
When adding an H5P activity to a discussion forum description the height of the activity breaks. However, when adding H5P activities to course pages this issue does not exist. In our case, this happens when adding H5P inside of an activity,

@thomasmars
Copy link
Member

@dnutzz As Otacke is saying, you seem to be using a custom iframe solution, the H5P embed code is wrapped within a "iframeplayer":

<iframe class="iframeplayer" type="text/html" src="https://imoox.at/mooc/mod/hvp/embed.php?id=8451" frameborder="0" allowfullscreen="" width="640" height="480" scrolling="no">
</iframe>

That iframe is limiting the size of the H5P embed. You'll have to solve it for that integration, I don't know if this is something theme specific or something special that you have implemented yourselves.

Here is a working iframe from your iframeplayer:

<iframe class="iframeplayer" type="text/html" src="https://imoox.at/mooc/mod/hvp/embed.php?id=8448" frameborder="0" allowfullscreen="" width="640" height="480" scrolling="no" style="width: 100%; height: 647px;">
</iframe>

Notice the difference in that the style has been applied to this iframe width: 100% which is necessary for the H5P iframe to be able to utilize the full width.

@anrichp Please contact Moodle core if you're using the H5P integration they've made for core, or optionally you can use the H5P plugin as provided here and I'll be able to look further into it if you're experiencing the same problem.


For anyone else experiencing this problem. Please check if there can be any problems arising from conflict with other javascript, themes or special embed techniques you are using. You can check this by embedding the H5P embed on a clean page, if this works then it is likely something to do with your setup, and you can find the likely source of the issue by disabling themes/plugins/special scripts one by one until you find what has caused the issue and then contact their support, get rid of it or look into how the issue can be amended.

I'll be happy to look into any issues where you the issue is reproducible and you can rule out any interference of third party themes/scripts. Thanks.

@dnutzz
Copy link

dnutzz commented Oct 7, 2020

@thomasmars thanks for pointing this out.
I've tested it now in a course, where we directly use the embed code suggested from h5p (taken from /mod/hvp/view.php) without any outer wrapper. It behaves the same way as you have described it. When it's working, the html-style attribute with width and height is being applied (this should come from h5p-resizer.js). I can test this because when I want to change in developer console the height it get's immediately resized by javascript.

On those cases it's not working, the style attribute stays empty (I think the js code does not get the hook on the element right > timing).

@anieminen
Copy link
Author

Here is another example of a H5P embed on a Page activity, that doesn't require logging in. Usually I can see the resize issue right after loading the activity, but sometimes I need to refresh the page a couple of times.

I did some testing in that Moodle installation. At first I couldn't reproduce the issue with H5P Moodle-plugin version 1.20.2 installed. But after upgrading mod_hvp to the latest version (1.21.0) the resize issue was there.

There are no third-party themes installed in that Moodle. There are a few additional plugins, but I don't think they are causing any problems.

@dnutzz
Copy link

dnutzz commented Oct 13, 2020

Can reproduce it on Firefox 81. It occurs randomly. When it's working, the style attribute is injected on the iframe wrapper (which is copy pasted from the hvp embed code modal). When it's not resized correctly, the style attribute is missing (see screenshots). This hook happens asynchronously. Video example: https://youtu.be/ahStxagJeGE

Not resized:

image
Screenshot from 2020-10-13 16-29-54

Resized:

image
Screenshot from 2020-10-13 16-33-04

@thomasmars
Copy link
Member

Thanks for the example.
I've been digging a bit into this and narrowed the issue down to a racing condition.
There are two functions, both relying on when the document is ready, and if the Moodle page document loads before the iframe this problem can be seen. I've created an issue in Jira where you can follow the progress: https://h5ptechnology.atlassian.net/browse/HFP-3142

@Urpokarhu1
Copy link

Hello,

I would like to see this fixed.

@tonket
Copy link

tonket commented Nov 13, 2020

Hey,

This bug is causing a lot of problems with many H5P users. I see that the Jira issue is still unassigned and priority only 'Medium'. Is the issue being worked at? Many Moodle courses are unusable and teachers are trying to find other ways or tools to deliver their content.

@Urpokarhu1
Copy link

I would like to see an fix to this

@marzipanbrain
Copy link

I just ran into this problem as well.
Looking at the code, this does look like a timing issue.
I think the problem is that on line 126 of h5p-resizer.js, the code checks the iframe(s) src for 'h5p' but it should be checking for 'hvp'. If that line is changed, then h5p-resizer.js will send a 'ready' message to the iframe to kick off the resizing process.

@alexmorrisnz
Copy link
Contributor

Looking at the code, this does look like a timing issue.
I think the problem is that on line 126 of h5p-resizer.js, the code checks the iframe(s) src for 'h5p' but it should be checking for 'hvp'. If that line is changed, then h5p-resizer.js will send a 'ready' message to the iframe to kick off the resizing process.

We've deployed this change to a production site and it hasn't had a noticeable effect on the issue.

@mabli
Copy link

mabli commented Jul 3, 2021

I have a similar problem on a Moodle 3.8 Plattform with Adaptable Design. The problem appears in different Browsers and could not be solved by simple adaption of the HTML-Code of the IFrame. It is described also here in this Moodle Forum Post & Discussion and here. Did anyone find a simple workaround?

@marzipanbrain
Copy link

I've found a couple of timing issues that could contribute to this problem.

  1. The first is in \mod\hvp\amd\src\embed.js, line 25. If this script runs before the H5P instance is fully set up, the script exits and never establishes the mechanism to that listens for resizes and communicates resizes to the parent iframe.

  2. The second is more obscure and less likely the cause of the problem people are seeing. In \mod\hvp\library\js\h5p.js, lines 415 to 419, the head contents of the intermediate iframe are generated. Due to the order of those lines, it is possible that the H5P object is created by a script before it is properly initialized on line 419 with H5P.externalEmbed = false.

Proposed fix for 1: If the H5P.instance is not set up, wait for the 'initialized' event and then set up the H5PEmbedCommunicator.
Proposed fix for 2: Move line 419 to before the createScriptTags() calls so that the H5P object will always have H5P.externalEmbed = false.

@marzipanbrain
Copy link

I expect that people following this issue are still interested in getting this resolved, so I thought I'd post an update.

The latest released version of the Moodle H5P plugin (release 1.22.4, version 2022012000) still does not solve the primary resizing bug (see point 1 in previous comment).
If you want a fix, you are welcome to look at https://github.com/marzipanbrain/moodle-mod_hvp/tree/HFP-3142_resize. This branch includes the released H5P updates AND a fix for the resizing bug. The only changes I've made are to version.php, /amd/src/embed.js, and the corresponding files in /amd/build/.

I hope this helps!

@golenkovm
Copy link
Contributor

Hi @marzipanbrain

Thank you for implementing the patch. This sounds promising and I really look forward to giving it a try until H5P team will be able to fix it. Is there a chance you could raise a pull request so it will clearly show the diff between canonical repo and your patched branch?

Kind regards,
Misha

@danowar2k
Copy link

Hi,
we're currently experiencing the race condition problem and tested the pull request code of catalyst#23 which fixes the issue. What can I do to help getting this approved?

@marzipanbrain
Copy link

Thanks @golenkovm for creating the pull request and @danowar2k for testing. Our site has been running with the patch since last fall and we haven't experienced any resizing issues since. As for getting this approved and into the hvp code base, good luck!
I tried reaching out to @thomasmars last fall but never heard back. Does anyone here know someone at hvp that can give this issue some attention?

@danowar2k
Copy link

Meanwhile, here's our local workaround (done in our theme):
Once any iframe is loaded, check if it is a h5p-iframe, and adjust the width if it is.

amd/src/h5pEmbeddedResizeFix.js

/**
 *
 * @module     theme_boost_custom/h5pEmbeddedResizeFix
 * @since      3.11
 */
import $ from 'jquery';

const fixEmbeddedResizing = () => {
    $(document).ready(function() {
        let iframes = $('iframe');
        iframes.on('load', function(loadedEvent) {
            let loadedIframe = $(loadedEvent.target);
            if (loadedIframe.has(".h5p-iframe")) {
                loadedIframe.css("width", "100%");
            }
        });


    });
};

export const init = () => {
    fixEmbeddedResizing();
};

classes/output/core_renderer.php

    public function standard_top_of_body_html() {
        global $PAGE;
        $output = parent::standard_top_of_body_html();
        $PAGE->requires->js_call_amd('theme_boost_custom/h5pEmbeddedResizeFix', 'init',[]);
        return $output;
    }

@danowar2k
Copy link

Well, it seems that there are two problems here, an easily solvable one and a harder problem. My code above solves the easy problem (setting the width to 100% of embedded h5p iframes).

The harder problem is "the necessary height of the h5p iframe is known at a point in time which can be anytime before or after something else happened or not."

Trying too early to resize the embed iframe may result in the wrong height read from the h5p iframe (which may even be 1 sometimes).

And just waiting for the h5p iframe document to be ready may be too late. Or the h5p iframe document could declare itself ready before the code that does $(h5pIframeDoc).ready() is even run through, so that the ready listener listens for a thing that has already happened.

Another variant of the above code did this and failed setting the height, too.

Meanwhile, we just now tested the catalyst pull request (catalyst#23) after understanding what it does (just delaying the resize messages until H5P says the h5p iframe document is initialized correctly).

Right now it looks like those changes to embed.js do the trick. Funny because h5p-resizer.js isn't even included on the pages.
But i looks like the h5p-resizer.js file is more for embedding the activities in pages external to Moodle, as some resizing code seems to be present in the plugin js libraries.

tl;dr The catalyst PR works, please merge. :-)

@golenkovm golenkovm linked a pull request Jul 20, 2022 that will close this issue
@danowar2k
Copy link

We're still using the resize fix above, please merge that fix :-)

@Urpokarhu1
Copy link

When will this fix be released?

@danowar2k
Copy link

The above fix in combination with a fix for using embed.php (

moodle-mod_hvp/embed.php

Lines 75 to 78 in b1105da

$view = new \mod_hvp\view_assets($cm, $course, [
'disabledownload' => $disabledownload,
'disablefullscreen' => $disablefullscreen
]);
) and to class view_assets (https://github.com/h5p/moodle-mod_hvp/blob/stable/classes/view_assets.php#L86-L88) leads to working resizing (in Moodle >= 4.0) is necessary for at least Moodle 4.1.

embed.php needs to use the option forceembedtype and set it to div, because otherwise there are multiple levels of iframe + embed.php + h5p-resizer.js adding and that screws with dynamic resizing.

view_assets has a bug where the $options variable doesn't use the forceembedtype array index but instead stdClass notation ($option->forceembedtype).

@danowar2k
Copy link

Here's the PR for the $options bug: #545

@danowar2k
Copy link

Here's the PR for forcing the embedtype in embed.php to be a div: #546

@danowar2k
Copy link

Huh, I've just randomly decide to remove the embed.js fix for this (catalyst#23). It seems (!) that my two pull requests fix this bug on their own?

Maybe someone could confirm that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.