Bug 119356 – [Forms: color] <input type='color'> popover color well implementation

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

[Ruth Fong]

This bug tracks implementing <input type='color'> with a popover color well.

[Ruth Fong]

<rdar://problem/14411008>

[Ruth Fong]

Created attachment 207899 [details]
Patch

first patch towards popover implementation

[Early Warning System Bot]

Comment on attachment 207899 [details]
Patch

Attachment 207899 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1295940

[EFL EWS Bot]

Comment on attachment 207899 [details]
Patch

Attachment 207899 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1292896

[Early Warning System Bot]

Comment on attachment 207899 [details]
Patch

Attachment 207899 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1298956

[Build Bot]

Comment on attachment 207899 [details]
Patch

Attachment 207899 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1292902

[Ruth Fong]

Comment on attachment 207899 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2944
> +#if ENABLE(INPUT_TYPE_COLOR_POPOVER)
> +    m_colorPicker = m_pageClient->createColorPicker(this, initialColor, elementRect);
> +#else
>      if (!m_colorPicker)
>          m_colorPicker = m_pageClient->createColorPicker(this, initialColor, elementRect);
>      m_colorPicker->showColorPicker(initialColor);
> +#endif

Every time a new <input type='color'> element is activated, the previous popover is destroyed and a new one constructed. This differs from the panel implementation; which "resets" the color picker instead of tearing it down and building a new one when it's associated to another color element. Should we standardize the implementation? (Probably yes, that where there's a more similar implementation for the color picker UIs in WebColorPickerMac.)

Disadvantage of createIfNeeded-and-show: It'd be harder to follow the createIfNeeded then show model for popover because in WKColorPopoverMac, initForFrame needs to be called to set the popover over the right color element, but it can be done (more information would have to be passed to showColorPicker, such as elementRect and m_pageClient->wkView()).

For destroy-and-create: For the panel implementation, it may be a poor UI experience (will try it out) having a picker disappear and then reappear.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:103
> +#endif

The deconstructor probably shouldn't have the ASSERT. Will file a new bug to fix this.

[Ruth Fong]

Created attachment 207901 [details]
Patch

fixed feature defines bug

[Benjamin Poulain]

Comment on attachment 207901 [details]
Patch

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

Some comments:

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:65
> +@interface WKColorPickerMac  : NSObject<NSWindowDelegate> {
> +@protected
> +BOOL _lastChangedByUser;
> +WebColorPickerMac* _picker;
>  }
> -
> -- (id)init;
> -- (void)setAndShowPicker:(WebKit::WebColorPickerMac*)picker withColor:(NSColor *)color;
> -- (void)didChooseColor:(NSColorPanel *)panel;
> +- (void)didChooseColor:(id)sender;
>  - (void)invalidate;
> -
> -// Sets color to the NSColorPanel as a non user change.
>  - (void)setColor:(NSColor *)color;
> +@end
>  
> +@implementation WKColorPickerMac
> +- (void)didChooseColor:(id)sender { }
> +- (void)invalidate { }
> +- (void)setColor:(NSColor *)color { }
>  @end

I think you should transform WKColorPickerMac to a protocol, and move the two attributes to the subclass.

WKColorPickerMac does not have ownership of _lastChangedByUser which leads to improper encapsulation. You should try to avoid inheritance as a way to share code, using encapsulation/aggregation leads to better designs.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:74
> +    NSPopoverColorWell* popoverWell;

You could use a RetainPtr here to avoid risking leaking this.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:159
> +            return self;

Indent.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:167
> +    [popoverWell retain];

This is odd!

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:177
> +}
> +

I would also add a destructor just to be safe. You could call invalidate from it, or just assert that _picker and popoverWell are nil.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:183
> +    [popoverWell removeFromSuperviewWithoutNeedingDisplay];
> +    [popoverWell deactivate];
> +    [popoverWell release];
> +    _picker = nil;

I believe you also want to remove the target of the popowerWell. Otherwise, if an other reference to the object exist, you may still get called.

[Build Bot]

Comment on attachment 207901 [details]
Patch

Attachment 207901 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1307198

[Build Bot]

Comment on attachment 207901 [details]
Patch

Attachment 207901 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1259019

[Build Bot]

Comment on attachment 207901 [details]
Patch

Attachment 207901 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1311014

[Ruth Fong]

Comment on attachment 207901 [details]
Patch

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

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:65
>>  @end
> 
> I think you should transform WKColorPickerMac to a protocol, and move the two attributes to the subclass.
> 
> WKColorPickerMac does not have ownership of _lastChangedByUser which leads to improper encapsulation. You should try to avoid inheritance as a way to share code, using encapsulation/aggregation leads to better designs.

Moving the private variables makes  sense.

I like WKColorPickerMac as a base class because that way m_colorPickerUI can be of type WKColorPickerMac and I can maintain more abstraction in WebColorPickerMac.

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:74
>> +    NSPopoverColorWell* popoverWell;
> 
> You could use a RetainPtr here to avoid risking leaking this.

Got it.

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:177
>> +
> 
> I would also add a destructor just to be safe. You could call invalidate from it, or just assert that _picker and popoverWell are nil.

Got it.

>> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:183
>> +    _picker = nil;
> 
> I believe you also want to remove the target of the popowerWell. Otherwise, if an other reference to the object exist, you may still get called.

Got it.

[Brady Eidson]

Comment on attachment 207901 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1008
> +#if ENABLE(INPUT_TYPE_COLOR) && ENABLE(INPUT_TYPE_COLOR_POPOVER)

This preprocessor statement is unnecessary.

Enabling INPUT_TYPE_COLOR_POPOVER without enabling INPUT_TYPE_COLOR is an invalid build config.

I think each place you use this macro it should just be the INPUT_TYPE_COLOR_POPOVER clause by itself.

[Ruth Fong]

Created attachment 207942 [details]
Patch

uses WKSI, which hasn't been updated yet

[Build Bot]

Comment on attachment 207942 [details]
Patch

Attachment 207942 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1321064

[Build Bot]

Comment on attachment 207942 [details]
Patch

Attachment 207942 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1321066

[Build Bot]

Comment on attachment 207942 [details]
Patch

Attachment 207942 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1314149

[Ruth Fong]

Created attachment 207953 [details]
Patch

don't need header because not using SPI methods declared in the header

[Ruth Fong]

Created attachment 207957 [details]
Patch

added space

[Daniel Bates]

Comment on attachment 207957 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:150
> +            return self;

Nit: The indentation is off here.

[Benjamin Poulain]

Comment on attachment 207957 [details]
Patch

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

> Source/WebKit2/Configurations/FeatureDefines.xcconfig:102
> +ENABLE_INPUT_TYPE_COLOR_POPOVER = ENABLE_INPUT_TYPE_COLOR_POPOVER;

You should probably update all 4 FeatureDefines.xcconfig to avoid future confusions.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1009
> +    // Close popover color well when resizing window.

This comment does not add value.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2938
> +    // A new popover color well needs to be created (and the previous one destroon each activation of a color element.

The comment should be in the #ifdef.

destroon? :)

Aaaaaarg, an unclosed parenthesis.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2940
> +    m_colorPicker = nil;

nil -> 0

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.h:74
> +    RetainPtr<NSObject<WKColorPickerUIMac> > m_colorPickerUI;

You can remove the space between the two '>' (Yay C++ 11).

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:106
> +    m_colorPickerUI = nullptr;

nullptr -> nil?

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:150
> +            return self;

Indentation.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:200
> +        _lastChangedByUser = YES;
> +            return;
> +        }

Indentation.

[Ruth Fong]

Created attachment 207972 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 207972 [details]
Patch

Clearing flags on attachment: 207972

Committed r153633: <http://trac.webkit.org/changeset/153633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.