Feature #22483

Show PDF attachments and repo entries instead of downloading them

Added by Gregor Schmidt over 1 year ago. Updated over 1 year ago.

Status:Needs feedbackStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Attachments
Target version:-
Resolution:

Description

Following up on #22058, I would like to propose, that also PDF files are shown directly instead of forcing downloads.

This applies to two use cases:

1. Following a download link

When following a download link, image are sent with content-disposition: inline, while all other files are sent with attachment. Since the some browsers is able to render PDF files directly (Firefox, Chrome, Safari, IE with Adobe Reader installed) and people are probably used to it, I think that extending this exception to PDF files is a reasonable change. If native PDF rendering within the browser is not supported, then a download will be initiated by the browser anyway.

2. Displaying a PDF file in repository browser

When accessing "View" within the file view in the repository browsers, Redmine currently forces a download of the PDF. Instead an iframe could be rendered which displays the PDF within the Redmine UI. Browser support for this use case is similar to No. 1.


The patch series is based on current trunk (r15332) plus the changes of #22482. If desired, I could isolate the changes further, to make them work without #22482.

0004-Don-t-force-download-of-PDFs-but-show-in-browser-pre.patch

The change updates AttachmentsController and RepositoryController to send PDF files with content-disposition: inline. (See first use case.)

0005-Show-pdf-preview-for-repository-files-and-attachment.patch

The change adds attachments/pdf and common/_pdf views to render PDF files in an iframe/object construct. (See second use case.)

Since we cannot easily know the PDF’s dimensions, the frame size follows the following rules:

  • it is as wide as possible (100%)
  • in general it is as high as it is wide
  • it is never higher than 80 % of the viewport (80vh), since we want to make sure, that there is always a bit of Redmine UI visible to avoid getting “trapped” within the frame

Using a nested object/iframe structure was taken from http://pdfobject.com/static.html

pdf-preview.png (54.7 KB) Gregor Schmidt, 2016-04-13 14:51

0001-Don-t-force-download-of-PDFs-but-show-in-browser-pre.patch Magnifier (3.06 KB) Gregor Schmidt, 2016-05-09 11:08

0002-Show-pdf-preview-for-repository-files-and-attachment.patch Magnifier (33.7 KB) Gregor Schmidt, 2016-05-09 11:08

safari-error.png (61.5 KB) Go MAEDA, 2016-05-09 11:28

060719210727_example.pdf - text fixture (copy this file to /test/fixtures/files/2006/07 directory) (18.5 KB) Go MAEDA, 2016-05-09 18:00

060719210727_example.pdf (22.9 KB) Gregor Schmidt, 2016-05-09 18:52


Related issues

Duplicated by Redmine - Feature #22098: Open attached pdf files in the browser Closed
Blocked by Redmine - Feature #22482: Respond with "No preview available" instead of sending th... Closed

Associated revisions

Revision 15409
Added by Jean-Philippe Lang over 1 year ago

Don't force download of PDF (#22483).

Patch by Gregor Schmidt.

History

#1 Updated by Gregor Schmidt over 1 year ago

#2 Updated by Go MAEDA over 1 year ago

  • Blocked by Feature #22482: Respond with "No preview available" instead of sending the file when no preview is available added

#3 Updated by Go MAEDA over 1 year ago

@Gregor, could you please provide rebased version of these patches?

I am very interested in this feature but patches cannot be applied to the current trunk.

#5 Updated by Go MAEDA over 1 year ago

Thanks for updating patches.
But in my environemnt, I cannot view PDF in browsers. I tried two web servers, Pow 0.5.0 and Webrick, both failed.

Error:

HTTP Response header:

Content-Type is binary/octet-stream. Perhaps it causes this problem.

HTTP/1.1 200 OK
X-Frame-Options: SAMEORIGIN
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
ETag: "13891eae2aa4115e138138ea507d1fdf" 
Content-Disposition: inline; filename="aaaaaaa-40.pdf" 
Content-Transfer-Encoding: binary
Content-Type: binary/octet-stream
Cache-Control: private
Content-Length: 8331
X-Request-Id: ef5b1ea8-cb19-4cfa-8db2-1aac427b0b1e
X-Runtime: 0.025105
Date: Mon, 09 May 2016 09:17:53 GMT
Connection: keep-alive

#6 Updated by Gregor Schmidt over 1 year ago

I'm sorry, but I cannot reproduce your problems. I've tried Firefox (Mac), Safari (Mac and iOS 9 Simulator) on Webrick, Unicorn and Puma. In any case, the Content-Type was application/pdf, which is determined by the detect_content_type method within the AttachmentsController. Maybe you could try debugging there to find out, what happened.

