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

Remove the viewerCssTheme-option, since it's partially broken #17222

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

The viewerCssTheme-implementation has always been somewhat hacky, and now it's also partially broken ever since we've started using CSS nesting.
Trying to support nested media queries would thus require a lot more parsing of the CSS rules, which seems inefficient and thus generally undesirable.[1]

As discussed on Matrix, let's try to remove the viewerCssTheme-option and see if there's any (significant) fallout from this.


[1] If this option is brought back, it seems to me that it (in Firefox) should probably be set through the platform-code that handles theming.

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/35fa5addf0e2cb9/output.txt

@Snuffleupagus Snuffleupagus added the release-blocker Blocker for the upcoming release label Nov 4, 2023
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/35fa5addf0e2cb9/output.txt

Total script time: 1.40 mins

Published

@Snuffleupagus Snuffleupagus linked an issue Nov 4, 2023 that may be closed by this pull request
The `viewerCssTheme`-implementation has always been somewhat hacky, and now it's also *partially* broken ever since we've started using CSS nesting.
Trying to support nested media queries would thus require a lot more parsing of the CSS rules, which seems inefficient and thus generally undesirable.[1]

As discussed on Matrix, let's try to remove the `viewerCssTheme`-option and see if there's any (significant) fallout from this.

---
[1] If this option is brought back, it seems to me that it (in Firefox) should probably be set through the platform-code that handles theming.
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself looks good, but since I can't really tell how many people use this functionality, and if it's therefore useful to keep or not, I'd also like @calixteman to sign off on this before merging.

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option exists in Firefox but it's hidden and totally undocumented so it's fine to remove it.
Thank you.

@Snuffleupagus Snuffleupagus merged commit 50f52b4 into mozilla:master Nov 4, 2023
4 checks passed
@Snuffleupagus Snuffleupagus deleted the rm-viewerCssTheme branch November 4, 2023 16:40
@Rob--W
Copy link
Member

Rob--W commented Nov 5, 2023

I'm one day late to the party... but can this functionality be restored? The Chrome extension has historically been using the dark theme, and the viewerCssTheme preference is relied upon by the extension in #13841

@Rob--W
Copy link
Member

Rob--W commented Nov 5, 2023

If runtime parsing is a concern, an alternative is to do that at compile time, by extracting the parts with "prefers-color-scheme", and generate a separate stylesheet with only these rules. If the pref is set to anything but the default (forced light/dark), then the viewer code can load the extra stylesheet that unconditionally applies the light/dark theme.

@snofear
Copy link

snofear commented Nov 15, 2023

Is there any other way to manually change pdfjs theme? It was very useful option for us.

@Rob--W
Copy link
Member

Rob--W commented Nov 18, 2023

FYI: The viewerCssTheme option has been restored in #17293, implemented in a more reliable way than before to avoid #17057.

@tester-sn
Copy link

so how can i force dark theme in pdf viewer in version 121.0?
my windows and firefox use light theme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Blocker for the upcoming release viewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting theme programmatically does not seem to work
7 participants