Moodle
  1. Moodle
  2. MDL-20320

PDFs are not displaying according to settings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.5
    • Fix Version/s: 1.9.6
    • Component/s: Resource
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      31337

      Description

      In 1.9 PDFs are not displaying according to the settings.

      See http://moodle.org/mod/forum/discuss.php?d=132906#p581622 for details.

      In short, here are combinations and expectations:

      1) Same window, navigation = No ..... PDF should be called directly and it isn't.
      2) Same window, navigation = Yes, with Frame ..... PDF should appear in lower frame and it isn't.
      3) Same window, navigation = Yes, without Frame .... PDF should appear embedded and it is (this is what happens for above two as well)

      Apparently all that is needed is to remove $embedded = true but I'm not sure if that's 100% correct on all browsers. Can you review this and fix it?

      1. MDL-20320_fix_only_PDF.patch
        2 kB
        Jérôme Mouneyrac
      2. MDL-20320.patch
        6 kB
        Jérôme Mouneyrac
      3. MDL-20320-db-upgrade.patch
        2 kB
        Jérôme Mouneyrac
      4. MDL-20320-db-upgrade.patch
        2 kB
        Jérôme Mouneyrac
      5. MDL-20320-db-upgrade.patch
        4 kB
        Jérôme Mouneyrac
      6. MDL-20320-db-upgrade.patch
        2 kB
        Jérôme Mouneyrac
      7. MDL-20320-db-upgrade.patch
        2 kB
        Jérôme Mouneyrac
      8. MDL-20320-fix_PDF-add_notice.patch
        3 kB
        Jérôme Mouneyrac

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          MDL-10021 is the bug where these settings were last looked at

          Show
          Martin Dougiamas added a comment - MDL-10021 is the bug where these settings were last looked at
          Hide
          Jérôme Mouneyrac added a comment -

          Same behavior as before the fix of MDL-10021
          I guess it's just a wording error, the option should be named:
          "Keep page navigation visible on the same page for html file"

          Show
          Jérôme Mouneyrac added a comment - Same behavior as before the fix of MDL-10021 I guess it's just a wording error, the option should be named: "Keep page navigation visible on the same page for html file"
          Hide
          Jérôme Mouneyrac added a comment -

          Here is the situation:

          mod/resource/type/file/resource.class.php:

          • mp3, flv, mediaplayer, quicktime, flash, mpeg, zip, pdf, rm are always embedded in an object tag with a menu displayed at the top of the page . The option "Keep page navigation visible on the same page" doesnt affect them.
          • html and other (like mp4) are affected by the option.

          The MDL-10021 fix didn't change the behaviors, it just added the possibility to choose between frame, noframe and object for HTML/unknown files

          The resource code has been refactored in HEAD.
          To fix this problem 1.9 does need to be refactored too. It will probably take a day of work so it's ok, but my main concerns are:

          • It could bring some new bugs (for finally fixing a behavior that in fact didn't change). The code is messy.
          • We probably would have a new wave of complains. (mp3, flv,... could be displayed differently, we need to alert people again,...)

          If there is no mandatory needs to have mp3, pdf, displaying no menu or a frame, I'm really for doing no change, and just fixing the wording.

          Show
          Jérôme Mouneyrac added a comment - Here is the situation: mod/resource/type/file/resource.class.php: mp3, flv, mediaplayer, quicktime, flash, mpeg, zip, pdf, rm are always embedded in an object tag with a menu displayed at the top of the page . The option "Keep page navigation visible on the same page" doesnt affect them. html and other (like mp4) are affected by the option. The MDL-10021 fix didn't change the behaviors, it just added the possibility to choose between frame, noframe and object for HTML/unknown files The resource code has been refactored in HEAD. To fix this problem 1.9 does need to be refactored too. It will probably take a day of work so it's ok, but my main concerns are: It could bring some new bugs (for finally fixing a behavior that in fact didn't change). The code is messy. We probably would have a new wave of complains. (mp3, flv,... could be displayed differently, we need to alert people again,...) If there is no mandatory needs to have mp3, pdf, displaying no menu or a frame, I'm really for doing no change, and just fixing the wording.
          Hide
          Martin Dougiamas added a comment -

          Thanks Jerome!

          I'd really like to see all those types behaving as described in the current menu items ASAP. People should be able to use frames when they want to (as long as it is explicitly described so they know the pitfalls wrt accessibility).

          I think fixing this is pretty simple, just removing the $embedded = true from those items in the switch statement to make those media types act like HTML and undefined media types do.

          The main problem is going to be the change of behaviour in 1.9 stable.

          1) Sites already in 1.9 are going to see slight changes in how courses are displaying.
          2) Sites upgrading to 1.9 from earlier branches could see wierd changes in display ... not sure.

          I'd like to see some community testing of all the upgrade permutations to make sure that we aren't going to introduce any major regressions here before we make these changes in 1.9.

          Show
          Martin Dougiamas added a comment - Thanks Jerome! I'd really like to see all those types behaving as described in the current menu items ASAP. People should be able to use frames when they want to (as long as it is explicitly described so they know the pitfalls wrt accessibility). I think fixing this is pretty simple, just removing the $embedded = true from those items in the switch statement to make those media types act like HTML and undefined media types do. The main problem is going to be the change of behaviour in 1.9 stable. 1) Sites already in 1.9 are going to see slight changes in how courses are displaying. 2) Sites upgrading to 1.9 from earlier branches could see wierd changes in display ... not sure. I'd like to see some community testing of all the upgrade permutations to make sure that we aren't going to introduce any major regressions here before we make these changes in 1.9.
          Hide
          Jérôme Mouneyrac added a comment -

          no worries I'll do the change, add a patch here and ask the community on the forums.

          Show
          Jérôme Mouneyrac added a comment - no worries I'll do the change, add a patch here and ask the community on the forums.
          Hide
          Jérôme Mouneyrac added a comment -

          it is not as simple as it, remove embedded = true will display the OS media player for MP3 file for example, we will lose all specific tags to the different extension, I need to refactor a bit the code...

          Show
          Jérôme Mouneyrac added a comment - it is not as simple as it, remove embedded = true will display the OS media player for MP3 file for example, we will lose all specific tags to the different extension, I need to refactor a bit the code...
          Show
          Jérôme Mouneyrac added a comment - just a reminder to update: http://docs.moodle.org/en/File_or_website_link#Keep_page_navigation_visible_on_the_same_page
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I attached a patch. We still need to edit resource db/upgrade.php in order to manage different 1.9 config upgrade. I'll do it later however you can test the patch now.

          Show
          Jérôme Mouneyrac added a comment - - edited I attached a patch. We still need to edit resource db/upgrade.php in order to manage different 1.9 config upgrade. I'll do it later however you can test the patch now.
          Hide
          Mauno Korpelainen added a comment -

          I tested it quickly... The original issue was about showing pdf files like html pages - not the other embedded files - but even after this patch PDF (Keep page navigation visible on the same page - Yes, without frame) does not look "correct" (whole width and height) if $embedded is true for pdf because resource.class.php will skip over

          if (empty($frameset) and !$embedded and !$inpopup and ($resource->options == "frame" || $resource->options == "objectframe") and empty($USER->screenreader))

          {...}

          and source code will have something like

          <div class="resourcepdf"><object data="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" type="application/pdf"><param name="src" value="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" />To open this document, click on this link: <a href="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf">PDF - Same Window - Nav no Frame</a></object></div>

          If $embedded is false result seems to be more correct and moodle will use Yahoo javascript libaries for updating embedded object size with source code

          <p><object id="embeddedhtml" data="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" type="application/pdf" width="800" height="600">
          alt : <a href="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf">http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf</a>
          </object></p><script type="text/javascript">
          //<![CDATA[
          function resizeEmbeddedHtml() {
          //calculate new embedded html height size
          objectheight = YAHOO.util.Dom.getViewportHeight() - 120;
          //the object tag cannot be smaller than a human readable size
          if (objectheight < 200)

          { objectheight = 200; }

          //resize the embedded html object
          YAHOO.util.Dom.setStyle("embeddedhtml", "height", objectheight+"px");
          YAHOO.util.Dom.setStyle("embeddedhtml", "width", "100%");
          }
          resizeEmbeddedHtml();
          YAHOO.widget.Overlay.windowResizeEvent.subscribe(resizeEmbeddedHtml);
          //]]>
          </script>

          Should not default value for $embedded for pdf be false like Jon in Jon's demo?

          Show
          Mauno Korpelainen added a comment - I tested it quickly... The original issue was about showing pdf files like html pages - not the other embedded files - but even after this patch PDF (Keep page navigation visible on the same page - Yes, without frame) does not look "correct" (whole width and height) if $embedded is true for pdf because resource.class.php will skip over if (empty($frameset) and !$embedded and !$inpopup and ($resource->options == "frame" || $resource->options == "objectframe") and empty($USER->screenreader)) {...} and source code will have something like <div class="resourcepdf"><object data="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" type="application/pdf"><param name="src" value="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" />To open this document, click on this link: <a href="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf">PDF - Same Window - Nav no Frame</a></object></div> If $embedded is false result seems to be more correct and moodle will use Yahoo javascript libaries for updating embedded object size with source code <p><object id="embeddedhtml" data="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" type="application/pdf" width="800" height="600"> alt : <a href="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf"> http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf </a> </object></p><script type="text/javascript"> //<![CDATA[ function resizeEmbeddedHtml() { //calculate new embedded html height size objectheight = YAHOO.util.Dom.getViewportHeight() - 120; //the object tag cannot be smaller than a human readable size if (objectheight < 200) { objectheight = 200; } //resize the embedded html object YAHOO.util.Dom.setStyle("embeddedhtml", "height", objectheight+"px"); YAHOO.util.Dom.setStyle("embeddedhtml", "width", "100%"); } resizeEmbeddedHtml(); YAHOO.widget.Overlay.windowResizeEvent.subscribe(resizeEmbeddedHtml); //]]> </script> Should not default value for $embedded for pdf be false like Jon in Jon's demo?
          Hide
          Mauno Korpelainen added a comment -

          If javascript is disabled result is of course different...

          Show
          Mauno Korpelainen added a comment - If javascript is disabled result is of course different...
          Hide
          Jérôme Mouneyrac added a comment -

          attached new patch

          Show
          Jérôme Mouneyrac added a comment - attached new patch
          Hide
          Jérôme Mouneyrac added a comment -

          I attached a fix only PDF patch

          Show
          Jérôme Mouneyrac added a comment - I attached a fix only PDF patch
          Hide
          Jérôme Mouneyrac added a comment -

          If we decide to fix only PDF, as a dev I'm happy there will be less potential bug. However "Keep navigation" still need some rewording explaining that it doesn't work with some specific files (MP3,...). Some suggestions?

          Show
          Jérôme Mouneyrac added a comment - If we decide to fix only PDF, as a dev I'm happy there will be less potential bug. However "Keep navigation" still need some rewording explaining that it doesn't work with some specific files (MP3,...). Some suggestions?
          Hide
          Jérôme Mouneyrac added a comment -

          http://docs.moodle.org/en/File_or_website_link#Keep_page_navigation_visible_on_the_same_page says:
          "Note that this option is normally not necessary for media types such as movies, audio files and flash files, as without this option turned on they will be embedded within a navigable page."
          so we probably should only fix PDF (at this moment PDF, ZIP and image are also considered as media).

          Show
          Jérôme Mouneyrac added a comment - http://docs.moodle.org/en/File_or_website_link#Keep_page_navigation_visible_on_the_same_page says: "Note that this option is normally not necessary for media types such as movies, audio files and flash files, as without this option turned on they will be embedded within a navigable page." so we probably should only fix PDF (at this moment PDF, ZIP and image are also considered as media).
          Hide
          Martin Dougiamas added a comment -

          OK, after discussion .... we might create more issues if we tinker too much with how 1.9.x works.

          So let's do this for 1.9.6:

          1) Remove the embedded=true setting for PDFs
          2) Add a description text in the form near the "show navigation" menu to say "Some media types may ignore this setting"

          Show
          Martin Dougiamas added a comment - OK, after discussion .... we might create more issues if we tinker too much with how 1.9.x works. So let's do this for 1.9.6: 1) Remove the embedded=true setting for PDFs 2) Add a description text in the form near the "show navigation" menu to say "Some media types may ignore this setting"
          Hide
          Jérôme Mouneyrac added a comment -

          Attached another patch:
          1. Fixed PDF only
          2. Added comment under the setting saying that some media file ignore this setting
          3. Renamed 'Keep navigation visible on the same page' into 'Show navigation'

          Show
          Jérôme Mouneyrac added a comment - Attached another patch: 1. Fixed PDF only 2. Added comment under the setting saying that some media file ignore this setting 3. Renamed 'Keep navigation visible on the same page' into 'Show navigation'
          Hide
          Jérôme Mouneyrac added a comment -

          committed into 1.9
          Thank you.

          Show
          Jérôme Mouneyrac added a comment - committed into 1.9 Thank you.
          Hide
          Jérôme Mouneyrac added a comment -

          We should set the option to "yes, without frame" for people upgrading otherwise they will have to change all their PDF resource settings... fixing it

          Show
          Jérôme Mouneyrac added a comment - We should set the option to "yes, without frame" for people upgrading otherwise they will have to change all their PDF resource settings... fixing it
          Hide
          Jérôme Mouneyrac added a comment -

          Hi Eloy,
          can you review the upgrade database patch and commit it?

          Explantion of this issue and the two patches:
          In resource settings page, the ' Show navigation' setting (previously called 'Keep navigation on the same page') had no effect on PDF files. PDF files were always displayed into an object tag.
          The first patch fixed it. This patch has been committed.

          Now PDF files can be displayed into a frame ('Yes, with frame'), into an object tag ('Yes, without frame') or just displayed without navigation ('No').

          We need a DB upgrade patch because on previous installation, PDF were always displayed as 'Yes, without frame'. The default being 'No', they will be displayed without navigation now. We need to update all PDF entries into the database to put them back to 'Yes, without frame'.

          Thanks a lot

          Show
          Jérôme Mouneyrac added a comment - Hi Eloy, can you review the upgrade database patch and commit it? Explantion of this issue and the two patches: In resource settings page, the ' Show navigation' setting (previously called 'Keep navigation on the same page') had no effect on PDF files. PDF files were always displayed into an object tag. The first patch fixed it. This patch has been committed. Now PDF files can be displayed into a frame ('Yes, with frame'), into an object tag ('Yes, without frame') or just displayed without navigation ('No'). We need a DB upgrade patch because on previous installation, PDF were always displayed as 'Yes, without frame'. The default being 'No', they will be displayed without navigation now. We need to update all PDF entries into the database to put them back to 'Yes, without frame'. Thanks a lot
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          The patch looks ok but:

          1) Instead of getting all records with all columns, I'd get only those needed (id, type, reference, popup, options). And perhaps I'd use a recordset (I can imagine one site with zillions of pdfs).

          2) As it's only a matter of updating one field, perhaps set_field() is better than update_record (far less to write).

          3) IMO, you also need to check that popup is empty, because resources being showed in new window must not be updated to "objectframe". So condition must be:

          if (empty($resource->popup && (empty($resource->options) || $resource->options == 'frame' )) {
          

          4) Finally, IMO this should ALSO be merged into 2.0 resources (before the split into new modules happen). Please confirm this with Petr.

          5) Edited: ".PDF" files aren't processed. Need to support them too.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited The patch looks ok but: 1) Instead of getting all records with all columns, I'd get only those needed (id, type, reference, popup, options). And perhaps I'd use a recordset (I can imagine one site with zillions of pdfs). 2) As it's only a matter of updating one field, perhaps set_field() is better than update_record (far less to write). 3) IMO, you also need to check that popup is empty, because resources being showed in new window must not be updated to "objectframe". So condition must be: if (empty($resource->popup && (empty($resource->options) || $resource->options == 'frame' )) { 4) Finally, IMO this should ALSO be merged into 2.0 resources (before the split into new modules happen). Please confirm this with Petr. 5) Edited: ".PDF" files aren't processed. Need to support them too.
          Hide
          Petr Škoda added a comment -

          2.0 has a branch new code that should not have any problem like this, of course if there is any need for DB upgrades the existing migration code should be updated.

          Looking at the code the update_record('resource',$resource); is a SQL injection - there would have to be addslashes_recursive(). I agree with Eloy, this would definitely run out of memory on most sites - you have to use recordsets and specify the fetched fields.

          Show
          Petr Škoda added a comment - 2.0 has a branch new code that should not have any problem like this, of course if there is any need for DB upgrades the existing migration code should be updated. Looking at the code the update_record('resource',$resource); is a SQL injection - there would have to be addslashes_recursive(). I agree with Eloy, this would definitely run out of memory on most sites - you have to use recordsets and specify the fetched fields.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          About 2.0 Petr, is it a matter of "cloning" the upgrade block in 2.0 mod/resource (before the migration happens, of course). Or is it a matter of changing the migration itself to fix those resources ?

          Jerome, just noticed another minor thing: ".PDF" (uppercase) isn't fixed by the patch. (I've edited my original comment).

          Show
          Eloy Lafuente (stronk7) added a comment - About 2.0 Petr, is it a matter of "cloning" the upgrade block in 2.0 mod/resource (before the migration happens, of course). Or is it a matter of changing the migration itself to fix those resources ? Jerome, just noticed another minor thing: ".PDF" (uppercase) isn't fixed by the patch. (I've edited my original comment).
          Hide
          Petr Škoda added a comment -

          I looks like this new upgrade step could be implemented as some extra ifs during migration of both 'url' and 'resource' module.

          Show
          Petr Škoda added a comment - I looks like this new upgrade step could be implemented as some extra ifs during migration of both 'url' and 'resource' module.
          Hide
          Jérôme Mouneyrac added a comment -

          Hi all,
          thanks for the reviews

          I tested that popup were not impacted but I'll add the test for security to a new patch.
          I'll also add addslashes_recursive for security (I will unset everything except 'options' and 'id' so I think we should not have any need of slashes formatting anyway)
          I'll use get_recordset instead of get_records. Thanks for the advice, I only had a quick look to recordset_to_array() though that get_records had a good name to be picked safetly

          The new patch will be committed today after review, then the weekly build (on hold at this moment) will be rebuild and released.

          Thanks

          Show
          Jérôme Mouneyrac added a comment - Hi all, thanks for the reviews I tested that popup were not impacted but I'll add the test for security to a new patch. I'll also add addslashes_recursive for security (I will unset everything except 'options' and 'id' so I think we should not have any need of slashes formatting anyway) I'll use get_recordset instead of get_records. Thanks for the advice, I only had a quick look to recordset_to_array() though that get_records had a good name to be picked safetly The new patch will be committed today after review, then the weekly build (on hold at this moment) will be rebuild and released. Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          quickly add a new patch for Martin

          Show
          Jérôme Mouneyrac added a comment - quickly add a new patch for Martin
          Hide
          Jérôme Mouneyrac added a comment -

          Hi I'm on my iPhone at the hospital. I've just tested the patch I attached before to get my cab, it works well, ready for reviewing and being committed hopefully. Thanks Martin

          Show
          Jérôme Mouneyrac added a comment - Hi I'm on my iPhone at the hospital. I've just tested the patch I attached before to get my cab, it works well, ready for reviewing and being committed hopefully. Thanks Martin
          Hide
          Jérôme Mouneyrac added a comment -

          A new optimized patch for site with Zillion of PDF

          Show
          Jérôme Mouneyrac added a comment - A new optimized patch for site with Zillion of PDF
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 here

          I think you could have saved the 2nd loop by doing all in the 1st one but your solution should work fine too (I hope nobody will have really zillions of resources or that $resources array will boom too, hehe).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - +1 here I think you could have saved the 2nd loop by doing all in the 1st one but your solution should work fine too (I hope nobody will have really zillions of resources or that $resources array will boom too, hehe). Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Petr came back with the best solution, attached what I hope to be the last patch

          Show
          Jérôme Mouneyrac added a comment - Petr came back with the best solution, attached what I hope to be the last patch
          Hide
          Petr Škoda added a comment - - edited

          hehe, this ain't gonna work for bloody oracle, remember that ugly one space hack for empty strings there

          Show
          Petr Škoda added a comment - - edited hehe, this ain't gonna work for bloody oracle, remember that ugly one space hack for empty strings there
          Hide
          Jérôme Mouneyrac added a comment -

          another patch to review

          Show
          Jérôme Mouneyrac added a comment - another patch to review
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1

          Also, from HQ Chat:

          [11:18:20] <Eloy> btw.. that transformation should also be applied somewhere in the 1.9 => 2.0 migration (uhm... although it's destructive, replacing empties by "objectframe" twice. Uhm)
          [11:18:57] <skodak> please file a separate issue for this - it will be in "url" and "resource" modules

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - +1 Also, from HQ Chat: [11:18:20] <Eloy> btw.. that transformation should also be applied somewhere in the 1.9 => 2.0 migration (uhm... although it's destructive, replacing empties by "objectframe" twice. Uhm) [11:18:57] <skodak> please file a separate issue for this - it will be in "url" and "resource" modules Ciao
          Hide
          Jérôme Mouneyrac added a comment -

          Resolving thanks everybody

          Show
          Jérôme Mouneyrac added a comment - Resolving thanks everybody
          Hide
          Eloy Lafuente (stronk7) added a comment -

          good evolution! Tested all combinations seems to work.

          Show
          Eloy Lafuente (stronk7) added a comment - good evolution! Tested all combinations seems to work.
          Hide
          Tim Hunt added a comment -

          You have a hard-coded mdl_

          And you are not checking for SQL errors, so after the table does not exist error, it says 'upgrade completed successfully'

          I am about to commit a fix.

          Show
          Tim Hunt added a comment - You have a hard-coded mdl_ And you are not checking for SQL errors, so after the table does not exist error, it says 'upgrade completed successfully' I am about to commit a fix.
          Hide
          Tim Hunt added a comment -

          OK. Re-fixed.

          I guess someone needs to re-roll the weekly build.

          Show
          Tim Hunt added a comment - OK. Re-fixed. I guess someone needs to re-roll the weekly build.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Re-rolled ! Thanks Tim, I missed that point!

          Show
          Eloy Lafuente (stronk7) added a comment - Re-rolled ! Thanks Tim, I missed that point!
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Tim

          Show
          Jérôme Mouneyrac added a comment - Thanks Tim

            People

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

              Dates

              • Created:
                Updated:
                Resolved: