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: support more color formats for backgroundColor #31868

Merged
merged 10 commits into from Mar 21, 2022

Conversation

codebytere
Copy link
Member

Description of Change

Refs #31019.

In #30193, we unintentionally refactored WebPreferences such that backgroundColor no longer accepted CSS color values like red. However, that only worked for some types of backgroundColor methods, since others did enforce that the passed value be hex.

This PR thus adds more official support for the following formats:

  • CSS Color Names
  • HSL Colors
  • RGB Colors
  • Hex Color Values

And as a bonus removes some duplicate code from our color_util file.

Checklist

Release Notes

Notes: Added support for more color formats in setBackgroundColor.

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

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 23, 2021
@miniak
Copy link
Contributor

miniak commented Nov 24, 2021

API LGTM

@codebytere codebytere force-pushed the support-more-color-formats branch 2 times, most recently from e5097a6 to 7c84127 Compare January 17, 2022 10:19
docs/api/browser-view.md Outdated Show resolved Hide resolved
docs/api/browser-window.md Outdated Show resolved Hide resolved
docs/api/browser-window.md Outdated Show resolved Hide resolved
shell/common/color_util.h Outdated Show resolved Hide resolved
shell/common/color_util.cc Outdated Show resolved Hide resolved
shell/common/color_util.cc Outdated Show resolved Hide resolved
shell/common/color_util.cc Outdated Show resolved Hide resolved
docs/api/browser-window.md Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the support-more-color-formats branch 2 times, most recently from c2dcb17 to f797a7a Compare January 20, 2022 09:56
@codebytere codebytere added semver/major incompatible API changes target/18-x-y and removed target/16-x-y semver/minor backwards-compatible functionality labels Feb 2, 2022
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

I have a couple of nit suggestions but overall LGTM

shell/common/color_util.cc Outdated Show resolved Hide resolved
shell/common/color_util.cc Outdated Show resolved Hide resolved
shell/common/color_util.cc Outdated Show resolved Hide resolved
shell/common/color_util.h Show resolved Hide resolved
shell/common/color_util.cc Outdated Show resolved Hide resolved
shell/common/color_util.cc Outdated Show resolved Hide resolved
@nornagon
Copy link
Member

nornagon commented Mar 3, 2022

Discussed with the API WG and we'll keep the ARGB format to avoid breaking existing apps, and drop the format option from getBackgroundColor.

docs/api/browser-view.md Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Rose <jeremya@chromium.org>
docs/api/browser-view.md Outdated Show resolved Hide resolved
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

lgtm with nits

@codebytere codebytere merged commit db79734 into main Mar 21, 2022
@codebytere codebytere deleted the support-more-color-formats branch March 21, 2022 17:35
@release-clerk
Copy link

release-clerk bot commented Mar 21, 2022

Release Notes Persisted

Added support for more color formats in setBackgroundColor.

@trop
Copy link
Contributor

trop bot commented Mar 21, 2022

I have automatically backported this PR to "18-x-y", please check out #33364

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.

None yet

7 participants