Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is correct 🤔 I think this should only be
SK_ColorTRANSPARENT
if the owning BrowserWindow is transparent 🤔 I think WebContentsPreferences used to have a concept of transparency but apparently not any moreThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the intention with using the optional value.
The
backgroundColor
web preferences property was changed to use a hidden field. I wonder if this also needs to be the same with BrowserViews somewhere?electron/shell/browser/api/electron_api_browser_window.cc
Lines 40 to 50 in c5b517d
Also, just a heads up that the v14 implementation differs a bit because of the WebPreferences refactor. Backport might be painful 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I have some wip tests written for backgroundColor so hopefully we'll be able to catch these issues in the future! 🤞 https://github.com/electron/electron/compare/main...samuelmaddock:test/bw-bg-color?expand=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little more investigation: BrowserView might not be calling
WebContents::SetPageBaseBackgroundColor()
? 🤔electron/shell/browser/api/electron_api_browser_view.cc
Lines 149 to 151 in b491a4c
compared to BrowserWindow's code
electron/shell/browser/api/electron_api_browser_window.cc
Lines 363 to 375 in b491a4c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not be calling it or there may be a painting/timing error on when it's called?
I actually found this bug yesterday while digging into this more: #30993 If I start a transparent window in Fiddle, the background renders as white, but then sometimes will switch back to transparent if I alt+tab away. Maybe there's a timing issue where background_color sometimes isn't being set before the first paint? Looking into it more today! 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samuelmaddock You were right - it looks like there's an issue with
SetPageBaseBackgroundColor
, where transparent windows would be white until a repaint (e.g. resize) happened. It looks like Chrome's going to merge the two APIs and address this down the line (https://chromium-review.googlesource.com/c/chromium/src/+/2958369), but we can get around it now by setting the background directly on the render widget host view. There's a new PR open to address this (linked below) 🙂