Moodle
  1. Moodle
  2. MDL-29987

Embedded PDF files cause problems in some configurations

    Details

    • Testing Instructions:
      Hide

      Note: This test run needs to be done using a browser that supports embedding PDF files, such as Firefox 21.

      • Enter a course when logged in as teacher.
      • Upload a file by drag & drop.
      • Click on the filename (to open the file).
      • Verify: PDF file is opened in a separate window/tab/viewer (not embedded in the Moodle page).
      • Return to the course page (with editing on).
      • Click on "Update" from the pop-up menu next to the file resource.
      • Set "Appearance -> Display" to "Embed", then click "Save and return to course".
      • Click on the filename again.
      • Verify: PDF file is opened in an embedded PDF viewer.
      Show
      Note: This test run needs to be done using a browser that supports embedding PDF files, such as Firefox 21. Enter a course when logged in as teacher. Upload a file by drag & drop. Click on the filename (to open the file). Verify: PDF file is opened in a separate window/tab/viewer (not embedded in the Moodle page). Return to the course page (with editing on). Click on "Update" from the pop-up menu next to the file resource. Set "Appearance -> Display" to "Embed", then click "Save and return to course". Click on the filename again. Verify: PDF file is opened in an embedded PDF viewer.
    • Workaround:
      Hide

      Use the "Display: Open" setting for the file resource.

      Show
      Use the "Display: Open" setting for the file resource.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
    • Rank:
      19532

      Description

      In Moodle 2.x, the default setting for the "File" resource is "Display: Automatic". For PDF files, this means that they will be shown as an embedded PDF viewer (if a browser plugin is available). This causes problems in various configurations:

      • In IE 8 (other versions not tested), the PDF file is displayed but not in a very user-friendly way: The "viewport" for the PDF file is rather small, covers only part of the screen, and is in landscape format (whereas typical PDF documents are portrait format, at least for us). There is no obvious option to open the document in a larger window.
      • Users of mobile devices (Android, iPad) have reported that the files are displayed, but they are unable to scroll to the next page. (Likely this is a problem with mobile browsers, but one that seems to be widespread.)

      All in all, I don't see much benefit for displaying PDFs in an embedded window, however I see that it causes problems to some of our users.

      Suggestions for a solution:
      (a) Remove PDF from the list of file types to be displayed in an embedded frame - see mod/resource/locallib.php, function resource_get_final_display_type(), variable $embed

      (b) Alternatively, display a link to the file above the embedded frame.

      I can provide a patch, but would be happy about some feedback regarding the options above.

      See also http://moodle.org/mod/forum/discuss.php?d=164712 .

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          I don't think the embed problem is unique to PDFs; anything that is embedded is embedded in a window that's far too small to be any use.

          This is not directly related to my issue MDL-31015 although it's in the same sort of area. Less obviously it is also linked to my MDL-29624 (about redoing media embedding) although I'm afraid I didn't directly touch this area (but, any change to it is likely to break, so probably wait for that to go in).

          My preferred solution would be something like:

          1) Set it so that nothing is ever embedded by default, except for the audio/video/Flash types.

          2) When users manually choose to embed something (such as a PDF or HTML page) use JavaScript and/or CSS code that automatically sets the size of the embed window to match the size of the user's browser. (It will still have two scrollbars, though, which is always suck.)

          Show
          Sam Marshall added a comment - I don't think the embed problem is unique to PDFs; anything that is embedded is embedded in a window that's far too small to be any use. This is not directly related to my issue MDL-31015 although it's in the same sort of area. Less obviously it is also linked to my MDL-29624 (about redoing media embedding) although I'm afraid I didn't directly touch this area (but, any change to it is likely to break, so probably wait for that to go in). My preferred solution would be something like: 1) Set it so that nothing is ever embedded by default, except for the audio/video/Flash types. 2) When users manually choose to embed something (such as a PDF or HTML page) use JavaScript and/or CSS code that automatically sets the size of the embed window to match the size of the user's browser. (It will still have two scrollbars, though, which is always suck.)
          Hide
          Richard Reeves added a comment -

          I'd second Sam's recommended solutions as presented.

          Show
          Richard Reeves added a comment - I'd second Sam's recommended solutions as presented.
          Hide
          Heikki Wilenius added a comment - - edited

          Yes, the suggested solutions seem apt. I think the priority of this issue should be major. A lot of our students are using mobile devices. Also, branch 2.2 is affected as well.

          Show
          Heikki Wilenius added a comment - - edited Yes, the suggested solutions seem apt. I think the priority of this issue should be major. A lot of our students are using mobile devices. Also, branch 2.2 is affected as well.
          Hide
          Barbara Taylor added a comment -

          We have 2.3 and this is causing us a big problem with iPad users. We have changed our default setting to "new window" so anything newly created will open in a new window. However, we have hundreds, if not thousands, of PDF files that we do not want instructors to have to go back and change from "Automatic" to "new window," "force download," or "pop-up" which is what we have tested. When a file is edited to one of these options then the PDF will work on an iPad. There has to be something causing the Automatic option to no longer work. Does anyone have any ideas? We didn't have issues with 1.9. It is something with 2.3. Could it be a theme issue or something in the Moodle code?

          Show
          Barbara Taylor added a comment - We have 2.3 and this is causing us a big problem with iPad users. We have changed our default setting to "new window" so anything newly created will open in a new window. However, we have hundreds, if not thousands, of PDF files that we do not want instructors to have to go back and change from "Automatic" to "new window," "force download," or "pop-up" which is what we have tested. When a file is edited to one of these options then the PDF will work on an iPad. There has to be something causing the Automatic option to no longer work. Does anyone have any ideas? We didn't have issues with 1.9. It is something with 2.3. Could it be a theme issue or something in the Moodle code?
          Hide
          Henning Bostelmann added a comment -

          @Barbara: "Automatic" defaults to "Embed" for PDF files in Moodle 2.x.

          In order to change this, look for the following piece of code in mod/resource/locallib.php near line 431:

          if (file_mimetype_in_typegroup($mimetype, array('web_image', '.pdf', '.htm', 'web_video', 'web_audio')))

          { return RESOURCELIB_DISPLAY_EMBED; }

          Remove '.pdf' from the list above, and it will now default to "Open" instead of "Embed". This should resolve the problem on iPads.

          Show
          Henning Bostelmann added a comment - @Barbara: "Automatic" defaults to "Embed" for PDF files in Moodle 2.x. In order to change this, look for the following piece of code in mod/resource/locallib.php near line 431: if (file_mimetype_in_typegroup($mimetype, array('web_image', '.pdf', '.htm', 'web_video', 'web_audio'))) { return RESOURCELIB_DISPLAY_EMBED; } Remove '.pdf' from the list above, and it will now default to "Open" instead of "Embed". This should resolve the problem on iPads.
          Hide
          Henning Bostelmann added a comment -

          Regarding point 2) raised by Sam, there seems to be some JS code already that maximizes the embedded frame. (See lib/resourcelib.php, function reourcelib_embed_pdf() etc.) However, this may have bugs; see MDL-31368.

          In still think that 1) should be implemented regardless.

          Show
          Henning Bostelmann added a comment - Regarding point 2) raised by Sam, there seems to be some JS code already that maximizes the embedded frame. (See lib/resourcelib.php, function reourcelib_embed_pdf() etc.) However, this may have bugs; see MDL-31368 . In still think that 1) should be implemented regardless.
          Hide
          Klickagent.ch added a comment -

          For all iOs users: As a bugfix for moodle users, I've built a javascript to generate an open link underneath the embedded pdf:

          http://tweaks.klickagent.ch/#pdfinmoodle

          Show
          Klickagent.ch added a comment - For all iOs users: As a bugfix for moodle users, I've built a javascript to generate an open link underneath the embedded pdf: http://tweaks.klickagent.ch/#pdfinmoodle
          Hide
          Chris Fryer added a comment -

          Henning's workaround is only applicable to Moodle 2.3. If you want to backport that workaround to Moodle 2.2, the following patch will work:

          
          diff --git a/mod/resource/locallib.php b/mod/resource/locallib.php
          index 41b77cc..7aea0d9 100644
          --- a/mod/resource/locallib.php
          +++ b/mod/resource/locallib.php
          @@ -371,7 +371,7 @@ function resource_get_final_display_type($resource) {
                                        'application/x-shockwave-flash', 'video/x-flv', 'video/x-ms-wm', // video formats
                                        'video/quicktime', 'video/mpeg', 'video/mp4',
                                        'audio/mp3', 'audio/x-realaudio-plugin', 'x-realaudio-plugin',   // audio formats
          -                             'application/pdf', 'text/html',
          +                             'text/html',
                                       );
           
               if (empty($resource->mainfile)) {
          

          In other words, remove 'application/pdf' from the 'embed' array

          Show
          Chris Fryer added a comment - Henning's workaround is only applicable to Moodle 2.3. If you want to backport that workaround to Moodle 2.2, the following patch will work: diff --git a/mod/resource/locallib.php b/mod/resource/locallib.php index 41b77cc..7aea0d9 100644 --- a/mod/resource/locallib.php +++ b/mod/resource/locallib.php @@ -371,7 +371,7 @@ function resource_get_final_display_type($resource) { 'application/x-shockwave-flash', 'video/x-flv', 'video/x-ms-wm', // video formats 'video/quicktime', 'video/mpeg', 'video/mp4', 'audio/mp3', 'audio/x-realaudio-plugin', 'x-realaudio-plugin', // audio formats - 'application/pdf', 'text/html', + 'text/html', ); if (empty($resource->mainfile)) { In other words, remove 'application/pdf' from the 'embed' array
          Hide
          Barbara Taylor added a comment -

          Our simple work around was to edit the options on Plugins -> activity modules -> page, scrolled to the "display" section and changed the default to 'open' then Saved changes at the bottom. We then searched the database and changed all 29,000 files from 'automatic' to 'open'. This enables files on computer to open without downloading and for PDF to open on iPad.

          Show
          Barbara Taylor added a comment - Our simple work around was to edit the options on Plugins -> activity modules -> page, scrolled to the "display" section and changed the default to 'open' then Saved changes at the bottom. We then searched the database and changed all 29,000 files from 'automatic' to 'open'. This enables files on computer to open without downloading and for PDF to open on iPad.
          Hide
          Ben Dailey added a comment -

          I made the change that Barbara Taylor suggested but with in the file activity module not the page activity module. This is on Moodle 2.3.2. Also here is the MySQL query I used to display the PDFs that needed updated:
          SELECT `resource`.`id` , `resource`.`course` , `resource`.`name` , `resource`.`display` , `course_modules`.`id` AS 'courseid', `context`.`id` AS 'contextid', `files`.`id` AS 'filesid', `files`.`filename`, `files`.`mimetype`
          FROM `resource`
          JOIN `course_modules` ON `resource`.`course` = `course_modules`.`course`
          AND `resource`.`id` = `course_modules`.`instance`
          JOIN `context` ON `context`.`instanceid`=`course_modules`.`id`
          JOIN `files` ON `files`.`contextid`=`context`.`id`
          AND `files`.`mimetype`='application/pdf'
          WHERE `resource`.`display`=0;

          And the MySQL Query used to update the existing PDFs to Open instead of automatic:
          UPDATE `resource`
          JOIN `course_modules` ON `resource`.`course` = `course_modules`.`course`
          AND `resource`.`id` = `course_modules`.`instance`
          JOIN `context` ON `context`.`instanceid`=`course_modules`.`id`
          JOIN `files` ON `files`.`contextid`=`context`.`id`
          AND `files`.`mimetype`='application/pdf'
          SET `resource`.`display`=5
          WHERE `resource`.`display`=0;

          Hope this helps someone.

          Show
          Ben Dailey added a comment - I made the change that Barbara Taylor suggested but with in the file activity module not the page activity module. This is on Moodle 2.3.2. Also here is the MySQL query I used to display the PDFs that needed updated: SELECT `resource`.`id` , `resource`.`course` , `resource`.`name` , `resource`.`display` , `course_modules`.`id` AS 'courseid', `context`.`id` AS 'contextid', `files`.`id` AS 'filesid', `files`.`filename`, `files`.`mimetype` FROM `resource` JOIN `course_modules` ON `resource`.`course` = `course_modules`.`course` AND `resource`.`id` = `course_modules`.`instance` JOIN `context` ON `context`.`instanceid`=`course_modules`.`id` JOIN `files` ON `files`.`contextid`=`context`.`id` AND `files`.`mimetype`='application/pdf' WHERE `resource`.`display`=0; And the MySQL Query used to update the existing PDFs to Open instead of automatic: UPDATE `resource` JOIN `course_modules` ON `resource`.`course` = `course_modules`.`course` AND `resource`.`id` = `course_modules`.`instance` JOIN `context` ON `context`.`instanceid`=`course_modules`.`id` JOIN `files` ON `files`.`contextid`=`context`.`id` AND `files`.`mimetype`='application/pdf' SET `resource`.`display`=5 WHERE `resource`.`display`=0; Hope this helps someone.
          Hide
          Howard Miller added a comment -

          This is still a problem in 2.3.

          IMO the simplest thing would be to recognise that a mobile device is on the other end and always display a download link rather than trying to do something clever(er).

          Show
          Howard Miller added a comment - This is still a problem in 2.3. IMO the simplest thing would be to recognise that a mobile device is on the other end and always display a download link rather than trying to do something clever(er).
          Hide
          Howard Miller added a comment -

          Also, making this major as it really is quite a big problem (and increasingly so) for sites with lots of PDFs.

          Show
          Howard Miller added a comment - Also, making this major as it really is quite a big problem (and increasingly so) for sites with lots of PDFs.
          Hide
          Didier Raboud added a comment -

          Actually, there are two possible patches:

          1) Make sure all "automatic" PDFs are not embedded anymore but are force-downloaded:

          --- a/mod/resource/locallib.php
          +++ b/mod/resource/locallib.php
          @@ -425,10 +425,10 @@ function resource_get_final_display_type($resource) {
                   $mimetype = mimeinfo('type', $resource->mainfile);
               }
           
          -    if (file_mimetype_in_typegroup($mimetype, 'archive')) {
          +    if (file_mimetype_in_typegroup($mimetype, array('archive','.pdf'))) {
                   return RESOURCELIB_DISPLAY_DOWNLOAD;
               }
          -    if (file_mimetype_in_typegroup($mimetype, array('web_image', '.pdf', '.htm', 'web_video', 'web_audio'))) {
          +    if (file_mimetype_in_typegroup($mimetype, array('web_image', '.htm', 'web_video', 'web_audio'))) {
                   return RESOURCELIB_DISPLAY_EMBED;
               }
          

          2) For non-automatic PDFs, ignore the setting and force-download:

          --- a/mod/resource/locallib.php
          +++ b/mod/resource/locallib.php
          @@ -415,7 +415,7 @@ function resource_print_filenotfound($resource, $cm, $course) {
           function resource_get_final_display_type($resource) {
               global $CFG, $PAGE;
           
          -    if ($resource->display != RESOURCELIB_DISPLAY_AUTO) {
          +    if ($resource->display != RESOURCELIB_DISPLAY_AUTO and !file_mimetype_in_typegroup($mimetype,'.pdf')) {
                   return $resource->display;
               }
          

          It "just works" here. PDFs should IMHO never be embedded, so this behavioural change doesn't respect the user choice but does a better one than him

          Show
          Didier Raboud added a comment - Actually, there are two possible patches: 1) Make sure all "automatic" PDFs are not embedded anymore but are force-downloaded: --- a/mod/resource/locallib.php +++ b/mod/resource/locallib.php @@ -425,10 +425,10 @@ function resource_get_final_display_type($resource) { $mimetype = mimeinfo('type', $resource->mainfile); } - if (file_mimetype_in_typegroup($mimetype, 'archive')) { + if (file_mimetype_in_typegroup($mimetype, array('archive','.pdf'))) { return RESOURCELIB_DISPLAY_DOWNLOAD; } - if (file_mimetype_in_typegroup($mimetype, array('web_image', '.pdf', '.htm', 'web_video', 'web_audio'))) { + if (file_mimetype_in_typegroup($mimetype, array('web_image', '.htm', 'web_video', 'web_audio'))) { return RESOURCELIB_DISPLAY_EMBED; } 2) For non-automatic PDFs, ignore the setting and force-download: --- a/mod/resource/locallib.php +++ b/mod/resource/locallib.php @@ -415,7 +415,7 @@ function resource_print_filenotfound($resource, $cm, $course) { function resource_get_final_display_type($resource) { global $CFG, $PAGE; - if ($resource->display != RESOURCELIB_DISPLAY_AUTO) { + if ($resource->display != RESOURCELIB_DISPLAY_AUTO and !file_mimetype_in_typegroup($mimetype,'.pdf')) { return $resource->display; } It "just works" here. PDFs should IMHO never be embedded, so this behavioural change doesn't respect the user choice but does a better one than him
          Hide
          Lisa Kidder added a comment -

          +1 for Howard Miller's suggestion. I personally do like when I'm forced to download a file, so the embed is a viable option to have the best of both worlds (unless you have an iOS device).

          It would be nice, if for the embed option, it automatically displayed a download link or had an option to display the download link - similar to the display resource description option.

          Show
          Lisa Kidder added a comment - +1 for Howard Miller's suggestion. I personally do like when I'm forced to download a file, so the embed is a viable option to have the best of both worlds (unless you have an iOS device). It would be nice, if for the embed option, it automatically displayed a download link or had an option to display the download link - similar to the display resource description option.
          Hide
          Kyle Temkin added a comment - - edited

          How about something like this? I wrote this (quickly) for use at my home institution. This adds a setting that, when enabled, forces download when a mobile/tablet device is detected:

          http://source.bumoodle.com/moodle/commit/d82c72d3a70ac006b09cf9d1ac15b0975f5a0ea6

          The setting name and language strings need tweaking- but that's enough to make the point.

          Show
          Kyle Temkin added a comment - - edited How about something like this? I wrote this (quickly) for use at my home institution. This adds a setting that, when enabled, forces download when a mobile/tablet device is detected: http://source.bumoodle.com/moodle/commit/d82c72d3a70ac006b09cf9d1ac15b0975f5a0ea6 The setting name and language strings need tweaking- but that's enough to make the point.
          Hide
          Howard Miller added a comment -

          Still an issue in 2.4 (probably 2.5 as well)

          Show
          Howard Miller added a comment - Still an issue in 2.4 (probably 2.5 as well)
          Hide
          Adam Barbary added a comment -

          Yes, still an issue in 2.5.1

          Show
          Adam Barbary added a comment - Yes, still an issue in 2.5.1
          Hide
          Mei Ling added a comment -

          It can't be solve?

          Show
          Mei Ling added a comment - It can't be solve?
          Hide
          Howard Miller added a comment -

          I think the issue is not that it can't be solved, more that it's difficult to agree on the best solution. Most of the proposals are actually trivial from a coding point of view. Something like Kyle's solution is way better than it is now.

          This is compounded by there being no default option for drag and drop. If you have to support iOS devices then one needs to drag and drop the PDF and then go and edit it. To make matters worse, there are now required fields to complete that would not have been necessary if just using drag and drop. The end result is that teachers with lots of PDFs get very frustrated.

          Show
          Howard Miller added a comment - I think the issue is not that it can't be solved, more that it's difficult to agree on the best solution. Most of the proposals are actually trivial from a coding point of view. Something like Kyle's solution is way better than it is now. This is compounded by there being no default option for drag and drop. If you have to support iOS devices then one needs to drag and drop the PDF and then go and edit it. To make matters worse, there are now required fields to complete that would not have been necessary if just using drag and drop. The end result is that teachers with lots of PDFs get very frustrated.
          Hide
          Michael de Raadt added a comment -

          Giving this a nudge.

          Show
          Michael de Raadt added a comment - Giving this a nudge.
          Hide
          Howard Miller added a comment -

          Thanks for raising the profile of this bug. My opinion is now that PDFs should never be embedded by default. The current default breaks the drag-and-drop processes. If you drag and drop a PDF onto a course page it is guaranteed to be broken in iOS. This is a big inconvenience for users. If you want embedded PDFs (and I'm struggling to see why you would) then it could be a selectable (non-default) option.

          Show
          Howard Miller added a comment - Thanks for raising the profile of this bug. My opinion is now that PDFs should never be embedded by default. The current default breaks the drag-and-drop processes. If you drag and drop a PDF onto a course page it is guaranteed to be broken in iOS. This is a big inconvenience for users. If you want embedded PDFs (and I'm struggling to see why you would) then it could be a selectable (non-default) option.
          Hide
          Henning Bostelmann added a comment -

          I agree with Howard. In view of drag-and-drop file uploads, it is best to set the "automatic" default behaviour for PDF files to "Open" rather than "Embed". Anyone who wants the "Embed" behaviour can of course choose this explicitly, either individually for an activity or by changing the default in the global settings for file resources.

          This issue has been sitting here for far too long. I will put a patch for 2.5 up for peer review; we have been using this (and similar patches for earlier versions) for a long time in our local Moodle.

          Show
          Henning Bostelmann added a comment - I agree with Howard. In view of drag-and-drop file uploads, it is best to set the "automatic" default behaviour for PDF files to "Open" rather than "Embed". Anyone who wants the "Embed" behaviour can of course choose this explicitly, either individually for an activity or by changing the default in the global settings for file resources. This issue has been sitting here for far too long. I will put a patch for 2.5 up for peer review; we have been using this (and similar patches for earlier versions) for a long time in our local Moodle.
          Hide
          Henning Bostelmann added a comment -

          Requesting peer review for this patch. It's a very small code change, but involves some change in functionality.

          The present patch is for master only. I'll be happy to supply backports for Moodle 2.3-2.5 if they can be integrated.

          Show
          Henning Bostelmann added a comment - Requesting peer review for this patch. It's a very small code change, but involves some change in functionality. The present patch is for master only. I'll be happy to supply backports for Moodle 2.3-2.5 if they can be integrated.
          Hide
          Kyle Temkin added a comment -

          After taking a look at your patch, I'm not convinced this is the best way to handle this-- the selection of "what to embed" is still hardcoded. As a quick idea, why not add three configuration options, as follows:

          • File types to embed, which would specify a comma or newline separated list of file mimetypes which should be embedded when the viewer is using a non-mobile configuration.
          • File types to embed (tablet), which would specify a list of file types which should be embedded when device_type == tablet.
          • File types to embed (mobile), which would specify a list of file types which should be embedded when device_type == mobile.

          Those values could be parsed to yield the array affected by Henning's patch above. My earlier "proof of concept" patch should provide a rough outline of how to do this:

          https://github.com/bumoodle/moodle/commit/d82c72d3a70ac006b09cf9d1ac15b0975f5a0ea6

          This would allow those who want to keep the old behavior to do so, without incurring the bug for mobile/tablet users.

          Show
          Kyle Temkin added a comment - After taking a look at your patch, I'm not convinced this is the best way to handle this-- the selection of "what to embed" is still hardcoded. As a quick idea, why not add three configuration options, as follows: File types to embed , which would specify a comma or newline separated list of file mimetypes which should be embedded when the viewer is using a non-mobile configuration. File types to embed (tablet) , which would specify a list of file types which should be embedded when device_type == tablet. File types to embed (mobile) , which would specify a list of file types which should be embedded when device_type == mobile. Those values could be parsed to yield the array affected by Henning's patch above. My earlier "proof of concept" patch should provide a rough outline of how to do this: https://github.com/bumoodle/moodle/commit/d82c72d3a70ac006b09cf9d1ac15b0975f5a0ea6 This would allow those who want to keep the old behavior to do so, without incurring the bug for mobile/tablet users.
          Hide
          Henning Bostelmann added a comment -

          Kyle, I don't quite agree, because the problem is not inherent to tablets or mobile devices; that's only where the problem becomes most apparent. The root cause is browser dependence (or even worse, dependence on certain browser plugins).

          To illustrate: When I tested this on our classroom PCs in 2011 (take this as a "random sample" of a typical browser configuration), embedding PDFs worked in IE (to some extent) but not in Firefox. When I retested this year, it worked in Firefox but not in IE. Mobile browsers seem even more unpredictable. There's just no generally accepted/followed standard on the feature "PDF embedding" across browsers.

          In my opinion, Moodle's "Automatic" setting should choose a way of displaying the file that works consistently and reliably on a wide range of browsers. Clearly "Embed" does not have this property, while "Open" is a better choice.

          It might make sense to have a global "File types to embed" configuration option, independent of the device. On the other hand, there's already the option to create all new file resources with "Embed" instead of "Automatic" (resource_display, see Site Administration -> Plugins -> Activity modules -> File -> Display).

          Show
          Henning Bostelmann added a comment - Kyle, I don't quite agree, because the problem is not inherent to tablets or mobile devices; that's only where the problem becomes most apparent. The root cause is browser dependence (or even worse, dependence on certain browser plugins). To illustrate: When I tested this on our classroom PCs in 2011 (take this as a "random sample" of a typical browser configuration), embedding PDFs worked in IE (to some extent) but not in Firefox. When I retested this year, it worked in Firefox but not in IE. Mobile browsers seem even more unpredictable. There's just no generally accepted/followed standard on the feature "PDF embedding" across browsers. In my opinion, Moodle's "Automatic" setting should choose a way of displaying the file that works consistently and reliably on a wide range of browsers. Clearly "Embed" does not have this property, while "Open" is a better choice. It might make sense to have a global "File types to embed" configuration option, independent of the device. On the other hand, there's already the option to create all new file resources with "Embed" instead of "Automatic" (resource_display, see Site Administration -> Plugins -> Activity modules -> File -> Display).
          Hide
          Dan Poltawski added a comment -

          Hi Henning,

          Thanks for the patch.

          I agree with your assessment of the situation and the code change. I'm sending this for integraiton review.

          Show
          Dan Poltawski added a comment - Hi Henning, Thanks for the patch. I agree with your assessment of the situation and the code change. I'm sending this for integraiton review.
          Hide
          Dan Poltawski added a comment -

          Please could you provide patches for 25 and 24 too?

          Show
          Dan Poltawski added a comment - Please could you provide patches for 25 and 24 too?
          Hide
          Henning Bostelmann added a comment -

          Thanks Dan. Branch is now rebased and backports provided.

          Show
          Henning Bostelmann added a comment - Thanks Dan. Branch is now rebased and backports provided.
          Hide
          Sam Hemelryk added a comment -

          Thanks Henning this has been integrated now. Nice assement of the situation.

          Show
          Sam Hemelryk added a comment - Thanks Henning this has been integrated now. Nice assement of the situation.
          Hide
          Jason Fowler added a comment -

          Sweet! It works perfectly, thanks Henning

          Show
          Jason Fowler added a comment - Sweet! It works perfectly, thanks Henning
          Hide
          Sam Hemelryk added a comment -

          Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow.
          The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut.

          Thanks for the effort everyone, another successful weekly release has been rolled.
          Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow. The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut. Thanks for the effort everyone, another successful weekly release has been rolled. Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP. Many thanks Sam
          Hide
          Timothy Allen added a comment -

          Hi, thanks a lot for the work on this. We have had problems with this issue too, with quite a lot of complaints. Just wanted to ask whether this fix will be back ported to Moodle 2.3? It only mentions 2.4/2.5 as the fix versions, but we are on 2.3 and need this badly too.

          Show
          Timothy Allen added a comment - Hi, thanks a lot for the work on this. We have had problems with this issue too, with quite a lot of complaints. Just wanted to ask whether this fix will be back ported to Moodle 2.3? It only mentions 2.4/2.5 as the fix versions, but we are on 2.3 and need this badly too.
          Hide
          Henning Bostelmann added a comment -

          I can provide a backport to 2.3 (it's easy enough), but I'm not sure whether it can still be integrated as Moodle 2.3 must be very close to the end of its lifecycle. (Dan would know more I guess.)

          I'm not sure how you mange your local installation - would you be happy to "cherry-pick" a patch into your local installation even if it's not integrated into the upstream branch?

          Show
          Henning Bostelmann added a comment - I can provide a backport to 2.3 (it's easy enough), but I'm not sure whether it can still be integrated as Moodle 2.3 must be very close to the end of its lifecycle. (Dan would know more I guess.) I'm not sure how you mange your local installation - would you be happy to "cherry-pick" a patch into your local installation even if it's not integrated into the upstream branch?
          Hide
          Timothy Allen added a comment -

          Hi Henning, I have followed up with our support regarding this and they are planning to move to Moodle 2.5 in 2 months, and in the meantime they have a code freeze on. It looks as though we will just have to wait for the 2.5 upgrade, so 2.3 code is no longer necessary for us. Thanks anyway for your offer and all your work on this.

          Show
          Timothy Allen added a comment - Hi Henning, I have followed up with our support regarding this and they are planning to move to Moodle 2.5 in 2 months, and in the meantime they have a code freeze on. It looks as though we will just have to wait for the 2.5 upgrade, so 2.3 code is no longer necessary for us. Thanks anyway for your offer and all your work on this.

            People

            • Votes:
              60 Vote for this issue
              Watchers:
              56 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: