Bug 216514 - [macOS] Date pickers should respect the document's color scheme
Summary: [macOS] Date pickers should respect the document's color scheme
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-14 17:41 PDT by Aditya Keerthi
Modified: 2020-09-15 20:10 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.27 KB, patch)
2020-09-14 17:44 PDT, Aditya Keerthi
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (5.27 KB, patch)
2020-09-15 16:02 PDT, Aditya Keerthi
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing. (5.26 KB, patch)
2020-09-15 19:43 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2020-09-14 17:41:04 PDT
...
Comment 1 Aditya Keerthi 2020-09-14 17:41:55 PDT
<rdar://problem/68889548>
Comment 2 Aditya Keerthi 2020-09-14 17:44:14 PDT
Created attachment 408774 [details]
Patch
Comment 3 Tim Horton 2020-09-14 17:57:03 PDT
Comment on attachment 408774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408774&action=review

> Source/WebCore/html/HTMLInputElement.cpp:2140
> +    if (auto* computedStyle = this->computedStyle()) {

I assume this null check is not actually necessary, since the old code did not. Is that true?

> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:221
> +    [_enclosingWindow setAppearance:[NSAppearance appearanceNamed:_params.useDarkAppearance ? NSAppearanceNameDarkAqua : NSAppearanceNameAqua]];

This is interesting! I feel like content that pops out of the page (like, say, context menus) usually follows the app, not the page. But probably this makes sense?
Comment 4 Aditya Keerthi 2020-09-14 18:02:56 PDT
(In reply to Tim Horton from comment #3)
> Comment on attachment 408774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408774&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:2140
> > +    if (auto* computedStyle = this->computedStyle()) {
> 
> I assume this null check is not actually necessary, since the old code did
> not. Is that true?

The check is not actually necessary, since this method is only called when the element is in the node tree. Should I replace the check with an ASSERT?
Comment 5 Tim Horton 2020-09-15 14:11:06 PDT
Comment on attachment 408774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408774&action=review

>> Source/WebKit/UIProcess/mac/WebDateTimePickerMac.mm:221
>> +    [_enclosingWindow setAppearance:[NSAppearance appearanceNamed:_params.useDarkAppearance ? NSAppearanceNameDarkAqua : NSAppearanceNameAqua]];
> 
> This is interesting! I feel like content that pops out of the page (like, say, context menus) usually follows the app, not the page. But probably this makes sense?

You can just leave it implicit, too. No reason to ASSERT() something that we expect will never happen and which will result in a crash in release, IMO.
Comment 6 Aditya Keerthi 2020-09-15 16:02:01 PDT
Created attachment 408872 [details]
Patch for landing
Comment 7 EWS 2020-09-15 19:39:52 PDT
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Comment 8 Aditya Keerthi 2020-09-15 19:43:17 PDT
Created attachment 408889 [details]
Patch for landing.
Comment 9 EWS 2020-09-15 20:10:35 PDT
Committed r267131: <https://trac.webkit.org/changeset/267131>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408889 [details].