I have also tested the changes on IE w/o Adobe Reader installed, i.e. it cannot render PDFs within the browser. It didn't give me an error but downloaded the file instead. So I was rather confident, that there should be no negative side effects when sending PDFs with Content-Disposition: inline.

#7 Updated by Go MAEDA over 1 year ago

Thanks for investigating the problem I wrote in #22483#note-5.

I found that no problem in your patches, it is caused by files which are uploaded by Firefox. Attachment#content_type of these files are 'binary/octet-stream', so 'Content-Type' field in response header will be 'binary/octet-stream'. Maybe related to https://bugzilla.mozilla.org/show_bug.cgi?id=373621 .

2.2.4 :004 > Attachment.find(43).content_type
  Attachment Load (0.2ms)  SELECT  "attachments".* FROM "attachments" WHERE "attachments"."id" = ? LIMIT 1  [["id", 43]]
 => "binary/octet-stream" 

The patch works fine with files uploaded by Chrome and Safari. This feature is extremely beneficial for users who handle PDF files frequently.

#8 Updated by Go MAEDA over 1 year ago

Although the patches works fine, but test fails because PDF file "test/fixtures/files/2006/07/060719210727_example.pdf" does not exist.

$ ruby test/functional/attachments_controller_test.rb

(snip)

  1) Failure:
AttachmentsControllerTest#test_show_pdf [test/functional/attachments_controller_test.rb:204]:
Expected response to be a <success>, but was <404>

log/test.log:

----------------------------------------
AttachmentsControllerTest: test_show_pdf
----------------------------------------
Processing by AttachmentsController#show as HTML
  Parameters: {"id"=>"21"}

(snip)

Cannot send attachment, /*****/redmine-trunk/test/fixtures/files/2006/07/060719210727_example.pdf does not exist or is unreadable.

(snip)

Completed 404 Not Found in 26ms (Views: 20.2ms | ActiveRecord: 1.0ms)

To pass tests, copy 060719210727_example.pdf to test/fixtures/files/2006/07/ directory.

#9 Updated by Go MAEDA over 1 year ago

  • Assignee set to Jean-Philippe Lang
  • Target version set to 3.3.0

@Jean-Philippe, could this feature be included in 3.3.0?

I know 3.3.0 is entering feature freeze but I think it would be nice if this feature is delivered along with #22058.

#10 Updated by Gregor Schmidt over 1 year ago

Go MAEDA wrote:

Although the patches works fine, but test fails because PDF file "test/fixtures/files/2006/07/060719210727_example.pdf" does not exist.

Sorry, I forgot to check how git's format-patch handles binary files. It turns out, they are omitted in the output by default.

Here is the example pdf, that I used locally. It's just a "Print as PDF" of http://example.org. As you mentioned, it needs to be placed at test/fixtures/files/2006/07/060719210727_example.pdf

#11 Updated by Gregor Schmidt over 1 year ago

Concerning your problem with the wrong mime type in Firefox:

I am using Firefox as my main browser and also uploaded the files, that I used for testing, using Firefox. Not sure, why you would observe a different behavior.

Anyway, the underlying problem is, that different methods are used to determine the mime type and therefore different results may occur. The disposition method just checks the file ending, the detect_content_type tries to reuse the information passed by the browser. I assume, this should be fixed, so that only a single source is used. Maybe the introduction of the mimemagic gem could provide better facilities for mime handling, than those currently in place in Redmine. But this might be a topic for a separate issue.

#12 Updated by Go MAEDA over 1 year ago

  • File deleted (0004-Don-t-force-download-of-PDFs-but-show-in-browser-pre.patch)

#13 Updated by Go MAEDA over 1 year ago

  • File deleted (0005-Show-pdf-preview-for-repository-files-and-attachment.patch)

#14 Updated by Jean-Philippe Lang over 1 year ago

  • Status changed from New to Needs feedback

Sending PDF inline instead of forcing the download is committed, but I'm not really sure about the preview in an iframe. I find it much less confortable than viewing it using the whole viewport of the browser. I get double scrollbars, one for the body and one for the iframe (I had to lower the height to 70vp but I guess it depens on the header and window height). Also, they're no way to zoom the content of the iframe under iOS (tested with 8) which make the preview useless.

#15 Updated by Jean-Philippe Lang over 1 year ago

  • Target version deleted (3.3.0)

#16 Updated by Go MAEDA over 1 year ago

  • Duplicated by Feature #22098: Open attached pdf files in the browser added

Also available in: Atom PDF