Bug 187794 – [Datalist][macOS] Display suggestions for input[type=color]

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

[Aditya Keerthi]

This bug tracks the macOS work required to show suggestions for color input fields with a list attribute.

[Aditya Keerthi]

Created attachment 345314 [details]
Patch

[EWS Watchlist]

Comment on attachment 345314 [details]
Patch

Attachment 345314 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8585589

New failing tests:
http/wpt/crypto/unwrap-ec-key-crash.any.worker.html

[EWS Watchlist]

Created attachment 345343 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Tim Horton]

Comment on attachment 345314 [details]
Patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1518
> +std::optional<Color> ArgumentCoder<Color>::decode(Decoder& decoder)

I don’t think it’s ever right to have two decode functions for the same type. You should check with e.g. Alex and see what the current direction is here. Might be that you just get rid of the non-optional one? I don’t recall.

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:51
> +@interface NSPopover (AppKitSecretsIKnow)

No need to name the category. Also these should go in the AppKitSPI header, no?

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:174
> ++ (NSPopover *)_colorPopoverCreateIfNecessary:(BOOL)forceCreation {

I think there’s been a lot of debate over this and people have settled on a particular naming scheme for these things. But in general:

- should probably be a static C function.
- should probably not have “create if necessary”. I think the word we use is “ensure” but you should check. Yours is a little odd in that only sometimes do you want to ensure it...
- might want a NeverDestroyed<RetainPtr<>>? But again, not sure there’s a reason for it, just seems to happen.

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:177
> +        NSPopover *popover = [[NSPopover alloc] init];

RetainPtr!

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:181
> +        NSColorPopoverController *controller = [[NSClassFromString(@"NSColorPopoverController") alloc] init];

Why NSClassFromString instead of putting it in an SPI header?

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:226
> +    [_suggestedColors release];

Why not a RetainPtr!

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:258
> +        suggestedColors =  [[[NSColorList alloc] init] autorelease];

extra space before [

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:260
> +            [suggestedColors insertColor:nsColor(suggestions.at(i)) key:[NSString stringWithFormat:@"%zu", i] atIndex:i];

There are much more efficient ways of making an NSString from a number, I believe. I don’t know what is best, but I bet @(i).stringValue is better than this.

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:323
> +- (void)setAndShowPicker:(WebColorPickerMac*)picker withColor:(NSColor *)color andSuggestions:(Vector<WebCore::Color>&&)suggestions

I think you can leave the “and” out of “andSuggestions”, don’t think that’s Cocoa convention.

[Aditya Keerthi]

Created attachment 345628 [details]
Patch

[Aditya Keerthi]

Created attachment 345697 [details]
Patch

[Tim Horton]

Comment on attachment 345697 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/mac/NSColorWellSPI.h:34
> +#endif // PLATFORM(MAC)

Is it USE(APPKIT), or PLATFORM(MAC)???

> Source/WebCore/PAL/pal/spi/mac/NSPopoverColorWellSPI.h:52
> +#endif // PLATFORM(MAC)

Ditto

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1520
> +    bool isExtended;

It has got to be possible to implement the other one in terms of this one, no? Just don’t love all the duplication, and also can see myself fixing one and not the other.

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:148
> ++ (NSPopover *)_colorPopoverCreateIfNecessary:(BOOL)forceCreation {

Brace in the wrong place

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:166
> +- (void)_showPopover {

Brace in the wrong place

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:174
> +    if ([owner isKindOfClass:[NSPopoverColorWell class]])
> +        [owner deactivate];

What is this fragile-looking code for? Comments, maybe?

[Aditya Keerthi]

Created attachment 345711 [details]
Patch

[Aditya Keerthi]

(In reply to Tim Horton from comment #7)
> Comment on attachment 345697 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345697&action=review
> 
> It has got to be possible to implement the other one in terms of this one,
> no? Just don’t love all the duplication, and also can see myself fixing one
> and not the other.

Removed the duplicated code. Also, Alex confirmed that we should keep both decode methods (one returning a bool and one returning an std::optional).

[Tim Horton]

Comment on attachment 345711 [details]
Patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1521
> +    if (!decode(decoder, color))

OMG SO MUCH BETTER

[WebKit Commit Bot]

Comment on attachment 345711 [details]
Patch

Clearing flags on attachment: 345711

Committed r234172: <https://trac.webkit.org/changeset/234172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42557159>