This bug tracks implementing <input type='color'> with a popover color well.
<rdar://problem/14411008>
Created attachment 207899 [details] Patch first patch towards popover implementation
Comment on attachment 207899 [details] Patch Attachment 207899 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1295940
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
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
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
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.
Created attachment 207901 [details] Patch fixed feature defines bug
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.
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
Comment on attachment 207901 [details] Patch Attachment 207901 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1259019
Comment on attachment 207901 [details] Patch Attachment 207901 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1311014
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.
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.
Created attachment 207942 [details] Patch uses WKSI, which hasn't been updated yet
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
Comment on attachment 207942 [details] Patch Attachment 207942 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1321066
Comment on attachment 207942 [details] Patch Attachment 207942 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1314149
Created attachment 207953 [details] Patch don't need header because not using SPI methods declared in the header
Created attachment 207957 [details] Patch added space
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.
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.
Created attachment 207972 [details] Patch
Comment on attachment 207972 [details] Patch Clearing flags on attachment: 207972 Committed r153633: <http://trac.webkit.org/changeset/153633>
All reviewed patches have been landed. Closing bug.