Implement the UI behavior for the Mac platform. You should be able to pick a color from a color picker when the user clicks on a color control.
Created attachment 94407 [details] test implementation of UI behaviour
(In reply to comment #1) > Created an attachment (id=94407) [details] > test implementation of UI behaviour Patch needs to be applied after the patch from Bug 61275.
Created attachment 94409 [details] Screen recording of patch
Created attachment 96554 [details] patch
Comment on attachment 96554 [details] patch I recommend you split the patch into - sort Xcode project (it doesn't need review) - WebCore part - WebKit(2)/mac part
<rdar://problem/10269922>
Created attachment 205435 [details] Patch
Comment on attachment 205435 [details] Patch Attachment 205435 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/944202 New failing tests: compositing/geometry/bounds-clipped-composited-child.html compositing/contents-opaque/body-background-painted.html compositing/contents-opaque/overflow-hidden-child-layers.html compositing/geometry/bounds-ignores-hidden-composited-descendant.html compositing/framesets/composited-frame-alignment.html compositing/backing/no-backing-for-clip-overlap.html compositing/filters/sw-shadow-overlaps-hw-shadow.html compositing/contents-opaque/background-color.html compositing/contents-opaque/layer-transform.html compositing/iframes/become-overlapped-iframe.html compositing/filters/sw-shadow-overlaps-hw-layer.html compositing/animation/animation-compositing.html compositing/contents-opaque/hidden-with-visible-child.html compositing/backing/no-backing-for-clip.html compositing/filters/sw-layer-overlaps-hw-shadow.html compositing/geometry/ancestor-overflow-change.html compositing/contents-opaque/filter.html compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html compositing/columns/composited-in-paginated.html compositing/iframes/become-composited-nested-iframes.html compositing/contents-opaque/hidden-with-visible-text.html compositing/contents-opaque/layer-opacity.html compositing/contents-opaque/body-background-skipped.html compositing/contents-opaque/background-clip.html compositing/iframes/composited-parent-iframe.html compositing/contents-opaque/visibility-hidden.html compositing/backing/no-backing-for-clip-overhang.html compositing/backing/no-backing-for-perspective.html compositing/contents-opaque/control-layer.html compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html
Created attachment 205450 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Comment on attachment 205435 [details] Patch Attachment 205435 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/916653 New failing tests: platform/mac/accessibility/role-subrole-roledescription.html accessibility/color-well.html
Created attachment 205484 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Created attachment 206014 [details] Patch
Created attachment 206031 [details] Patch
Created attachment 206032 [details] Patch include color tests on Mac platform
Comment on attachment 206032 [details] Patch Attachment 206032 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/991372
Comment on attachment 206032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206032&action=review > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:63 > + static WebColorChooserProxyMac* open(WebColorChooserProxy::Client*, const WebCore::Color&); If this is only called from create(), this should be private. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:64 > + WebColorChooserProxyMac(WebColorChooserProxy::Client*, const WebCore::Color&); Given open() is the only function calling the constructor, you should make it private. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:85 > + BOOL _nonUserChange; The naming of this is awkward. I think it would be better to track whether the change was from the user rather than the opposite. _lastChangedByUser, perhaps? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:93 > +- (void)didChooseColor:(NSColorPanel*)panel; (NSColorPanel *) (see later comment) > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:96 > +- (void)setColor:(NSColor*)color; ditto > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:61 > +: WebColorChooserProxy(client) Missing indentation. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:66 > + [[NSColorPanel sharedColorPanel] makeKeyAndOrderFront:nil]; I think it makes more sense for the shared color panel management to all happen through the WKColorPanelCocoa class rather than doing it here. That way all interaction with NSColorPanel happens via that class. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:72 > + if (m_client) { It might be better to do the reverse check here, and return early, so that most of the code in this function isn't overly-indented. You already do this in other functions. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:73 > + ASSERT(color != nullptr); This assert doesn't make sense. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:74 > + ASSERT(m_client); This assert doesn't make sense. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:111 > + NSColorPanel* panel = [NSColorPanel sharedColorPanel]; Cocoa style dictates "NSColorPanel *panel" not "NSColorPanel* panel" > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:121 > + NSColorPanel* panel = [NSColorPanel sharedColorPanel]; Ditto. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:131 > +- (void)windowWillClose:(NSNotification*)notification { Ditto. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:136 > +- (void)didChooseColor:(NSColorPanel*)panel { Ditto. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:146 > +- (void)setColor:(NSColor*)color { Ditto.
Created attachment 206336 [details] Patch
The most recent patch does not support a page with more than one <input type='color'> element (the current patch handles <input type='color'> across multiple processes fine though). This is because at WebChromeClient::createColorChooser, we're returning a nullptr if there's already an activeColorChooser. This is a bit more complicated than deleting that if-clause because WebColorChooserProxyMac::create first destroys its singleton object if it's already been created, setting a cascade down the stack to destroy the WebColorChooser m_activeColorChooser object in WebPage. My current approach is to not create a new WebColorChooser for every <input type='color'> on a webpage but to reuse it for a given process (since there's only going to be one color picker displayed at a time). In ColorInputType::handleDOMActivateEvent, if m_chooser has already been initialized, I'll call a new method like resetChooser(color) to use the color picker for the second <input type='color'> element. This avoids the unnecessary climb up and down the stack to create a new color picker and destroy the old one. This patch will be uploaded for review soon.
Created attachment 206501 [details] Patch still need to break association between color well and color picker when the user leaves the page
Comment on attachment 206501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206501&action=review very very brief first pass looking at small things and not the whole-picture architecture > Source/WebCore/ChangeLog:11 > + This needs sooooooo many more words. > Source/WebKit2/ChangeLog:46 > + WKColorPanelCocoa is a wrapper for a NSColorPanel object "Mac", I think? Unless there's precedence. > Source/WebKit2/UIProcess/WebPageProxy.cpp:2970 > + // FIXME: Where is this colorPickerResultListener code being used? Should probably figure out what's going on here. > Source/WebKit2/UIProcess/WebPageProxy.cpp:2999 > + m_colorChooser->resetColorChooser(color); Why does "reset" take a color? What does reset do? Is reset actually "reattach"? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:53 > +class WebColorChooserProxy; Don't need this. Already including WebColorChooserProxy.h above. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:74 > +@interface WKColorPanelCocoa : NSObject<NSWindowDelegate> { Still voting for WKColorPanelMac. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:81 > +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac *)chooser WithColor:(NSColor *)color; The first star is on the wrong side (left for C++ objects, right for ObjC). > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:43 > +//WebColorChooserProxyMac* WebColorChooserProxyMac::s_currentColorChooser; Please don't add commented out code. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:97 > +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser WithColor:(NSColor *)color { First { on next line even for ObjC methods, as far as I remember. There are a bunch of these in the patch. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:105 > + [self setColor: color]; no space after the colon. Mitz used to tell me to use dot notation for simple properties but people keep changing their mind so I don't really know.
(In reply to comment #19) > still need to break association between color well and color picker when the user leaves the page Also IIRC the color well doesn't stay highlighted when it's active?
Comment on attachment 206501 [details] Patch Attachment 206501 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/899676
Comment on attachment 206501 [details] Patch Attachment 206501 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/899673
Comment on attachment 206501 [details] Patch Attachment 206501 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/931650
Comment on attachment 206501 [details] Patch Attachment 206501 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/971838
Comment on attachment 206501 [details] Patch Attachment 206501 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1054151
Comment on attachment 206501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206501&action=review >> Source/WebKit2/UIProcess/WebPageProxy.cpp:2999 >> + m_colorChooser->resetColorChooser(color); > > Why does "reset" take a color? What does reset do? Is reset actually "reattach"? Reset is reattach. Will rename. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:74 >> +@interface WKColorPanelCocoa : NSObject<NSWindowDelegate> { > > Still voting for WKColorPanelMac. Yup - will rename.
Comment on attachment 206501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206501&action=review > Source/JavaScriptCore/ChangeLog:9 > + * Configurations/FeatureDefines.xcconfig: Please include a brief description of what's happening here. > Source/WTF/ChangeLog:9 > + * wtf/FeatureDefines.h: Please include a brief description of what's happening here. > Source/WebCore/ChangeLog:10 > + <input type='color'> objects. I assume this will be taken care of in a future patch. You won't be able to check anything if the changelogs contain "OOPS!". > Source/WebCore/html/ColorInputType.cpp:154 > + if (chrome) { Now that |chrome| has been isolated in the conditional, and it is only used within the if block, you could merge the above two lines as: if (Chrome* chrome = this->chrome()) { ... > Source/WebCore/html/ColorInputType.cpp:158 > + m_chooser->resetColorChooser(valueAsColor()); This code doesn't make sense to me on first blush. I understand there are side effects happening, but I had to comb through the whole logic to get there. Ideally you'd be seeing something more like: if (!m_chooser) m_chooser = chrome->createColorChooser(this); m_chooser->setColorChooser(valueAsColor()); where setColorChooser() implies connecting the web page to the UI. Would it possible to do this without wreaking havoc on the other ports? >> Source/WebKit2/ChangeLog:46 >> + WKColorPanelCocoa is a wrapper for a NSColorPanel object > > "Mac", I think? Unless there's precedence. +1 >> Source/WebKit2/UIProcess/WebPageProxy.cpp:2970 >> + // FIXME: Where is this colorPickerResultListener code being used? > > Should probably figure out what's going on here. You should do a grep on the other ports. If this is not needed for Macs, we should hide this in a PLATFORM #ifdef. > Source/WebKit2/UIProcess/WebPageProxy.cpp:2978 > + m_colorChooser = WebColorChooserProxy::create(this); I'm confused by this code. Why are we creating a chooser proxy here, and then also if the UI client fails to show the color picker a few lines later? > Source/WebKit2/UIProcess/WebPageProxy.cpp:2995 > +void WebPageProxy::resetColorChooser(const WebCore::Color& color) Looks like you need to provide stub impls of these for other ports. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:97 >> +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser WithColor:(NSColor *)color { > > First { on next line even for ObjC methods, as far as I remember. There are a bunch of these in the patch. Coding guidelines would make it -initWithChooser:withColor: not WithColor:. > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:-666 > - This is very strange code to have in here. In general I'm not a big fan of the side effects I'm seeing built into the constructors. > Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.cpp:74 > + m_page->setActiveColorChooser(this); Are you guaranteed that m_page is not null? > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:57 > +#include <WebCore/ColorChooser.h> #import, please.
Comment on attachment 206501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206501&action=review >> Source/WebCore/html/ColorInputType.cpp:158 >> + m_chooser->resetColorChooser(valueAsColor()); > > This code doesn't make sense to me on first blush. I understand there are side effects happening, but I had to comb through the whole logic to get there. > > Ideally you'd be seeing something more like: > > if (!m_chooser) > m_chooser = chrome->createColorChooser(this); > m_chooser->setColorChooser(valueAsColor()); > > where setColorChooser() implies connecting the web page to the UI. Would it possible to do this without wreaking havoc on the other ports? Yes. I can use the setSelectedColor method and make WebColorChooserProxyMac::setSelectedColor do the work that WebColorChooserProxyMac::resetColorChooser is currently doing, but WebColorChooserProxyMac::setSelectedColor is meant to be called from the UI: WKColorPanelMac::didChooseColor, not from the other end of the stack. It would cause a little redundancy in code execution when a color is being selected in a normal situation, as WebColorChooserProxyMac::setSelectedColor will unnecessarily call WKColorPanelMac::setColor. It's definitely do-able though (that's how I first implemented it actually). >>> Source/WebKit2/UIProcess/WebPageProxy.cpp:2970 >>> + // FIXME: Where is this colorPickerResultListener code being used? >> >> Should probably figure out what's going on here. > > You should do a grep on the other ports. If this is not needed for Macs, we should hide this in a PLATFORM #ifdef. Got it. Will grep. It's not needed for mac. >> Source/WebKit2/UIProcess/WebPageProxy.cpp:2978 >> + m_colorChooser = WebColorChooserProxy::create(this); > > I'm confused by this code. Why are we creating a chooser proxy here, and then also if the UI client fails to show the color picker a few lines later? This if statement is because I removed the ASSERT. Originally, showColorChooser was only supposed to be called if m_colorChooser was null (this is no longer the case as I'm letting WebColorChooser live as long as ColorInputType). This should be solved by preprocessing wrapping. >> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:-666 >> - > > This is very strange code to have in here. In general I'm not a big fan of the side effects I'm seeing built into the constructors. Are you referring to the lines above or the line below? (the lines above were removed) >> Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.cpp:74 >> + m_page->setActiveColorChooser(this); > > Are you guaranteed that m_page is not null? No, but I don't think there should be a real case when m_page is null and resetColorChooser is called, because it's only called when an <input type='color'> element is clicked on for a non-first time, and the WebColorChooser is owned by ColorInputType and lives as long as the input does. Will add an assert. >> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:57 >> +#include <WebCore/ColorChooser.h> > > #import, please. Got it.
Created attachment 206514 [details] Patch using setSelectedColor instead of the previous addition of resetColorChooser; still need to fix disassociating picker upon moving away from the webpage/to another webpage
Comment on attachment 206514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206514&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:582 > + m_colorChooser->endChooser(); > m_colorChooser->invalidate(); Should invalidate just implicitly endChooser()? If not, why not? > Source/WebKit2/UIProcess/WebPageProxy.cpp:2974 > #if ENABLE(INPUT_TYPE_COLOR) > void WebPageProxy::showColorChooser(const WebCore::Color& initialColor, const IntRect& elementRect) > { > +#if PLATFORM(MAC) > + m_colorChooser = m_pageClient->createColorChooserProxy(this, initialColor, elementRect); > +#else > ASSERT(!m_colorChooser); > Why is this mac specific? We should really try to keep the platform differences to a minimum at this layer. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:86 > +// A Listener class to act as a event target for NSColorPanel and send > +// the results to the C++ class, WebColorChooserProxyMac. > +@interface WKColorPanelMac : NSObject<NSWindowDelegate> { > + > +@private > + BOOL _lastChangedByUser; > + WebKit::WebColorChooserProxyMac* _chooser; > +} > + > +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser withColor:(NSColor *)color; > + > +- (void)didChooseColor:(NSColorPanel *)panel; > + > +// Sets color to the NSColorPanel as a non user change. > +- (void)setColor:(NSColor *)color; > + > +@end > + Does this need to be in the header? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:52 > +WebColorChooserProxyMac::~WebColorChooserProxyMac() > +{ > + if (m_panel) > + endChooser(); > +} The destructor should come after the constructor. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:58 > +WebColorChooserProxyMac::WebColorChooserProxyMac(WebColorChooserProxy::Client* client, const WebCore::Color& initialColor) > + : WebColorChooserProxy(client) > +{ > + m_panel = adoptNS([[WKColorPanelMac alloc] initWithChooser:this withColor:nsColor(initialColor)]); > +} can the initialization of m_panel be in the initializer list? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:63 > + if (m_panel) > + m_panel = nullptr; This branch seems unnecessary. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:91 > + if ((self = [super init])) { > + _chooser = chooser; We traditionally use the early return form of this. > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2506 > + BCBD38FA125BAB9A00D2C29F /* WebPageProxy.messages.in */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; lineEnding = 0; path = WebPageProxy.messages.in; sourceTree = "<group>"; }; > + BCBD3912125BB1A800D2C29F /* WebPageProxyMessageReceiver.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; lineEnding = 0; path = WebPageProxyMessageReceiver.cpp; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.cpp; }; > + BCBD3913125BB1A800D2C29F /* WebPageProxyMessages.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; lineEnding = 0; path = WebPageProxyMessages.h; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; }; These seem like unrelated changes. > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:-665 > - if (m_page->activeColorChooser()) > - return nullptr; Are you sure others are not relying on this?
Comment on attachment 206514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206514&action=review >> Source/WebKit2/UIProcess/WebPageProxy.cpp:582 >> m_colorChooser->invalidate(); > > Should invalidate just implicitly endChooser()? If not, why not? Yep, it should. >> Source/WebKit2/UIProcess/WebPageProxy.cpp:2974 >> > > Why is this mac specific? We should really try to keep the platform differences to a minimum at this layer. It's really because the Mac port doesn't use the stuff in the else statement. Before, INPUT_TYPE_COLOR was off for Mac. Also, earlier, the assumption was that showColorChooser would only be called when m_colorChooser is null. This is no longer the case. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:86 >> + > > Does this need to be in the header? It can be included in the implementation file, but I figured it'd be better to follow the pattern for C++ in separating the implementation and definitions. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:52 >> +} > > The destructor should come after the constructor. Got it. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:58 >> +} > > can the initialization of m_panel be in the initializer list? I'm not sure since it's a Objective-C object. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:63 >> + m_panel = nullptr; > > This branch seems unnecessary. Got it. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:91 >> + _chooser = chooser; > > We traditionally use the early return form of this. Got it. >> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2506 >> + BCBD3913125BB1A800D2C29F /* WebPageProxyMessages.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; lineEnding = 0; path = WebPageProxyMessages.h; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; }; > > These seem like unrelated changes. This may be because I'm building on Lion and on a different version of Xcode. >> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:-665 >> - return nullptr; > > Are you sure others are not relying on this? Not sure; will grep and wrap preprocessing in the meantime.
Comment on attachment 206501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206501&action=review >>>> Source/WebCore/html/ColorInputType.cpp:158 >>>> + m_chooser->resetColorChooser(valueAsColor()); >>> >>> This code doesn't make sense to me on first blush. I understand there are side effects happening, but I had to comb through the whole logic to get there. >>> >>> Ideally you'd be seeing something more like: >>> >>> if (!m_chooser) >>> m_chooser = chrome->createColorChooser(this); >>> m_chooser->setColorChooser(valueAsColor()); >>> >>> where setColorChooser() implies connecting the web page to the UI. Would it possible to do this without wreaking havoc on the other ports? >> >> Yes. I can use the setSelectedColor method and make WebColorChooserProxyMac::setSelectedColor do the work that WebColorChooserProxyMac::resetColorChooser is currently doing, but WebColorChooserProxyMac::setSelectedColor is meant to be called from the UI: WKColorPanelMac::didChooseColor, not from the other end of the stack. It would cause a little redundancy in code execution when a color is being selected in a normal situation, as WebColorChooserProxyMac::setSelectedColor will unnecessarily call WKColorPanelMac::setColor. >> >> It's definitely do-able though (that's how I first implemented it actually). > > Yes. I can use the setSelectedColor method and make WebColorChooserProxyMac::setSelectedColor do the work that WebColorChooserProxyMac::resetColorChooser is currently doing, but WebColorChooserProxyMac::setSelectedColor is meant to be called from the UI: WKColorPanelMac::didChooseColor, not from the other end of the stack. It would cause a little redundancy in code execution when a color is being selected in a normal situation, as WebColorChooserProxyMac::setSelectedColor will unnecessarily call WKColorPanelMac::setColor. > > It's definitely do-able though (that's how I first implemented it actually). Actually, I'm pretty sure we need to introduce the new API resetColorChooser to reset the color picker's to the current <input type='color'> element (i.e. we will need to call [panel setTarget:self] again from the current WebColorChooserProxyMac object). Will upload a new patch that fixes some of the problems associating to having multiple <input type='color'> elements across multiple tabs.
Created attachment 206727 [details] Patch patch provides correct behavior for multiple color elements on a page and in multiple tabs
Created attachment 206734 [details] Patch merged conflicts with trunk
Comment on attachment 206734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206734&action=review Overall, looks good. Unofficial r=me. > Source/WebKit2/UIProcess/WebPageProxy.cpp:2973 > + else // handles the case where a color picker already exists for the web process. Nit: this is a little strange in terms of style; it's not something I've generally seen in the code base. I would suggest pushing the comment into the else clause and wrapping it in braces instead. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:79 > +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac *)chooser withColor:(NSColor*)color; Style fix: - (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser withColor:(NSColor *)color; > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:81 > +- (void)didChooseColor:(NSColorPanel*)panel; Ditto. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:84 > +- (void)setColor:(NSColor*)color; Ditto. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:78 > + m_panel = [[WKColorPanelMac alloc] initWithChooser:this withColor:nsColor(color)]; This is missing an adoptNS(), causing a leak, right? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:82 > + [m_panel initWithChooser:this withColor:nsColor(color)]; This is, I believe, a bit non-standard? Would it be better to refactor the needed code out to another method? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:89 > +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac *)chooser withColor:(NSColor*)color Spacing is the other way around! - (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser withColor:(NSColor *)color > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:118 > +- (void)windowWillClose:(NSNotification*)notification Please conform to Cocoa style. (NSNotification *) > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:124 > +- (void)didChooseColor:(NSColorPanel*)panel Ditto. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:135 > +- (void)setColor:(NSColor*)color Ditto.
Comment on attachment 206734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206734&action=review >> Source/WebKit2/UIProcess/WebPageProxy.cpp:2973 >> + else // handles the case where a color picker already exists for the web process. > > Nit: this is a little strange in terms of style; it's not something I've generally seen in the code base. > > I would suggest pushing the comment into the else clause and wrapping it in braces instead. Got it. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:79 >> +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac *)chooser withColor:(NSColor*)color; > > Style fix: > - (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser withColor:(NSColor *)color; Ah yes; reversed it. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:78 >> + m_panel = [[WKColorPanelMac alloc] initWithChooser:this withColor:nsColor(color)]; > > This is missing an adoptNS(), causing a leak, right? Yes. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:82 >> + [m_panel initWithChooser:this withColor:nsColor(color)]; > > This is, I believe, a bit non-standard? Would it be better to refactor the needed code out to another method? Yeah, I think it's a bit non-standard; can factor out half of initWithChooser: withColor: to a new method setWithChooser: withColor.
Comment on attachment 206734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206734&action=review >>> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:82 >>> + [m_panel initWithChooser:this withColor:nsColor(color)]; >> >> This is, I believe, a bit non-standard? Would it be better to refactor the needed code out to another method? > > Yeah, I think it's a bit non-standard; can factor out half of initWithChooser: withColor: to a new method setWithChooser: withColor. Actually, I think it shouldn't be refactored because there's two kinds of "reattachment" that can occur and I don't know if it's worthwhile to distinguish between the two here (we can distinguish them in initWithChooser: withColor by checking whether or not the color panel is displayed, [NSColorPanel sharedColorPanelExists]). The first one is when there's a UI color picker already living on another tab and you need to reattach it to an <input type='color'> element on your first tab. In that case, you only need to reset 1) the _chooser object 2) the panel's target 3) the panel's color. The second case is when the UI color picker was closed out of another tab, but you return to your first tab that had already had an m_panel instantiated for it. Because the color picker's been destroyed, it needs to be fully initialized again (i.e. all of initWithChooser: withColor).
Created attachment 206798 [details] Patch fixed style issues
Created attachment 206823 [details] Patch with hack fix for disassociating color picker when navigating away from the page
Comment on attachment 206823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206823&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:4521 > + endColorChooser(); This hack disassociates the color picker once the webpage with the color element is navigated away from. Currently, because WebKit caches forwardback history, if a webpage with a color picker associated is navigated away from and then returned to, the color picker is still associated to it. Sam suggested making it an ActiveDOMObject, which is suspended during caching; however, there's no precedence for an input element to be inherit from ActiveDOMObject, and the ColorInputType element doesn't have access to a ScriptExecutionContext object (i.e. the Document object), which is required to construct an ActiveDOMObject. Will look more into better ways to do this using ActiveDOMObject, but Beth suggested to add a FIXME for the issue for now.
Created attachment 206858 [details] Patch detaches color picker when caching page and wraps reattachColorChooser pathway in Mac preprocessing; still a bug where when a page is loaded from cache, sometimes the initial colors are off
Comment on attachment 206858 [details] Patch Attachment 206858 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1101126 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Created attachment 206879 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Created attachment 206892 [details] Patch fixed bug breaking the gtk build in webpageproxy.messages.in to include ReattachColorChooser only on Mac and when INPUT_TYPE_COLOR is enabled
Created attachment 206906 [details] Patch fixed bug where the first click in the color picker associated to a second color element wouldn't change the value of that color element, and fixed bug so that when javascript is used to change the value of a color element while the color picker is opened, the new color is displayed in the color picker
(In reply to comment #47) > Created an attachment (id=206906) [details] > Patch > > fixed bug where the first click in the color picker associated to a second color element wouldn't change the value of that color element, and fixed bug so that when javascript is used to change the value of a color element while the color picker is opened, the new color is displayed in the color picker Before this patch, there's a side effect that sometimes happens when there's more than one <input type='color'> element on a page. When we click in one color well, and then click in a second color well and select a different color in the color picker, the <input type='color'> value won't change on that click. However, when you click again in the color picker, the <input type='color'> value changes as expected. This is because in WebColorChooserProxyMac::didChooseColor, the if-clause is hit and the early return taken on that first click in the second well. This is because [self setColor:initialColor] is called in initWithChooser, which sets _lastChangedByUser to YES. Because [NSColorPanel sharedColorPanel]'s action is set to didChooseColor, the expected action should be then that didChooseColor is called after setColor, which would set _lastChangedByUser to NO. For some reason, there was a lag behind the didChooseColor that should be called due to switching color wells and instead the didChooseColor from the first click in the color picker is executed first, which is why the color picked is never communicated back to the <input type='color'> element. Probably in another bug, we should test that JavaScript changes to an <input type='color'>'s value sends off the right events and changes the color in an open color picker correctly. Currently, when a color element's value is changed using JavaScript, the change event is not fired. in ColorInputType::setValue, the TextFieldEventBehavior when it's called in this case is a DispatchNoEvent.
Created attachment 206909 [details] Patch fixed bug so that color elements will be loaded from page cache as the last value chosen before caching
Comment on attachment 206909 [details] Patch Attachment 206909 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1103338 New failing tests: svg/batik/text/smallFonts.svg
Created attachment 206937 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Comment on attachment 206909 [details] Patch Attachment 206909 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1097386
Comment on attachment 206909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206909&action=review Please fix the windows builder. > Source/WebCore/html/HTMLInputElement.cpp:1975 > +#if ENABLE(INPUT_TYPE_COLOR) && PLATFORM(MAC) The ENABLE should move to encompass the whole function. > Source/WebCore/html/HTMLInputElement.h:305 > + virtual void documentWillSuspendForPageCache() OVERRIDE; This ought to be wrapped in an ENABLE. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:87 > + // handle the case where the color picker has been closed for an <input type='color'> element. nit: capital-H please > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:142 > + if (_lastChangedByUser) { What is the reason for this check? Can this be called by something else other than the color panel when the user chooses a color? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:148 > + _lastChangedByUser = NO; This is redundant. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:153 > + _lastChangedByUser = YES; The booleans I think are consistent with the last name for this variable. They should be reversed.
Comment on attachment 206909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206909&action=review >> Source/WebCore/html/HTMLInputElement.h:305 >> + virtual void documentWillSuspendForPageCache() OVERRIDE; > > This ought to be wrapped in an ENABLE. Sam gave me some comments in person and one of them was to keep the platform-specific statements out of WebCore (or at least out of ColorInputType.cpp). Thoughts? >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:142 >> + if (_lastChangedByUser) { > > What is the reason for this check? Can this be called by something else other than the color panel when the user chooses a color? WKColorPanelMac::setColor, which is how <input type='color'>'s value is set programmatically. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:148 >> + _lastChangedByUser = NO; > > This is redundant. Got it.
Created attachment 207142 [details] Patch fixed style; removed most mac-specific preprocessing
Comment on attachment 207142 [details] Patch LGTM. Unofficial r=me.
Comment on attachment 207142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207142&action=review This review is ChangeLog Nazi-ism only, will continue reviewing code later... > Source/JavaScriptCore/ChangeLog:5 > + https://bugs.webkit.org/show_bug.cgi?id=61276 > + <rdar://problem/10269922> These can be on the same line (and repeat for each ChangeLog) > Source/WebCore/ChangeLog:10 > + Tests to be added in a later patch. > + No new tests (OOPS!). Can't leave "OOPS!" in the log, the patch will be rejected. Just delete line 10. > Source/WebCore/ChangeLog:24 > + * html/ColorInputType.cpp: > + (WebCore::ColorInputType::handleDOMActivateEvent): Updated to reattach > + the color picker if a color picker has already been shown for an element. > + > + (WebCore::ColorInputType::shouldResetOnDocumentActivation): Added. > + Returns true; needed to detach color chooser when caching a page. I've never seen the style of paragraphing different functions within the same file, and I'm not a fan of it. I'd remove the empty line #22 > Source/WebCore/ChangeLog:27 > + * html/ColorInputType.h: > + * html/HTMLInputElement.cpp: These two lines, however, should be paragraphed! > Source/WebCore/ChangeLog:32 > + (WebCore::HTMLInputElement::documentDidResumeFromPageCache): Updated. > + For <input type='color'>, don't reset the element. > + > + (WebCore::HTMLInputElement::documentWillSuspendForPageCache): Added. > + For <input type='color'>, call detach(). Same here - line #30 grumble! > Source/WebCore/ChangeLog:35 > + * html/HTMLInputElement.h: > + * platform/ColorChooser.h: And same here, these two should be separated! > Source/WebKit2/ChangeLog:8 > + Implemented <input type='color'> on Mac using the > + native color picker. No reason to split this up in multiple lines. > Source/WebKit2/ChangeLog:40 > + (WebKit::WebPageProxy::showColorChooser): Added mac-specific handling > + that allows showColorChooser to be called when first clicking on an > + <input type='color'> element. (Previously, showColorChooser assumed > + that m_colorChooser, which is owned by the web page, is null; > + disallowing multiple <input type='color'> elements on the same web page.) > + > + (WebKit::WebPageProxy::reattachColorChooser): Added. > + Reattached an <input type='color'> element (represented by m_colorChooser) > + to the color picker. > + > + (WebKit::WebPageProxy::didEndColorChooser): Added preprocessing for > + non-Mac platforms that have more color UI objects to clean up. You can guess my comment here. > Source/WebKit2/ChangeLog:56 > + (WebKit::WebColorChooserProxyMac::reattachColorChooser): > + WebColorChooserProxyMac contains a reference to a WKColorPanelMac object > + and is responsible for communicating between the WebProcess and > + the color picker UI. In some descriptions you start on the same line as the method name then wrap. In others the entire description is newlined. I can't tell why the difference.
Comment on attachment 207142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207142&action=review >> Source/WebKit2/ChangeLog:56 >> + the color picker UI. > > In some descriptions you start on the same line as the method name then wrap. In others the entire description is newlined. I can't tell why the difference. In this case because this describes the above functions, not just the reattachColorChooser function.
Created attachment 207155 [details] Patch fixed style in changelogs
Comment on attachment 207155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207155&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.cpp:86 > void WebColorChooser::setSelectedColor(const Color& color) > { > if (!m_page) > return; > + > + if (m_page->activeColorChooser() != this) > + return; In what case would this happen? > LayoutTests/media/track/track-disabled-expected.txt:-6 > -EVENT(loadstart) > -Waiting for the duration of the first cue to elapse. > -The duration of the first cue has elapsed. > - > -END OF TEST > - What? > LayoutTests/media/track/track-disabled.html:-39 > -<!DOCTYPE html> > -<html> > - <head> > - <script src="../media-file.js"></script> > - <script src="../video-test.js"></script> > - <script> > - waitForEvent('loadstart', function () > - { > - var video = document.getElementById('vid'); > - video.textTracks[0].mode = "disabled"; > - > - consoleWrite("Waiting for the duration of the first cue to elapse."); > - video.addEventListener('timeupdate', function (e) > - { > - if (e.target.currentTime < 1) > - return; > - > - // End test after the duration of the first cue to make sure the test > - // doesn't crash during the period this cue would have been > - // rendered if the track was not disabled. > - consoleWrite("The duration of the first cue has elapsed.<br>"); > - endTest(); > - }); > - }); > - > - function start() > - { > - findMediaElement(); > - video.src = findMediaFile('video', '../content/test'); > - } > - > - </script> > - </head> > - <body onload="start()"> > - <video id="vid" autoplay controls> > - <track kind='subtitles' srclang='en' label='English' src='captions-webvtt/captions.vtt' /> > - </video> > - </body> > -</html> What?
Comment on attachment 207155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207155&action=review >> Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.cpp:86 >> + return; > > In what case would this happen? When you have two <input type='color'> elements (i.e. <input type='color' id='c1'> and <input type='color' id='c2'>). If the second element is currently associated to the color picker and is the active color chooser, and the first element is being programatically set (i.e. c1.value = '#000000';). It's not the active color chooser, so it shouldn't change the color in the color picker. >> LayoutTests/media/track/track-disabled-expected.txt:-6 >> - > > What? Hmm, I think this is from another bug I was working on. I must've accidentally deleted it. Sorry; ignore this. >> LayoutTests/media/track/track-disabled.html:-39 >> -</html> > > What? Same.
Created attachment 207257 [details] Patch
Comment on attachment 207257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207257&action=review > Source/WebCore/ChangeLog:21 > + * html/ColorInputType.cpp: > + (WebCore::ColorInputType::handleDOMActivateEvent): Updated to reattach the color picker if a > + color picker has already been shown for an element. > + (WebCore::ColorInputType::shouldResetOnDocumentActivation): Always returns true, needed to > + detach color chooser when caching a page. > + > + * html/ColorInputType.h: line #20 is unnecessary - ColorInputType.h is logically connected to the .cpp > Source/WebCore/ChangeLog:28 > + * html/HTMLInputElement.cpp: > + (WebCore::HTMLInputElement::documentDidResumeFromPageCache): For <input type='color'>, > + don't reset the element. > + (WebCore::HTMLInputElement::documentWillSuspendForPageCache): For <input type='color'>, call detach(). > + > + * html/HTMLInputElement.h: Same here... and same for all Changelogs > Source/WebCore/html/HTMLInputElement.cpp:1978 > + if (!isColorControl()) > + return; > + static_cast<InputType*>(m_inputType.get())->detach(); This should be a cast to ColorInputType* > Source/WebKit2/ChangeLog:40 > + * UIProcess/WebPageProxy.cpp: > + (WebKit::WebPageProxy::showColorChooser): Added mac-specific handling > + that allows showColorChooser to be called when first clicking on an > + <input type='color'> element. (Previously, showColorChooser assumed > + that m_colorChooser, which is owned by the web page, is null; > + disallowing multiple <input type='color'> elements on the same web page.) > + (WebKit::WebPageProxy::reattachColorChooser): Added. Reattaches an > + <input type='color'> element (represented by m_colorChooser) > + to the color picker. > + (WebKit::WebPageProxy::didEndColorChooser): Added preprocessing for > + non-Mac platforms that have more color UI objects to clean up. > + > + * UIProcess/WebPageProxy.h: > + > + * UIProcess/WebPageProxy.messages.in: Updated to include the definition of ReattachColorChooser. And in WK2, we often even leave the .cpp, .h, and .messages.in all paragraphed together (no empty line between them) > Source/WebKit2/UIProcess/WebPageProxy.cpp:2933 > +#if PLATFORM(MAC) > + if (!m_colorChooser) > + m_colorChooser = m_pageClient->createColorChooserProxy(this, initialColor, elementRect); > + else { > + // Handles the case where a color picker already exists for the web process. > + reattachColorChooser(initialColor); > + } > +#else In general, I'm confused as to why this method has a completely different code path for Mac than the code path that already exists in this method. I don't seen any particular reason why the #if PLATFORM(MAC) code is inherently incompatible with how you want Mac to behave. Also, this file is not the correct place for a PLATFORM(MAC) ifdef. > Source/WebKit2/UIProcess/WebPageProxy.cpp:2999 > +#if !PLATFORM(MAC) > m_colorPickerResultListener->invalidate(); > m_colorPickerResultListener = nullptr; > > m_uiClient.hideColorPicker(this); > +#endif // PLATFORM(MAC) Also, this file is not the correct place for a !PLATFORM(MAC) ifdef > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:38 > + > +#if USE(APPKIT) > + Wut? > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:49 > +namespace WebCore { > + > +class Color; > + > +} l46 and l48 are unnecessary > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:38 > + > +#if USE(APPKIT) > + Wut. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:87 > + // Handle the case where the color picker has been closed for an <input type='color'> element. Not sure this comment is useful. > Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:100 > +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser withColor:(NSColor *)color Instead of the WebKit::, please do a `using namespace WebKit` above the @implmentation. > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:669 > PassOwnPtr<ColorChooser> WebChromeClient::createColorChooser(ColorChooserClient* client, const Color& initialColor) > { > +#if !PLATFORM(MAC) > if (m_page->activeColorChooser()) > return nullptr; > - > +#endif > return adoptPtr(new WebColorChooser(m_page, client, initialColor)); > } It seems super peculiar to me that previously this method *always* returned a ColorChooser, and yet your code sometimes does not. That can't be right, can it? Why do other implementations always want a fresh chooser, but this implementation returns null if one already exists? Also, this file is not the correct place for a PLATFORM(MAC) ifdef. > Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.cpp:86 > void WebColorChooser::setSelectedColor(const Color& color) > { > if (!m_page) > return; > + > + if (m_page->activeColorChooser() != this) > + return; It seems wrong that the inactive chooser doesn't get its color set as requested. If a different color chooser is now the active one, shouldn't this one's color still be updated? > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:730 > +PassOwnPtr<ColorChooser> WebChromeClient::createColorChooser(ColorChooserClient* client, const Color& initialColor) > +{ > + notImplemented(); > + return nullptr; > +} This seems wrong,
Comment on attachment 207257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207257&action=review >> Source/WebCore/html/HTMLInputElement.cpp:1978 >> + static_cast<InputType*>(m_inputType.get())->detach(); > > This should be a cast to ColorInputType* ColorInputType::detach() is private. I think the original intention was that detach() would never be directly called. >> Source/WebKit2/UIProcess/WebPageProxy.cpp:2933 >> +#else > > In general, I'm confused as to why this method has a completely different code path for Mac than the code path that already exists in this method. > > I don't seen any particular reason why the #if PLATFORM(MAC) code is inherently incompatible with how you want Mac to behave. > > Also, this file is not the correct place for a PLATFORM(MAC) ifdef. Got it. Bug 119030 will track refactoring this platform specific code. >> Source/WebKit2/UIProcess/WebPageProxy.cpp:2999 >> +#endif // PLATFORM(MAC) > > Also, this file is not the correct place for a !PLATFORM(MAC) ifdef See bug 119030. >> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:38 >> + > > Wut? AppKit is used for NSColorPanel. >> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:669 >> } > > It seems super peculiar to me that previously this method *always* returned a ColorChooser, and yet your code sometimes does not. > That can't be right, can it? > > Why do other implementations always want a fresh chooser, but this implementation returns null if one already exists? > > Also, this file is not the correct place for a PLATFORM(MAC) ifdef. I think the previous implementation only allowed one WebColorChooser per WebPage. Will remove this though. >> Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.cpp:86 >> + return; > > It seems wrong that the inactive chooser doesn't get its color set as requested. > > If a different color chooser is now the active one, shouldn't this one's color still be updated? This is for setting the color in the color picker UI. If JavaScript changes the color of an <input type='color'> that's not the active color element, it will only change the swatch of the color element, not the selected color in the color picker, which is associated to another color element. >> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:730 >> +} > > This seems wrong, This patch tracks implementing <input type='color'> for Mac on WK2; this is WK1. Will change to ASSERT_NOT_REACHED().
Created attachment 207471 [details] Patch
Comment on attachment 207471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207471&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:-4411 > - 85174EC2BCCAF17EAE3F46F8 /* JSSVGGraphicsElement.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 5A91469E8E9F8485C37A2876 /* JSSVGGraphicsElement.cpp */; }; > - 7BE7427381FA906FBB4F0F2C /* JSSVGGraphicsElement.h in Headers */ = {isa = PBXBuildFile; fileRef = 950C4C02BED8936F818E2F99 /* JSSVGGraphicsElement.h */; }; Xcode is still touching stuff it shouldn't. Can we land these in a different patch to appease it (have you reverted these and it's gone ahead and done it again?) and reduce the size of your already large patch. > Source/WebCore/html/HTMLInputElement.cpp:1978 > + static_cast<InputType*>(m_inputType.get())->detach(); Why are you casting an InputType* to an InputType*? > Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:70 > + if (!m_client) > + return; > + > + if (!m_panel) > + return; could merge these > Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:107 > + [panel setShowsAlpha:NO]; Is there a bug about supporting alpha in the future? How does the spec handle that? > Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:147 > + // Check if the color changed programatically or through the color picker. Typo programmatically. And this comment is a little weird (it's a bit "what" and not enough "why"). > Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:153 > + _picker->didChooseColor(WebCore::colorFromNSColor([[panel color] colorUsingColorSpaceName:NSDeviceRGBColorSpace])); colorFromNSColor already shoves the NSColor into DeviceRGB, so you don't need to. Also I'd really like to avoid adding more places where we explicitly name DeviceRGB for ... reasons. So please remove that part.
Comment on attachment 207471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207471&action=review >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-4411 >> - 7BE7427381FA906FBB4F0F2C /* JSSVGGraphicsElement.h in Headers */ = {isa = PBXBuildFile; fileRef = 950C4C02BED8936F818E2F99 /* JSSVGGraphicsElement.h */; }; > > Xcode is still touching stuff it shouldn't. Can we land these in a different patch to appease it (have you reverted these and it's gone ahead and done it again?) and reduce the size of your already large patch. Yes; even after reverting and updating, it's doing this. I'll land a different patch to simply add ColorInputType.cpp to the project (bug 119111). >> Source/WebCore/html/HTMLInputElement.cpp:1978 >> + static_cast<InputType*>(m_inputType.get())->detach(); > > Why are you casting an InputType* to an InputType*? Good catch. Initially I was casting to ColorInputType, but it's detach method is private since it was never meant to be called directly. Will remove casting in next patch. >> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:70 >> + return; > > could merge these Yup. >> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:107 >> + [panel setShowsAlpha:NO]; > > Is there a bug about supporting alpha in the future? How does the spec handle that? No, the HTML5 spec does not support alpha. >> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:147 >> + // Check if the color changed programatically or through the color picker. > > Typo programmatically. And this comment is a little weird (it's a bit "what" and not enough "why"). Got it. This block is executed if setColor is called programatically, which occurs when JavaScript is used to change the value of an <input type='color'> element. If the user changes the color uses the native mac color picker, then this block is skipped. It may make more sense to move the comment down to line 152 and say something like "When user picks a color in the color picker, transmit that selection down to the element to change its value". >> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:153 >> + _picker->didChooseColor(WebCore::colorFromNSColor([[panel color] colorUsingColorSpaceName:NSDeviceRGBColorSpace])); > > colorFromNSColor already shoves the NSColor into DeviceRGB, so you don't need to. Also I'd really like to avoid adding more places where we explicitly name DeviceRGB for ... reasons. So please remove that part. Got it.
Created attachment 207495 [details] Patch
Comment on attachment 207495 [details] Patch Attachment 207495 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1287211
Comment on attachment 207495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207495&action=review > Source/WebCore/html/HTMLInputElement.cpp:1978 > + m_inputType.get()->detach(); No need for .get() (from Brady)
Comment on attachment 207495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207495&action=review > Source/WebCore/html/HTMLInputElement.cpp:1980 > +#if ENABLE(INPUT_TYPE_COLOR) > +void HTMLInputElement::documentWillSuspendForPageCache() > +{ > + if (!isColorControl()) > + return; > + m_inputType.get()->detach(); > +} > +#endif // ENABLE(INPUT_TYPE_COLOR) This should go next to documentDidResumeFromPageCache. > Source/WebCore/html/HTMLInputElement.h:307 > +#if ENABLE(INPUT_TYPE_COLOR) > + virtual void documentWillSuspendForPageCache() OVERRIDE; > +#endif This should go next to documentDidResumeFromPageCache(). > Source/WebKit2/UIProcess/mac/WebColorPickerMac.h:70 > +// A Listener class to act as a event target for NSColorPanel and send Unnecessary capital L. > Source/WebKit2/UIProcess/mac/WebColorPickerMac.h:74 > +@interface WKColorPanelMac : NSObject<NSWindowDelegate> { > + > +@private Unnecessary newline. > Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:134 > +- (void)dealloc > +{ > + NSColorPanel *panel = [NSColorPanel sharedColorPanel]; > + if ([panel delegate] == self) { > + [panel setDelegate:nil]; > + [panel setTarget:nil]; > + [panel setAction:nil]; > + } > + > + [super dealloc]; > +} You should move this to an invalidate method.
Created attachment 207808 [details] Patch
Comment on attachment 207808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207808&action=review > Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:123 > +- (void)setAndShowPicker:(WebColorPickerMac*)picker withColor:(NSColor *)color Moved setup logic here because some various set-up work needs to be done when moving between color elements on the same page and across different tabs (i.e. panel object may be non-null when another color element is clicked but certain set-up like setting the delegate and target needs to be done).
Comment on attachment 207808 [details] Patch Attachment 207808 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1292568
Comment on attachment 207808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207808&action=review I think I know why this broke the Windows build. Everything else looks fine. r- for now, but please upload a new patch to fix the windows build and i'll r+ that. > Source/WTF/wtf/FeatureDefines.h:507 > #if !defined(ENABLE_INPUT_TYPE_COLOR) > -#define ENABLE_INPUT_TYPE_COLOR 0 > +#define ENABLE_INPUT_TYPE_COLOR 1 > #endif I think the reason the windows build broke was because this clause globally enables the feature on all platforms that haven't decided to enable/disable it earlier in FeatureDefines.h You should probably leave this clause alone, and add a new "#define ENABLE_INPUT_TYPE_COLOR 1" clause to the "/* --------- Apple MAC port (not IOS) --------- */" section of the file.
Created attachment 207858 [details] Patch
Comment on attachment 207858 [details] Patch r+ after looking at FeatureDefines.h and blindly assuming Ruth didn't make other changes from the previous patch. :)
Comment on attachment 207858 [details] Patch Clearing flags on attachment: 207858 Committed r153541: <http://trac.webkit.org/changeset/153541>
All reviewed patches have been landed. Closing bug.