Bug 188477 – [macOS] Color wells should appear pressed when presenting a color picker

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

[Aditya Keerthi]

Color wells should appear pressed when presenting a color picker.

[Aditya Keerthi]

Created attachment 346921 [details]
Patch

[Tim Horton]

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?

[Aditya Keerthi]

(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.

[Tim Horton]

(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.

[Aditya Keerthi]

Created attachment 346926 [details]
Patch

[Tim Horton]

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!

[Aditya Keerthi]

Created attachment 346987 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 346987 [details]
Patch for landing

Clearing flags on attachment: 346987

Committed r234788: <https://trac.webkit.org/changeset/234788>

[Radar WebKit Bug Importer]

<rdar://problem/43391628>