Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-31368

Embedded HTML resources don't make full use of available space, and have extra scrollbars.

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: None
    • Component/s: Resource
    • Labels:
    • Affected Branches:
      MOODLE_22_STABLE

      Description

      Related to MDL-22446, MDL-22736, MDL-29624, MDL-29987...

      When using the "Embed" display option for a HTML file in mod_resource, space is wasted around the embedding object element, and an extra scrollbar appears if the embedded HTML is bigger than the object element (see attached embed-html-dist.png).

      The attached embedheight.patch changes M.util.init_maximised_embed() to find the height of the embedded HTML via DOM getContentDocument, and resizes the object to not need scrollbars (see attached embed-html-patched.png).

      Unfortunately this isn't ready for release as:
      1. IE < 8 don't like it (although it seems to be no worse than without the patch for those browsers), and
      2. The embedded HTML height changes depending on how it's queried:
      automatically from M.util.init_maximised_embed(), or
      manually from JS console.

      FF 9
      auto: 2969
      manual: 2585

      Chromium 14
      auto: 2968
      manual: 2584

      IE 9
      auto: 19871
      manual: 2633

      For Firefox and Chromium it's not too bad, but IE is out by an order of magnitude!

      So - not production ready, but I wanted to post what I've done so far in case someone else can figure out how to fix those issues.

        Gliffy Diagrams

        1. ann-file.txt
          2 kB
          Anna M
        2. calculate_resource_height_and_use_if_available_v2.patch
          1 kB
          Jonathan Champ
        3. embedheight.patch
          2 kB
          David Balch
        1. embed-html-dist.png
          35 kB
        2. embed-html-patched.png
          43 kB
        3. wasted-whitespace-snapshot.png
          255 kB

          Issue Links

            Activity

            Hide
            quen Sam Marshall added a comment -

            Agree an improvement is urgently needed in this area (not just for HTML but for all kinds of embedded resource that use the iframe scrollbar approach - it'll do it for PDFs as well, for instance).

            I'm marking this as blocked by my issue MDL-29624 (which is ready to go in but still awaiting review). That is a large change affecting video embedding and though it doesn't relate directly to this change, I suspect there might be conflicts.

            Show
            quen Sam Marshall added a comment - Agree an improvement is urgently needed in this area (not just for HTML but for all kinds of embedded resource that use the iframe scrollbar approach - it'll do it for PDFs as well, for instance). I'm marking this as blocked by my issue MDL-29624 (which is ready to go in but still awaiting review). That is a large change affecting video embedding and though it doesn't relate directly to this change, I suspect there might be conflicts.
            Hide
            balchd David Balch added a comment -

            D'oh! I hadn't realised you'd already done so much - nice work
            I agree that waiting for MDL-29624 would be most productive.

            Show
            balchd David Balch added a comment - D'oh! I hadn't realised you'd already done so much - nice work I agree that waiting for MDL-29624 would be most productive.
            Hide
            poltawski Dan Poltawski added a comment -

            Since MDL-29624 has been integrated, i'm closing this one as fxed

            Show
            poltawski Dan Poltawski added a comment - Since MDL-29624 has been integrated, i'm closing this one as fxed
            Hide
            jrchamp Jonathan Champ added a comment -

            I'm not sure this is fixed. My 2.3.7 is experiencing this issue and the JavaScript for the maximize function is the same as in 2.6dev. I'm attaching my patch which is David's size code, but with small tweaks.

            Show
            jrchamp Jonathan Champ added a comment - I'm not sure this is fixed. My 2.3.7 is experiencing this issue and the JavaScript for the maximize function is the same as in 2.6dev. I'm attaching my patch which is David's size code, but with small tweaks.
            Hide
            jrchamp Jonathan Champ added a comment -

            Improved patch to avoid zeroing when the embedded content is something the DOM cannot reference (i.e. PDFs).

            Show
            jrchamp Jonathan Champ added a comment - Improved patch to avoid zeroing when the embedded content is something the DOM cannot reference (i.e. PDFs).
            Hide
            annamoodmath Anna M added a comment - - edited

            Hi,
            I tried both David's patch and Jonathan's latest patch (calculate_resource_height_and_use_if_available_v2.patch) and none of them is working. Is it supposed to work and maybe I miss something? I try to embed an HTML resource and there is still wasted space below the embedded object. I attached what I did in ann-file.txt (this is pretty much supposed to be from Jonathan's patch). I tried it on both Safari and Firefox.

            Show
            annamoodmath Anna M added a comment - - edited Hi, I tried both David's patch and Jonathan's latest patch (calculate_resource_height_and_use_if_available_v2.patch) and none of them is working. Is it supposed to work and maybe I miss something? I try to embed an HTML resource and there is still wasted space below the embedded object. I attached what I did in ann-file.txt (this is pretty much supposed to be from Jonathan's patch). I tried it on both Safari and Firefox.
            Hide
            jrchamp Jonathan Champ added a comment -

            It looks like your version has been modified since my patch was generated.

            You might want to try switching the "YAHOO.util.Dom.getViewportHeight()" to "Y.one('body').get('winHeight')" without the double quotes of course. If that works, please let me know.

            Show
            jrchamp Jonathan Champ added a comment - It looks like your version has been modified since my patch was generated. You might want to try switching the "YAHOO.util.Dom.getViewportHeight()" to "Y.one('body').get('winHeight')" without the double quotes of course. If that works, please let me know.
            Hide
            annamoodmath Anna M added a comment -

            I already switched "YAHOO.util.Dom.getViewportHeight()" to "Y.one('body').get('winHeight')", but it's not working yet. I tried in both Safari and Firefox. My latest change is at: ann-file-2.txt

            Show
            annamoodmath Anna M added a comment - I already switched "YAHOO.util.Dom.getViewportHeight()" to "Y.one('body').get('winHeight')", but it's not working yet. I tried in both Safari and Firefox. My latest change is at: ann-file-2.txt
            Hide
            jrchamp Jonathan Champ added a comment -

            It looks like you are now missing the line "var newheight;". After modifying the javascript-static.js, you'll probably need to use the "Purge all caches" button in Moodle and then clear the browser cache.

            Actually, are you getting scrollbars on the embedded item? Or is the issue that you are seeing just "too much whitespace"? The goal of the patch was to display the entire embedded content such that the embed area was large enough for the content, transferring any applicable scrollbars to the top level page.

            Show
            jrchamp Jonathan Champ added a comment - It looks like you are now missing the line "var newheight;". After modifying the javascript-static.js, you'll probably need to use the "Purge all caches" button in Moodle and then clear the browser cache. Actually, are you getting scrollbars on the embedded item? Or is the issue that you are seeing just "too much whitespace"? The goal of the patch was to display the entire embedded content such that the embed area was large enough for the content, transferring any applicable scrollbars to the top level page.
            Hide
            annamoodmath Anna M added a comment -

            Hi Jonathan,

            Thanks for your reply. I am back to ann-file.txt version (except for changing "YAHOO.util.Dom.getViewportHeight()" to "Y.one('body').get('winHeight')"). I attached wasted-whitespace-snapshot.png, under the embedded object, I put red arrow showing the whitespace I am trying to get rid of. The scrollbar appears on the embedded item (it didn't show up in the pic, but I can scroll down), but no scrollbar appears on the top page. I do not mind the scrollbar on the embedded item, what I really want is removing the whitespace. I also have "purged all cache" from Site Admin > Development > Purge all cache (from within moodle) and I also clear caches in both Safari and Firefox. But the result is still the same. Any idea?
            Thanks,

            Ann

            Show
            annamoodmath Anna M added a comment - Hi Jonathan, Thanks for your reply. I am back to ann-file.txt version (except for changing "YAHOO.util.Dom.getViewportHeight()" to "Y.one('body').get('winHeight')"). I attached wasted-whitespace-snapshot.png, under the embedded object, I put red arrow showing the whitespace I am trying to get rid of. The scrollbar appears on the embedded item (it didn't show up in the pic, but I can scroll down), but no scrollbar appears on the top page. I do not mind the scrollbar on the embedded item, what I really want is removing the whitespace. I also have "purged all cache" from Site Admin > Development > Purge all cache (from within moodle) and I also clear caches in both Safari and Firefox. But the result is still the same. Any idea? Thanks, Ann
            Hide
            jrchamp Jonathan Champ added a comment -

            Hi Ann,

            It looks like the page is close to being correct, so you may just want to adjust the "+ 70" to a larger number. In this case, even 200 would be reasonable. If it does not make a visual difference, it may be that the "else" code is executing, in which case, reducing the "- 100" to a smaller number, such as 0, would help.

            If you need to test (hopefully you have a server to test on) to make sure the new JavaScript is running, you can add something like the following after the newheight is finalized:

            if (newheight < 400) {
                newheight = 400;
            }
            alert('Resource Height: ' + JSON.stringify(resourceheight) + '; Setting Embedded Height to: ' + JSON.stringify(newheight));

            Show
            jrchamp Jonathan Champ added a comment - Hi Ann, It looks like the page is close to being correct, so you may just want to adjust the "+ 70" to a larger number. In this case, even 200 would be reasonable. If it does not make a visual difference, it may be that the "else" code is executing, in which case, reducing the "- 100" to a smaller number, such as 0, would help. If you need to test (hopefully you have a server to test on) to make sure the new JavaScript is running, you can add something like the following after the newheight is finalized: if (newheight < 400) { newheight = 400; } alert('Resource Height: ' + JSON.stringify(resourceheight) + '; Setting Embedded Height to: ' + JSON.stringify(newheight));
            Hide
            annamoodmath Anna M added a comment -

            Hi Jonathan,

            Thanks for the suggestion. It is working now!!! I attached the change in ann-file.txt. It is actually the 'else' part that gets executed.

            Ann

            Show
            annamoodmath Anna M added a comment - Hi Jonathan, Thanks for the suggestion. It is working now!!! I attached the change in ann-file.txt. It is actually the 'else' part that gets executed. Ann
            Hide
            jrchamp Jonathan Champ added a comment -

            I'm delighted to heard that it's working for you! The if part is supposed to be executed when the embedded content is a webpage, but relies on the ability of the JavaScript to access the embedded content at the expected identifier.

            Show
            jrchamp Jonathan Champ added a comment - I'm delighted to heard that it's working for you! The if part is supposed to be executed when the embedded content is a webpage, but relies on the ability of the JavaScript to access the embedded content at the expected identifier.

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: