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

feat: add transparent webpreference to webview #40301

Merged
merged 16 commits into from Jan 5, 2024
Merged

feat: add transparent webpreference to webview #40301

merged 16 commits into from Jan 5, 2024

Conversation

BrandonXLF
Copy link
Contributor

@BrandonXLF BrandonXLF commented Oct 23, 2023

Add a transparent webpreference that when disabled makes webviews have an opaque background that follows the guest page's color scheme.

Fixes #40207

Checklist

Release Notes

Notes: Added a transparent webpreference to webviews

@welcome
Copy link

welcome bot commented Oct 23, 2023

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 23, 2023
@BrandonXLF BrandonXLF marked this pull request as ready for review October 23, 2023 03:23
@BrandonXLF BrandonXLF requested a review from a team as a code owner October 23, 2023 03:23
AbdullahWins

This comment was marked as spam.

@samuelmaddock samuelmaddock added semver/major incompatible API changes no-backport labels Oct 29, 2023
@electron-cation electron-cation bot added api-review/requested 🗳 and removed new-pr 🌱 PR opened in the last 24 hours labels Oct 29, 2023
spec/webview-spec.ts Outdated Show resolved Hide resolved
shell/browser/api/electron_api_web_contents.h Outdated Show resolved Hide resolved
spec/webview-spec.ts Outdated Show resolved Hide resolved
shell/browser/api/electron_api_web_contents.cc Outdated Show resolved Hide resolved
@BrandonXLF BrandonXLF changed the title feat: add transparent attribute to webview feat: add opaque attribute to webview Nov 10, 2023
@samuelmaddock
Copy link
Member

I hadn't realized defaulting transparent to true wouldn't be compatible with the design of the HTML attribute.

transparent may still better align with existing APIs such as BrowserWindow's transparent option. I'll run this by @electron/wg-api and get back to you on next steps regarding the breaking change.

@BrandonXLF
Copy link
Contributor Author

I hadn't realized defaulting transparent to true wouldn't be compatible with the design of the HTML attribute.

transparent may still better align with existing APIs such as BrowserWindow's transparent option. I'll run this by @electron/wg-api and get back to you on next steps regarding the breaking change.

Sounds good! I initially thought having to enable transparency seemed more intuitive since iframes are opaque by default, but I'm not sure if that warrants breaking the current default behaviour. Thanks for looking into this for me and I look forward to hearing back.

@samuelmaddock
Copy link
Member

@BrandonXLF the @electron/wg-api discussed the API and thought of adding transparent in webview's webpreferences attribute.

<!-- Disable transparency -->
<webview webpreferences="transparent=no"></webview>

<!-- Enable transparency -->
<webview webpreferences="transparent=yes"></webview>
<webview></webview>

This would avoid the naming awkwardness and any breaking changes.

Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

Comment on lines 223 to 224
The full list of supported preference strings can be found in [BrowserWindow](browser-window.md#new-browserwindowoptions)
in addition to `transparent`.
Copy link
Member

Choose a reason for hiding this comment

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

A few suggestions here:

  • Use a list similar to the linked docs for web preferences.
  • Link to native theme source. It seems like that's what's being referred to when you mentioned color scheme.
Suggested change
The full list of supported preference strings can be found in [BrowserWindow](browser-window.md#new-browserwindowoptions)
in addition to `transparent`.
The full list of supported preference strings can be found in [BrowserWindow](browser-window.md#new-browserwindowoptions). In addition, webview supports the following preferences:
- `transparent` boolean (optional) - Whether to enable background transparency in the guest page. Default is `true`. **Note:** Text and background colors are derived based on the [native theme source](native-theme.md#nativethemethemesource) by default. When transparency is enabled, the text color may still change accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to the suggested list format and added a link to https://developer.mozilla.org/en-US/docs/Web/CSS/color-scheme.

@samuelmaddock samuelmaddock added semver/minor backwards-compatible functionality and removed semver/major incompatible API changes api-review/reviewed labels Nov 29, 2023
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

API LGTM

@BrandonXLF
Copy link
Contributor Author

@zcbenz @samuelmaddock It looks like one of the Mac OS tests timed out, would you be able to rerun them?

@zcbenz
Copy link
Member

zcbenz commented Dec 14, 2023

@BrandonXLF Sorry for the delay, #35658 has been merged recently and this PR has a minor conflict with it, can you rebase this branch on latest main? After that I'll merge this PR.

@BrandonXLF
Copy link
Contributor Author

@zcbenz Done. It also looks like #35658 made webviews non-transparent by default, I've gone ahead and reverted this behaviour.

@zcbenz
Copy link
Member

zcbenz commented Dec 15, 2023

/cc @nornagon on the behavior of webview getting changed in #35658, how do you think if we merge this PR which reverts the changed behavior, and then fix the backports of 35658?

@zcbenz
Copy link
Member

zcbenz commented Jan 5, 2024

I'm merging this PR since it has implemented the API requested by API working group.

@zcbenz zcbenz merged commit 5086071 into electron:main Jan 5, 2024
14 checks passed
Copy link

release-clerk bot commented Jan 5, 2024

Release Notes Persisted

Added a transparent webpreference to webviews

@BrandonXLF BrandonXLF deleted the webview-transparent-attr branch January 6, 2024 19:59
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.

[Bug]: Setting color-scheme to dark changes text color but not background color for webviews
5 participants