Color wells should appear pressed when presenting a color picker.
Created attachment 346921 [details] Patch
Comment on attachment 346921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346921&action=review > Source/WebCore/platform/mac/ThemeMac.mm:714 > + NSButtonCell *buttonCell = button(ColorWellPart, controlStates, IntSize(zoomedRect.size()), zoomFactor); This button is kept statically, right? Who unsets highlighted? > Source/WebKit/UIProcess/WebColorPicker.cpp:48 > + m_client = 0; nullptr? > Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:98 > + if (m_colorPickerUI) { Why doesn't this just call endPicker?
(In reply to Tim Horton from comment #2) > Comment on attachment 346921 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346921&action=review > > > Source/WebCore/platform/mac/ThemeMac.mm:714 > > + NSButtonCell *buttonCell = button(ColorWellPart, controlStates, IntSize(zoomedRect.size()), zoomFactor); > > This button is kept statically, right? Who unsets highlighted? Each time a button cell is created, setUpButtonCell is called. This in turn calls updateStates, which sets the correct highlighted value given the state. Both of those methods are in ThemeMac. Now that I think about it, it might make sense to move the statement that sets the highlight from paintColorWell, to updateStates. > > > Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:98 > > + if (m_colorPickerUI) { > > Why doesn't this just call endPicker? There are two ways this destructor can be called. One is by calling endPicker() which eventually sets m_colorPicker to nullptr in WebPageProxy. If we had endPicker() in the destructor we would be calling endPicker() in an infinite loop. The other way for the destructor to be called is for a new color picker to be created while the previous one is still active. For example, when clicking on another color input while one is already active. In this case, endPicker() may not be called before the new color picker is created - which is why we invalidate m_colorPickerUI.
(In reply to Aditya Keerthi from comment #3) > (In reply to Tim Horton from comment #2) > > Comment on attachment 346921 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=346921&action=review > > > > > Source/WebCore/platform/mac/ThemeMac.mm:714 > > > + NSButtonCell *buttonCell = button(ColorWellPart, controlStates, IntSize(zoomedRect.size()), zoomFactor); > > > > This button is kept statically, right? Who unsets highlighted? > > Each time a button cell is created, setUpButtonCell is called. This in turn > calls updateStates, which sets the correct highlighted value given the > state. Both of those methods are in ThemeMac. Now that I think about it, it > might make sense to move the statement that sets the highlight from > paintColorWell, to updateStates. Yes please. Also fix the build.
Created attachment 346926 [details] Patch
Comment on attachment 346926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346926&action=review > Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:55 > +@protocol WKPopoverColorWellDelegate<NSObject> You should check, but I think we usually put a space before brackets for protocol conformance > Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:277 > + [[NSColorPanel sharedColorPanel] setDelegate:nil]; Why do you use dot notation for delegate on one line and bracket notation on the next? DOTS!
Created attachment 346987 [details] Patch for landing
Comment on attachment 346987 [details] Patch for landing Clearing flags on attachment: 346987 Committed r234788: <https://trac.webkit.org/changeset/234788>
<rdar://problem/43391628>