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

Support light and dark color scheme #1898

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

b0o
Copy link
Collaborator

@b0o b0o commented Nov 17, 2022

Background

Months ago, issue #1719 was opened, reporting that Surfingkeys was causing the page to become blank in Safari when dark mode was active. This was resolved in ccedde8 by setting color-scheme: light on the Surfingkeys container div.

ccedde8 fixed the original issue, but by forcing the Surfingkeys iframe to always use color-scheme: light, it created a new issue that the user couldn't use @media(prefers-color-scheme: dark) in their custom themes (at least in Firefox).

The interaction between color-scheme and transparent iframes

The way browsers handle color-scheme in combination with transparent iframes is nuanced. The CSSWG decided that "if the color scheme of an iframe differs from embedding document, the iframe gets an opaque canvas bg appropriate to its color scheme". I believe this behavior is what was causing #1719 in the first place. I found a good blog post explaining these behaviors.

This PR

Instead of setting color-scheme: light on the Surfingkeys container div, this PR adds a meta tag to the Surfingkeys iframe, declaring that it supports both light and dark mode. In my testing in Firefox, this solves both problems: no white blank page, and the user can use prefers-color-scheme media queries.

I tried testing this in Safari 14.1.2 on macOS 11.6.8 - I was not able to reproduce the original bug, but it seems this change works fine. (Note: I tested this manually in the devtools in Safari, I didn't build the extension with this change for Safari). I'd appreciate if you could test this in Safari on macOS and also iPadOS, to ensure that it works properly in those places before merging.

@b0o b0o marked this pull request as draft November 17, 2022 08:37
@b0o
Copy link
Collaborator Author

b0o commented Nov 17, 2022

Converting this to a draft because it doesn't seem to be working as expected.

@b0o
Copy link
Collaborator Author

b0o commented Nov 17, 2022

Seems to be working now with latest commit, but I think it should be tested on other platforms/configs before merging.

@b0o b0o marked this pull request as ready for review November 17, 2022 08:49
@brookhong
Copy link
Owner

Just tested the change. It works on Safari/MacOS, but does not on Safari/iPhone. On iPhone we got a black page.

image

stepnem added a commit to stepnem/surfingkeys-conf that referenced this pull request Jan 28, 2023
stepnem added a commit to stepnem/surfingkeys-conf that referenced this pull request Feb 1, 2023
stepnem added a commit to stepnem/surfingkeys-conf that referenced this pull request Feb 2, 2023
stepnem added a commit to stepnem/surfingkeys-conf that referenced this pull request Feb 11, 2023
stepnem added a commit to stepnem/surfingkeys-conf that referenced this pull request Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants