Skip to content
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 new preference, viewerCssTheme, to allow forcing the use of the light/dark viewer CSS themes (issue 12290) #12625

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

While this does work pretty well in my quick testing, it's very much a hack since as far as I can tell there's no support in the CSS specification for using e.g. a CSS variable to override a @media (prefers-color-scheme: dark) {...} block.

The solution implemented here is thus to edit the viewer CSS, by either removing the entire @media ... block in light-mode or by ensuring that its rules become unconditionally applied in dark-mode.
To simplify the overall implementation, since all of this does seem like somewhat of an edge-case, the viewerCssTheme preference will only be read during viewer initialization. (Similar to many other existing preferences, a reload is thus required when changing it.)

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/946bd442c0cdd65/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/946bd442c0cdd65/output.txt

Total script time: 4.16 mins

Published

@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 15, 2020

I might be doing something wrong here, but when running PDFViewerApplication.preferences.set("viewerCssTheme", 2); in the console and reloading the page, I get the following stack trace and the viewer doesn't load:

Uncaught (in promise) Error: Permission denied to access property 0
    _forceCssTheme app.js:425
    initialize app.js:259
    run app.js:593
    webViewerLoad viewer.js:230
    __webpack_modules__ viewer.js:240
    Webpack 3
app.js:425:8

Edit: Huh, this happens only when I reload the page with the console opened. If I run that line of code in the console, close the console and then reload the page is does work :-s

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Nov 15, 2020

I might be doing something wrong here, but when running PDFViewerApplication.preferences.set("viewerCssTheme", 2); in the console and reloading the page, I get the following stack trace and the viewer doesn't load:

That's really weird, and unfortunately I can't reproduce it with Firefox Nightly 84.0a1 (2020-11-15) (64-bit); however I've submitted a patch which hopefully should prevent that particular error.
Does that improve things at all, i.e. is the error gone and does the theme switch as expected now?

Edit: Also, I've pushed an updated version of the patch which wraps all of the styleSheet fetching/parsing in try...catch to at least ensure that any errors won't prevent the entire viewer from loading.

@Snuffleupagus Snuffleupagus force-pushed the viewerCssTheme-option branch 2 times, most recently from 954d38f to 5c00c28 Compare November 15, 2020 23:30
@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/a2663e4aabe9cdf/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/a2663e4aabe9cdf/output.txt

Total script time: 4.17 mins

Published

…he light/dark viewer CSS themes (issue 12290)

While this does work pretty well in my quick testing, it's *very much* a hack since as far as I can tell there's no support in the CSS specification for using e.g. a CSS variable to override a `@media (prefers-color-scheme: dark) {...}` block.

The solution implemented here is thus to *edit* the viewer CSS, by either removing the entire `@media ...` block in light-mode or by ensuring that its rules become *unconditionally* applied in dark-mode.
To simplify the overall implementation, since all of this does seem like somewhat of an edge-case, the `viewerCssTheme` preference will *only* be read during viewer initialization. (Similar to many other existing preferences, a reload is thus required when changing it.)
@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 16, 2020

The reason for the error is at least more clear now: _forceCssTheme: "Permission denied to access property 0". Not sure why this happens, because it only happens when the console is open during the reload. If I close it, it's all good (using Firefox 82.0.3 on Arch Linux). This very much looks like the open browser bug https://bugzilla.mozilla.org/show_bug.cgi?id=1423300, and given that this works fine without the console involved we can land this. Thanks!

@timvandermeij timvandermeij merged commit eda730a into mozilla:master Nov 16, 2020
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Nov 16, 2020

This very much looks like the open browser bug https://bugzilla.mozilla.org/show_bug.cgi?id=1423300,

That one also references https://bugzilla.mozilla.org/show_bug.cgi?id=1673199, a regression in Firefox 82, which was fixed in Firefox 84 (and uplifted to Firefox 83); so there's a chance that it's actually fixed :-)

Thanks for landing this; hopefully it'll help users work-around some of the problems described in #12290.

@Snuffleupagus Snuffleupagus deleted the viewerCssTheme-option branch November 17, 2020 09:09
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Nov 19, 2020

Given issues/bugs such as #12290 and bug 1660650, which show that a fair number of users clearly want to enforce the dark-theme for the PDF Viewer, I'm wondering if this is the sort of patch that we should consider uplifting to previous Firefox versions (once it's landed in Nightly of course)? /cc @brendandahl

As far as I'm concerned, it should be fairly safe to uplift this since all code which could potentially throw is wrapped in try...catch to prevent the viewer from breaking.

@qwer1304
Copy link

Why isn't this preference exported to Options?

@Snuffleupagus
Copy link
Collaborator Author

Why isn't this preference exported to Options?

Huh; please actually provide enough context with your question to make it easily understandable, since I've got no idea what "Options" you're referring to here!

Note that this is, in the master-branch (since it's not been part of an official release yet), already available through both the AppOptions and the Preferences, so I'm not sure what other "Options" there are here!?

@qwer1304
Copy link

qwer1304 commented Nov 27, 2020

I was referring to the 'Extension options' (I have built the extension from master and can set the cssTheme from preferences with the desired effect, but don't see anything in Extension options'.) This is in Chrome.

That was easy to fix:

  • Added a new viewerCssTheme template to extensions/chromium/options/options.html
<template id="viewerCssTheme-template">
<div class="settings-row">
  <label>
    <span></span>
    <select>
      <option value="0">Auto</option>
      <option value="1">Light</option>
      <option value="2">Dark</option>
    </select>
  </label>
</div>
</template>
  • Added title and description fields to viewerCssTheme in extensions/chromium/preferences_schema.json
"title": "Set Theme CSS",
"description": "What CSS should be used.\n 0 = Default (uses system CSS).\n 1 = Light.\n 2 = Dark.",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants