Bug 216514 – [macOS] Date pickers should respect the document's color scheme

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/68889548>

[Aditya Keerthi]

Created attachment 408774 [details]
Patch

[Tim Horton]

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?

[Aditya Keerthi]

(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?

[Tim Horton]

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.

[Aditya Keerthi]

Created attachment 408872 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[Aditya Keerthi]

Created attachment 408889 [details]
Patch for landing.

[EWS]

Committed r267131: <https://trac.webkit.org/changeset/267131>

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