-
Notifications
You must be signed in to change notification settings - Fork 10k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a color-scheme option to pdf.js #13676
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into issue #2071.
Based on a quick look, I've got some reservations about the implementation in this PR:
- First of all you should not be changing the UI of the viewer itself it in this way, since there's already a preference
viewerCssTheme
used for that purpose and the toolbar buttons shouldn't be modifying the viewer itself (only the actual PDF document). - Secondly, looking at the actual color-inversion part of the actual PDF document, I'm a bit worried about the maintainability of all of this new code (since it's e.g. completely untested) and also about any possible performance impact.
Can we not simplify all of this considerably, by using a CSS-only approach (there's some discussion about that in the issue) to just invert the colours after the fact and thus remove a lot of overall complexity?
As a new-user I would "kind of" expect if I select such option to have the whole UI change with it. But no hassle to remove that bit of code.
Luckily that code is only used for images as they somehow will need to be changed to - I actually haven't really got much of testing with that code and the images as I only could find a 1BIT_Images.pdf which is working as intended with that code. Also it's used for the So that part is pure to satisfy images-like and graphs on documents.
I've looked into it, and I'm not really satisfied by the result, especially because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a new-user I would "kind of" expect if I select such option to have the whole UI change with it. But no hassle to remove that bit of code.
Sorry, but anything related to the existing viewerCssTheme
handling would need to be removed.
Luckily that code is only used for images as they somehow will need to be changed to - I actually haven't really got much of testing with that code and the images as I only could find a 1BIT_Images.pdf which is working as intended with that code.
There's a lot of existing test-cases available in https://github.com/mozilla/pdf.js/tree/master/test/pdfs, and https://github.com/mozilla/pdf.js/wiki/Contributing explains how to run tests (doing so will also download all the linked test-cases as well).
When I spoke about the maintainability/complexity, I didn't mean just the colour conversion code added in this PR but mostly the overall changes being made in the src/display/canvas.js
file.
Also it's used for the setStrokeRGBColor and setFillRGBColor this is mainly to satisfy the "graphs" on the default loaded .pdf as they set a white color as background and on top of that paint the graph, which will look worse when painted without any color inversion.
Is there also not a risk that all the added checks, in various (potentially hot) parts of the src/display/canvas.js
code could have a performance impact for all documents even in the "normal" colour mode?
I've looked into it, and I'm not really satisfied by the result,
Maybe a CSS-only approach isn't perfect, but would it not be "good enough" for many cases!?
No problems, fixed.
for "normal" colour mode, it shouldn't be a problem, modern and even older browsers are very fast at checking properties for truthiness. For when "dark" colour mode is enabled, it will yes iterate over the images chunks and over the pixels, that is the only "big" hassle I could see when bench marking, the
If the PDF is simply a text-only. With some title's. Yeah it would be a suit-able? Other then the rendering of purple-ish sub-pixels. But if even a small images or graphs comes into place, I wouldn't even remotely recommending it. As for the test-cases, it seems like |
I've had some success with the |
fd22262 removes the modifying images code. Thus results to less more code being changed in |
4e04a74 makes the color_utils.js more minimal and combined with the previous commit removes around ~200 LOC with having minimal impact on the PR. |
Apart from the readability/maintainability aspect of all this (untested) code being sprinkled throughout the Again, for all of the reasons already mentioned, a CSS-only solution definitely seem to be the way to go here. /cc @timvandermeij What's your opinion on this patch? |
I could check for the current
I get that, I don't like it either.
This is going to be a agree to disagree situation on that. |
I agree with @Snuffleupagus that this is quite a lot of code for something that should be much simpler to address with CSS. It might not give perfect results, but to be honest looking at https://user-images.githubusercontent.com/25481501/124488836-d8b1c680-dd9f-11eb-9241-3b189003b24f.png doesn't quite look good to me as well. I find the text quite hard to read because because of the color change; the letters aren't "crisp" anymore and look like they consist of multiple colors of pixels instead of being white. Actually, I think the images from the simple CSS invert solution in #2071 look much better to me. If I look at https://user-images.githubusercontent.com/37630203/116760785-7a3b3580-aa48-11eb-9051-443a33d9402a.png the text is much more "crisp" and evenly colored, which makes me wonder why the CSS solution wouldn't just suffice here?! |
Honestly, I've worked quite hard on this system that now the PR is, took me a good couple of days. And the CSS Filter doesn't really spark interest in me. Because any issues raised with them will be way way harder to fix. Because such filter affect every element at once and not individually. Thereby this is just an opinion that the filter is way too dark, but that can be fixed, to be more inline with the dark cssViewerTheme. About the "crisp", my current setup with fonts and Firefox isn't the best and shouldn't be used for reference, the "broken" crispness is to fix some issue with my current monitor. Il get this PR changed a CSS alternative, thank you all for the reviewing of the code, I really do appreciate it. |
So, 1 of the downside of filter, any colors, won't have the original "color", we mainly resolve that with the So it's either no crisp but have incorrect-coloring. |
I'm a little bit disappointed that @Gusted's work isn't getting attention anymore. He was clearly listening to your opinions and putting work in. Both of the images above look good enough to me, either option is so much better than nothing. Many people would really appreciate to have either implementation. |
Isn't there a better way to implement this to take the PDF internally and swap the default colors so that the background and foreground text colors are different? This way pictures would look normal. |
Ah well, that was my first thought too(see earlier comments) and it was the implementation I had created at first. However the pdf.js team disagrees with this and prefers to use If the pdf.js team still wants to use this CSS implementation rather than the modifying color(which is still in the commit history) then I will be closing the PR as I don't like nor want that kind of implementation be the way it should be, it's poor and not the correct way to do this. I don't want my name signed off on this feature with such a poor implementation. |
Why dark-theme is not completely black on the background? |
Pure black isn't natural to look at, it's only interesting for people with a amoled screen and most of those are phone users anyway. Looking at a lighter shade of black makes it more natural to look at than pure black. |
I believe pure-black is healtier for the eye and easier to detect white fonts. If you try https://darkreader.org to read pdf, it does uses pure black on the end. |
I made an add-on for the viewer that does this (and a bit more). Live demo here (color scheme can be changed from the toolbar). It employs the same tactic as @Gusted's original patch, of modifying the Canvas drawing on the go. However instead of patching the PDF.js source I wrote it as an external script. Clearly there is a performance impact (probably more than Gusted's). But as Gusted said the CSS-filter method is suboptimal experience-wise (my add-on includes it only as a fallback option). It's not pretty, a bit too fancy (what it does) and, as @Snuffleupagus said, a lot of complexity for the maintainers. So I didn't imagine it getting merged anyway, and that's where stuff like this would actually be ideal as add-ons. It's really unfortunate Firefox add-ons don't work on the built-in viewer. I guess I might be able put together a custom viewer with my add-on on top and make it into a Firefox extension, and make it the default PDF viewer.. but then it'd be unnecessary bloat, and also elicit the refrain oft-repeated here:
Can we have the maintainers' take on this? I hope I am clear that my only intention is to extend the default viewer, not to "monkey patch" it. This seems to be the only way, at least till PDF.js natively supports add-ons. It does what I want (view PDFs in eye-friendly colors), and I don't mind the performance hit most of the time. It seems a lot of users desire the same and IMHO we are being held back by mere technicalities here. |
Let's close this PR for now, given the differing opinions and perspectives regarding how this could be implemented. Please note that #2071 is still open for tracking this issue. |
|
Hi everyone reading this!
This is my first PR for
pdf.js
so a fair warning that I might have done things that are considered not "best practices".This PR is mainly to resolve #2071 as I'm getting tired of whipping out a external PDF Viewer to not eyestrain while reading PDF's.
Having this automatically implemented within the browser(Firefox) is a feature I really loved to see, and thereby currently extensions like Dark Reader(DISCLAIMER: I'm the maintainer) cannot access the pdf files on Firefox, so it was basically my only option to see such feature in Firefox.
I'm really looking towards fixing issues being raised and answering how this whole thing actually works and of course the moment that's being merged ;).
Preview:
Regards,
Gusted