Bug 21751 – Convert checkbox/radio buttons on Mac to new Theme API

• Component: Forms
• Status: RESOLVED FIXED

[Dave Hyatt]

Convert checkbox/radio buttons on Mac to new Theme API

[Dave Hyatt]

Created attachment 24522 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 24522 [details]
Patch

 81     // The size here is in zoomed coordinates already.  If a new size is returned, it also needs to be in zoomed coordinates.
 82     virtual IntSize controlSize(ControlPart, const Font&, const IntSize& size, float zoomFactor) const { return size; }

Should we call the parameter "zoomedSize"?

 91     // Method for painting a control.  The rect is in zoomed coordinates.
 92     virtual void paint(ControlPart, ControlStates, GraphicsContext*, const IntRect&, float zoomFactor, ScrollView*) const { };

 96     // The rect passed in is in zoomed coordinates, so the inflation should take that into account and make sure the inflation
 97     // amount is also scaled by the zoomFactor.
 98     virtual void inflateControlPaintRect(ControlPart, ControlStates, IntRect&, float zoomFactor) const { }

Should we call the IntRect parameter in these two functions zoomedRect?

 62 static IntSize sizeFromFont(const Font& font, const IntSize& size, float zoomFactor, const IntSize* sizes)

It would be useful to document which of the size parameter, the sizes parameter, and the return value should be in zoomed coordinates, either with a comment or by making the parameter names clearer or both.

 75 static void setControlSize(NSCell* cell, const IntSize* sizes, const IntSize& minSize, float zoomLevel)

Ditto

 78     if (minSize.width() >= static_cast<int>(sizes[NSRegularControlSize].width() * zoomLevel) &&
 79         minSize.height() >= static_cast<int>(sizes[NSRegularControlSize].height() * zoomLevel))
 80         size = NSRegularControlSize;
 81     else if (minSize.width() >= static_cast<int>(sizes[NSSmallControlSize].width() * zoomLevel) &&
 82              minSize.height() >= static_cast<int>(sizes[NSSmallControlSize].height() * zoomLevel))
 83         size = NSSmallControlSize;

Is truncating really the right behavior here? Should we be rounding instead?

 96     bool pressed = (states & PressedState);

I don't think the parentheses here (and on other similar lines) improve clarity.

 116     if (oldIndeterminate != indeterminate) {
 117         [cell setState:indeterminate ? NSMixedState : (checked ? NSOnState : NSOffState)];
 118         return;
 119     }
 120     bool oldChecked = [cell state] == NSOnState;
 121     if (checked != oldChecked)
 122         [cell setState:checked ? NSOnState : NSOffState];

I think you could combine this into a single call to setState:

if (oldIndeterminate != indeterminate || oldChecked != checked)
    [cell setState:indeterminate ? NSMixedState : (checked ? NSOnState : NSOffState)];

 128 static IntRect inflateRect(const IntRect& r, const IntSize& size, const int* margins, float zoomLevel)

 165 static IntSize checkboxSize(ControlPart part, const Font& font, const IntSize& size, float zoomFactor)

 175 static NSButtonCell* checkbox(ControlStates states, const IntRect& rect, float zoomFactor)

 195 static void paintCheckbox(ControlStates states, GraphicsContext* context, const IntRect& rect, float zoomFactor, ScrollView* scrollView)

 251 static NSButtonCell* radio(ControlStates states, const IntRect& rect, float zoomFactor)

 270 static void paintRadio(ControlStates states, GraphicsContext* context, const IntRect& rect, float zoomFactor, ScrollView* scrollView)

Ditto for these functions re: which parameters/return value are zoomed.

 177     static RetainPtr<NSButtonCell> checkboxCell;

I think we should just use bare (leaked) pointers for all these static NSCell objects.

Is it possible to share code between paintCheckbox and paintRadio? It sure seems like it!

 230 static const int* radioMargins(NSControlSize controlSize)
 231 {
 232     static const int margins[3][4] =
 233     {
 234         { 2, 2, 4, 2 },
 235         { 3, 2, 3, 2 },
 236         { 1, 0, 2, 0 },
 237     };
 238     return margins[controlSize];
 239 }

Should margins instead be a ControlBox[3]?

 299 int ThemeMac::baselinePositionAdjustment(ControlPart part) const
 300 {
 301     if (part == CheckboxPart || part == RadioPart)
 302         return -2;

Should you be using cUnchangedControlSize instead of -2 here?

 345 void ThemeMac::inflateControlPaintRect(ControlPart part, ControlStates states, IntRect& rect, float zoomFactor) const
 346 {
 347     switch (part) {
 348         case CheckboxPart: {
 349             // We inflate the rect as needed to account for padding included in the cell to accommodate the checkbox
 350             // shadow" and the check.  We don't consider this part of the bounds of the control in WebKit.
 351             NSCell* cell = checkbox(states, rect, zoomFactor);
 352             NSControlSize controlSize = [cell controlSize];
 353             IntSize size = checkboxSizes()[controlSize];
 354             size.setHeight(size.height() * zoomFactor);
 355             size.setWidth(size.width() * zoomFactor);
 356             rect = inflateRect(rect, size, checkboxMargins(controlSize), zoomFactor);
 357             break;
 358         }
 359         case RadioPart: {
 360             // We inflate the rect as needed to account for padding included in the cell to accommodate the radio button
 361             // shadow".  We don't consider this part of the bounds of the control in WebKit.
 362             NSCell* cell = radio(states, rect, zoomFactor);
 363             NSControlSize controlSize = [cell controlSize];
 364             IntSize size = radioSizes()[controlSize];
 365             size.setHeight(size.height() * zoomFactor);
 366             size.setWidth(size.width() * zoomFactor);
 367             rect = inflateRect(rect, size, radioMargins(controlSize), zoomFactor);
 368             break;
 369         }
 370         default:
 371             break;
 372     }
 373 }

Can we share more code here?

 105         IntSize controlSize = m_theme->controlSize(part, style->font(), IntSize(lengthToInt(style->width()), lengthToInt(style->height())), style->effectiveZoom());
 106         if (controlSize.width() != cUnchangedControlSize)
 107             style->setWidth(intToLength(controlSize.width()));
 108         if (controlSize.height() != cUnchangedControlSize)
 109             style->setHeight(intToLength(controlSize.height()));

I wonder if it would be clearer for controlSize() to have two boolean out parmeters, widthChanged and heightChanged, rather than using these magic values.

374442 // FIXME: It would be nice to set this state on the RenderObjects instead of
375443 // having to dig up to the FocusController at paint time.
 444 ControlStates RenderTheme::controlStatesForRenderer(const RenderObject* o) const

The FIXME is now misplaced.

@@ bool RenderTheme::isFocused(const Render
418510         return false;
419511     Document* document = node->document();
420512     Frame* frame = document->frame();
421      return node == document->focusedNode() && frame && frame->selection()->isFocusedAndActive();
 513     return node == document->focusedNode() && frame && frame->selection()->isFocusedAndActive() && o->style()->outlineStyleIsAuto();

It seems wrong for isFocused to return false just because someone set "outline:none" on this element. Should callers of isFocused instead be responsible for checking this? Or maybe we should rename the function to isShowingFocus?

r=me

[Dave Hyatt]

Created attachment 24525 [details]
Patch #2

[Adam Roben (:aroben)]

Comment on attachment 24525 [details]
Patch #2

 243     Length width() const { return m_width; }
 244     Length height() const { return m_height; }
 245     
 246     void setWidth(Length w) { m_width = w; }
 247     void setHeight(Length h) { m_height = h; }

Should we be returning/accepting const Length& here, like we do for the constructor?

 116     if (oldIndeterminate != indeterminate || checked != oldChecked) {
 117         [cell setState:indeterminate ? NSMixedState : (checked ? NSOnState : NSOffState)];
 118         return;
 119     }

The return statement doesn't seem useful here.

I still think it would be good to share more code between checkboxes and radio buttons. At least paintCheckbox/paintRadio seem trivial to merge. Even a FIXME would be good!

@@ bool RenderTheme::isFocused(const Render
418489         return false;
419490     Document* document = node->document();
420491     Frame* frame = document->frame();
421      return node == document->focusedNode() && frame && frame->selection()->isFocusedAndActive();
 492     return node == document->focusedNode() && frame && frame->selection()->isFocusedAndActive() && o->style()->outlineStyleIsAuto();

I think you meant to undo this change.

r=me

[Dave Hyatt]

Fixed in r37731.