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

[Bug]: setWindowOpenHandler changes default stylesheet for plain text files in dark theme #37737

Closed
3 tasks done
ajafff opened this issue Mar 28, 2023 · 10 comments · Fixed by #36914
Closed
3 tasks done
Labels
22-x-y bug 🪲 duplicate has-repro-gist Issue can be reproduced with code at https://gist.github.com/ platform/all status/confirmed A maintainer reproduced the bug or agreed with the feature

Comments

@ajafff
Copy link

ajafff commented Mar 28, 2023

Preflight Checklist

Electron Version

>=22.0.0

What operating system are you using?

Windows

Operating System Version

Windows 10 Enterprise 22H2

What arch are you using?

x64

Last Known Working Electron version

21

Expected Behavior

Click on the link to open the plain text file in a new window.
Text should be readable, e.g. white text on dark background.

Actual Behavior

Text is not readable. It's white text on white background.
This seems to only happen if there is a setWindowOpenHandler. If you comment out that line, a dark background is applied.

Testcase Gist URL

https://gist.github.com/ajafff/8d9e8eb8cc0a7ec2297f2445c54f5526

Additional Information

No response

@ckerr ckerr added platform/all status/confirmed A maintainer reproduced the bug or agreed with the feature has-repro-gist Issue can be reproduced with code at https://gist.github.com/ labels Mar 28, 2023
@ckerr
Copy link
Member

ckerr commented Mar 28, 2023

This was introduced somewhere in v22.0.0-nightly.20220809...v22.0.0-nightly.20220810, most likely in #34526 from the announced breaking change of replacing the new-window event with webContents.setWindowOpenHandler().

@ckerr ckerr added the 22-x-y label Mar 28, 2023
@jeremyspiegel
Copy link
Contributor

Is this the same as #36538?

@ckerr
Copy link
Member

ckerr commented Mar 28, 2023

@jeremyspiegel probably not?

I agree they do look similar, but if 36538 was present in 21.3.1 and 37737 first appeared in v22.0.0-nightly.20220810 then they can't be the same issue 💫

@jeremyspiegel
Copy link
Contributor

@ckerr sorry you're right. Using onclick="window.open('/popup.txt')" to open the window had the issue in v21, but using <a href="/popup.txt" target="_blank"> doesn't.

@jeremyspiegel
Copy link
Contributor

I would guess that the underlying issue behind both is something about the change to use the transparent web preference to set the background color, introduced in #28054. Looks like that preference was only being set for the window.open case at that point, but then with #34526 it was then set for the <a target="_blank"> case as well.

@ajafff
Copy link
Author

ajafff commented Mar 28, 2023

I agree that they look like duplicates. Feel free to close this one.
I would also like to see the window.open() case fixed, as that's what I'm actually using in my app. I just reduced it to <a target="_blank"> for this reproduction

@ckerr
Copy link
Member

ckerr commented Mar 28, 2023

Well now I'm confused 😄

Can someone test & confirm whether or not 36538 is present in 21.3.1? If so, I don't see how these two tickets can be the same issue

@jeremyspiegel
Copy link
Contributor

jeremyspiegel commented Mar 28, 2023

#36538 is present in 21.3.1.

There's probably a single underlying cause for both issues. The problematic code was likely introduced earlier in #28054, and only affected the window.open case, but not the <a target="_blank"> case. Then later the change #34526 in v22 caused the <a target="_blank"> case to also fall into the same problematic code path.

@dsanders11
Copy link
Member

@ckerr, it's the same issue as #36538. I have the fix for these issues in #36914 but CI was giving me trouble with the tests, so I shelved it and then got sidetracked with other work. I've confirmed the fix in that PR fixes the gist provided in this issue. I'll pick it back up and see if I can make the tests happy.

@nornagon
Copy link
Member

Duplicate of #36538.

@nornagon nornagon closed this as not planned Won't fix, can't repro, duplicate, stale Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
22-x-y bug 🪲 duplicate has-repro-gist Issue can be reproduced with code at https://gist.github.com/ platform/all status/confirmed A maintainer reproduced the bug or agreed with the feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants