search

nextcloud / files_pdfviewer

:book: A PDF viewer for Nextcloud
GNU Affero General Public License v3.0
90 stars 43 forks source link

PDF Viewer in Nextcloud 25 not working with QtWebEngine 5.15 LTS (Chromium 87 based) - please use PDF.js legacy build #684

Open kkofler opened 1 year ago

kkofler commented 1 year ago

Steps to reproduce

  1. Open up-to-date versions of Falkon and Qt 5 QtWebEngine.
  2. Log into Nextcloud. That works fine.
  3. Try to view a PDF. That does not. It worked in Nextcloud 24.

Expected behaviour

The PDF is displayed.

Actual behaviour

Blank space is displayed instead of the PDF viewer, and I get errors in the JavaScript console.

Additional information

The issue is that the PDF.js version was upgraded from 2.13.216 to 2.15.349, which introduces new JavaScript constructs not supported in Chromium 87, and that you use the default build of PDF.js, which supports only the very latest browsers, instead of the legacy build, which is much more compatible (see https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support). QtWebEngine 5.15 LTS is a stable branch with backported security fixes (comparable to the Firefox ESR branches).

I was able to quickly work around the issue by downgrading PDF.js to 2.13.216 on the server I administer, but I think using the legacy build would be a better long-term fix. I have not tried that yet, because I am not sure what exactly I need to change. (pdfjs-dist ships both the default and the legacy build, the latter in a subdirectory.)

kkofler commented 1 year ago

I suspect that using the legacy build would also fix #674 (essentially the same issue, but with Safari, which was already not working with Nextcloud 24).

skjnldsv commented 1 year ago

Hum, we do not support chrom(ium) 87. Legacy is not being tested by the devs at Mozilla's, so I'm honestly not feeling risking this and seeing more people complain about missing or not working feature.

kkofler commented 1 year ago

According to: https://docs.nextcloud.com/server/latest/user_manual/en/webinterface.html the latest version of the browsers is recommended, but not required. (There is explicitly a note stating that Nextcloud Talk requires at least Chromium 49+, i.e., much earlier than 87.) Nextcloud 25 works fine (except for the PDF viewer) on QtWebEngine 5.15 LTS. PDF.js non-legacy has a hard requirement on the latest versions.

In addition, according to the same link, Nextcloud explicitly supports Safari and Edge which are also supported by PDF.js only in the legacy build.

And surely some missing or not working feature is a better experience than the viewer coming up completely blank as it does now for a non-negligible amount of users.

kkofler commented 1 year ago

(In fact, IMHO, PDF.js legacy is already very demanding for the minimum browser version, but acceptable. PDF.js modern supporting only the very latest&greatest, I personally consider absolutely unacceptable.)

When I try the demo links from Mozilla, which I assume use the very latest PDF.js (i.e., even newer than what Nextcloud 25 now uses):

the first link displays the same blank page on my browser that Nextcloud 25 does, whereas the second link works without any issues.

skjnldsv commented 1 year ago

Yep, I updated the documentation. https://github.com/nextcloud/documentation/pull/9408

skjnldsv commented 1 year ago

And surely some missing or not working feature is a better experience than the viewer coming up completely blank as it does now for a non-negligible amount of users.

Not if this is broken. It's a dev versus user point of vue. This repo have seen plenty of issues already. PDFs are hard to maintain across browsers

kkofler commented 1 year ago

Even with your change, you still document Safari and Edge as supported, and those also require the legacy version of PDF.js. The default/modern version supports only Firefox and Chrome! (And only the very latest version.)

And additionally there is a huge difference in practice between "For the best experience with the Nextcloud web interface, we recommend" (but in practice, older/LTS browsers mostly or even completely work) and PDF.js just displaying nothing with a JavaScript syntax error.

kkofler commented 1 year ago

(See also #674 for Safari.)

kkofler commented 1 year ago

In practice, your browser list for Nextcloud includes Safari 13, which uses a WebKit version approximately one year older than Chromium 87, so it is not a big surprise that Nextcloud does not fail with JavaScript syntax errors on QtWebEngine 5.15. Except for the PDF viewer which (according to #674) is also broken at least on Safari 14 (and possibly even on later versions too considering that PDF.js default/modern does not support Safari at all, no matter the version).

It would be great if QtWebEngine LTS were part of your testing, but it does not seem to be needed to work in practice. PDF.js is the one painpoint.

kkofler commented 1 year ago

Note that QtWebEngine 5.15 LTS is publicly available under the LGPL and can be built against any Qt 5.15, so the latest LTS version with the latest security backports can be used just fine with Qt Open Source. (That is what I am using.)

kkofler commented 1 year ago

Your browser list (the one at browserlists.dev to which you linked) also includes UC Browser 13.4 which is based on Chromium 78 (i.e., 9 versions older than QtWebEngine 5.15 LTS).

kkofler commented 1 year ago

Switching to legacy PDF.js is actually as easy as:

  1. download the tarball of the correct version of files_pdfviewer
  2. unpack it
  3. sed -i -e 's/-dist\.zip/-legacy-dist.zip/g' pdfjs-get.js
  4. npm update
  5. npm run build
  6. node pdfjs-get.js (because for some reason the above deletes the js/pdfjs directory)
  7. tar up the js directory and replace the one on the server (/var/www/nextcloud/apps/files_pdfviewer/js/) with it

I can confirm that this works just fine, so I no longer have to downgrade PDF.js to an older release, the legacy build of 2.15.349 works fine.

Please consider making this trivial change (see step 3) in the released Nextcloud application. It will greatly improve browser compatibility at practically no cost to you.

kkofler commented 1 year ago

Or alternatively, how much work would it be for you to simply ship both and let the local server admin choose through a UI checkbox which version to show to users (or even the individual user)? I would say it is not worth it because it is much easier to just ship legacy only, which will work everywhere, but if you are uncomfortable with that, shipping both could be a solution.

skjnldsv commented 1 year ago

Please stop spamming. Seeing 7 pings from the same user in one afternoon will make me ignore the ticket :cry:

Or alternatively, how much work would it be for you to simply ship both and let the local server admin choose through a UI checkbox which version to show to users

This will never happen. This is counter intuitive. you're looking to fix an issue that have minimal impact and applies to a handful of users (compared to millions)

Even with your change, you still document Safari and Edge as supported, and those also require the legacy version of PDF.js. The default/modern version supports only Firefox and Chrome! (And only the very latest version.)

This, I think, is the only point where it make sense to use the legacy build imho

r-hmn commented 1 year ago

The other new submitted bugs are on the same topic I think this needs fixing.. not being able to read PDF's online is a major hindrance.

kkofler commented 1 year ago

This script can be used by the local admin as a workaround. (It can be run remotely, and it needs npm and some other tools on the host on which it is run. And it needs to be rerun each time you upgrade Nextcloud.)

#!/bin/bash
SERVER=root@foo.example.com
if [ -z "$1" ] ; then
  echo "usage: ./fix-nextcloud-pdfviewer.sh VERSION"
  exit 1
fi
if [ ! -f files_pdfviewer-$1.tar.gz ] ; then
  wget https://github.com/nextcloud/files_pdfviewer/archive/refs/tags/v$1.tar.gz -O files_pdfviewer-$1.tar.gz || exit $?
fi
rm -rf files_pdfviewer-$1 files_pdfviewer-$1-pdfjs-legacy
tar xzf files_pdfviewer-$1.tar.gz || exit $?
cp -a files_pdfviewer-$1 files_pdfviewer-$1-pdfjs-legacy || exit $?
cd files_pdfviewer-$1-pdfjs-legacy
sed -i -e 's/-dist\.zip/-legacy-dist.zip/g' pdfjs-get.js || exit $?
npm update || exit $?
npm run build || exit $?
node pdfjs-get.js || exit $?
tar cJf js.tar.xz js pdfjs-get.js || exit $?
scp js.tar.xz $SERVER:/var/www/nextcloud/apps/files_pdfviewer/ || exit $?
ssh $SERVER "cd /var/www/nextcloud/apps/files_pdfviewer/ && tar xJf js.tar.xz && chown -R www-data:www-data js pdfjs-get.js && rm -f js.tar.xz" || exit $?
cd ..
exit 0
kkofler commented 1 year ago

FYI, the last PDF.js that supports QtWebEngine 5.15 LTS even in legacy mode is 3.5.141, so for Nextcloud 27.0.1, PDF.js also has to be downgraded:

sed -i -e 's/"pdfjs-dist": "\^3\.6\.[0-9]\+"/"pdfjs-dist": "^3.5.141"/g' package.json || exit $?

(I added this after my other sed in the above script.)

QtWebEngine 5.15 LTS is based on Chromium 87. PDF.js 3.6.172, which is used in Nextcloud 27.0.1 / files_pdfviewer 2.8.0, bumps the minimum Chromium version to 88 (!). The result is that there are several display issues in QtWebEngine 5.15 LTS due to the use of unsupported CSS features. (The latest upstream, 3.8.162, bumps it further to 92. The result in QtWebEngine 5.15 LTS is a completely blank rendering with 0 pages due to JavaScript errors, same as in the non-legacy version, just slower to load.)