Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

PDFs don't display on foia.state.gov #12992

Closed
jonathansampson opened this issue Feb 2, 2018 · 6 comments
Closed

PDFs don't display on foia.state.gov #12992

jonathansampson opened this issue Feb 2, 2018 · 6 comments

Comments

@jonathansampson
Copy link
Collaborator

Description

In Chrome and Firefox, PDFs open in a lightbox. In Brave, they prompt a download window.

Steps to Reproduce

  1. Navigate to https://foia.state.gov/search/results.aspx?publishedBeginDate=20180201&publishedEndDate=20180201
  2. Click one of the subject titles

Actual result:
A download window is presented.

Expected result:
The PDF is displayed in a lightbox.

Reproduces how often:
Always

Brave Version

0.20.30

about:brave info:

Brave: 0.20.30 
V8: 6.4.388.40 
rev: 8b78402e7b3b91d44d8146cbe4dc8013592c33fd 
Muon: 4.7.9 
OS Release: 10.0.16299 
Update Channel: Release 
OS Architecture: x64 
OS Platform: Microsoft Windows 
Node.js: 7.9.0 
Brave Sync: v1.4.2 
libchromiumcontent: 64.0.3282.119

Reproducible on current live release:
Yes

Additional Information

image

image

@J-Reis
Copy link

J-Reis commented Feb 2, 2018

Can confirm. Same behavior when interacting with media links in Dokuwiki. Firefox-ESR & Chrome both open the pdf in browser.

System

Debian Stable (9.3)
Brave installed via:
deb [arch=amd64] https://s3-us-west-2.amazonaws.com/brave-apt jessie main

Steps to Reproduce

Actual result:

Brave offers to download the pdf file locally

Expected result:

The PDF is displayed inside Brave tab

Reproduces how often:

Always

Brave Version

0.20.30

about:brave info:

Name Version
Brave 0.20.30
V8 6.4.388.40
rev 8b78402
Muon 4.7.9
OS Release 4.9.0-5-amd64
Update Channel Release
OS Architecture x64
OS Platform Linux
Node.js 7.9.0
Brave Sync v1.4.2
libchromiumcontent 64.0.3282.119

@humphd
Copy link

humphd commented Feb 16, 2018

I looked into this a bit today, and it's caused by the PDF handling code only checking for mainFrame vs. subFrame resource types. This fixes it, but isn't perfect because it replaces the entire tab's contents vs. just populating the intended sub frame--I have no idea how you'd know which element to use as the frame:

diff --git a/app/pdfJS.js b/app/pdfJS.js
index 18d9f2081..944a3d548 100644
--- a/app/pdfJS.js
+++ b/app/pdfJS.js
@@ -11,6 +11,10 @@ const settings = require('../js/constants/settings')

 const getViewerUrl = UrlUtil.getPDFViewerUrl

+function isFrame(resourceType) {
+  return resourceType === 'mainFrame' || resourceType === 'subFrame';
+}
+
 /**
  * Check if the request is a PDF file.
  * @param {Object} details First argument of the webRequest.onHeadersReceived
@@ -52,8 +56,8 @@ function isPDFDownloadable (details) {
   // viewer to open the PDF, but first check whether the Content-Disposition
   // header specifies an attachment. This allows sites like Google Drive to
   // operate correctly (#6106).
-  if (details.resourceType === 'mainFrame' &&
-      details.url.indexOf('=download') === -1) {
+  if (isFrame(details.resourceType) &&
+     details.url.indexOf('=download') === -1) {
     return false
   }
   var cdHeader = (details.responseHeaders &&
@@ -78,7 +82,7 @@ function getHeadersWithContentDispositionAttachment (details) {

 const onBeforeRequest = (details) => {
   const result = { resourceName: 'pdfjs' }
-  if (details.resourceType === 'mainFrame' &&
+  if (isFrame(details.resourceType) &&
     UrlUtil.isFileScheme(details.url) &&
     UrlUtil.isFileType(details.url, 'pdf')) {
     appActions.loadURLRequested(details.tabId, getViewerUrl(details.url))
@@ -90,7 +94,7 @@ const onBeforeRequest = (details) => {
 const onHeadersReceived = (details) => {
   const result = { resourceName: 'pdfjs' }
   // Don't intercept POST requests until http://crbug.com/104058 is fixed.
-  if (details.resourceType === 'mainFrame' && details.method === 'GET' && isPDFFile(details)) {
+  if (isFrame(details.resourceType) && details.method === 'GET' && isPDFFile(details)) {
     if (isPDFDownloadable(details)) {
       // Force download by ensuring that Content-Disposition: attachment is set
       result.responseHeaders = getHeadersWithContentDispositionAttachment(details)

@SilverPuppy
Copy link

SilverPuppy commented May 23, 2018

I get the same behavior with Google Calendar and other random PDFs. It has happened for quite awhile; since February sounds reasonable. I assumed it would go away without my bothering anyone, and simply lived with it. This seems to be a widespread problem with certain PDFs. Some open in the tab as expected, and some offer a download instead. I have yet to find a particular characteristic that triggers this behavior.

I will say that Google Calendar puts a script in theirs to trigger the print dialog, which is somewhat unique, but I have seen this on PDFs which did not appear to have any scripting. Google Calendar also previously gave Brave issues, where the Print PDF would throw a new tab with an error message. #5688 It did work correctly for many months after this issue was closed.

Unfortunately it seems that PDFs in Brave have consistent compatibility problems. I will say that I prefer getting a download over an error, but clearly PDF viewing is a work-in-progress.

@humphd
Copy link

humphd commented May 23, 2018

This has been fixed by #13587, it's just waiting to land.

@SilverPuppy
Copy link

When do you expect this to hit release?

I will update this tracker when I confirm resolution.

@bsclifton
Copy link
Member

works great now- thanks for the patch, @humphd 😄

@bsclifton bsclifton removed this from the Triage Backlog milestone Aug 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants