Moodle
  1. Moodle
  2. MDL-31368

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

    Details

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

      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.

      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
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - Since MDL-29624 has been integrated, i'm closing this one as fxed
          Hide
          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
          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
          Jonathan Champ added a comment -

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

          Show
          Jonathan Champ added a comment - Improved patch to avoid zeroing when the embedded content is something the DOM cannot reference (i.e. PDFs).
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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: