247631 - gtk2 nsITheme fixes for checkbox/radio

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Brian Ryner (not reading)]

I originally intended to just fix the double focus ring we're getting on gtk2
for radio and checkbox elements, but I ended up totally rethinking the way we
lay out the elements inside the checkbox container.  The net result of all of
this is that with my patch, we get pixel-perfect spacing of checkbox and radio
controls with every theme included on Fedora Core 1 (with a couple of minor
problems, detailed below).

So here are the main changes I made:

- First, to fix the focus ring problems, I pay much better attention to the
interior_focus property.
- Previously to work around themes which misreport the indicator size, I just
forced the indicator to be 14px minimum and hoped for the best.  What I do now
is I have the indicator (i.e. .checkbox-check) take up all of the padding on all
sides of it.  That is, it extends all the way to the left, top, and bottom of
the container, and all the way over to the label on the right.  This allows the
"slop" to come into the padding between the indicator and the label, which is
what happens on native controls.
- Added a new CHECKBOX/RADIO_LABEL theme part to handle drawing the focus border.
- Some misc cleanups to the gtkdrawing API -- no longer check all out parameters
for NULL (you have to pass in pointers for all out-params), and fold all of the
scrollbar metrics into a struct.

I tried not to regress gtk1 here but I can't promise things didn't shift by a
pixel or two.  I did test that things show up ok when theming gets disabled on
gtk2 (i.e. a bad theme engine).

Now, the minor problems I mentioned:

1. gtk has what I'd consider a minor bug in the way it calculates a height to
request for check/radio buttons.  In particular,

      temp = indicator_size + indicator_spacing * 2;
      requisition->height = MAX (requisition->height, temp) + 2 * (focus_width +
 focus_pad);

In the case where the indicator and its vertical padding are taller than the
requested height of the child, it will take the indicator size and add the focus
border to that.  I don't believe this is optimal for interior focus because it
results in requesting more space than you need.  For example, suppose the
indicator and its padding require 17px height, the child requires 16px height,
and the focus border requires 4px height (2 on top, 2 on bottom).  Using this
formula, it will take the indicator's 17px and add 4px to that to get a height
of 21px.  But in reality it would be quite happy with 20px, since that provides
enough space for the interior focus border around the child.  The net effect
here is that with larger indicator sizes mozilla is "missing" some vertical
padding, but IMO the result often looks better than native controls, where the
extra rows end up outside the interior focus outline.

2. In gtk_default_draw_flat_box, there's a check that causes it to call
gdk_draw_rectangle rather than gtk_style_apply_default_background if
GDK_IS_PIXMAP(window).  So even if there's a bg pixmap it doesn't draw it if
it's rendering into a pixmap.  I can't figure out the point of that check;
things seem to work fine if I remove it (gtk_style_apply_default_background
seems to be able to handle rendering into a pixmap fine).

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Brian Ryner (not reading)]

Oh, also:

I had to add some extra boxes to the xbl bindings for these widgets to get
things to continue to look correct if theming gets disabled.  In particular, the
right border of the .checkbox-check can't be used to supply the padding between
the indicator and the label when theming is disabled, since the border is used
to paint the checkbox itself.  So I had to add an outer box that could supply
the padding, as a border.  Also, I wanted the indicator to stretch to the full
vertical height of the control (-moz-box-align: stretch), but the label needs to
be centered (-moz-box-align: center).  So I made the outer box stretch and put
the label in an inner box which has center alignment.

Comment 3

[[e-mail]]

Is there a particular reason to stick with -moz-box-align styles when dealing
with anonymous nodes? I realize that setting align="center" on a bound element
is probably a bad idea but I thought on anonymous nodes it was acceptable.

Comment 4

[Axel Hecht]

attachment #151193 [details] [diff] [review] reverts quite a few licenses from the css files back to NPL.
Why?

Comment 5

[Brian Ryner (not reading)]

(In reply to comment #3)
> Is there a particular reason to stick with -moz-box-align styles when dealing
> with anonymous nodes? I realize that setting align="center" on a bound element
> is probably a bad idea but I thought on anonymous nodes it was acceptable.

Doesn't really matter to me one way or the other.

(In reply to comment #4)
> attachment #151193 [details] [diff] [review] reverts quite a few licenses from the css files back to NPL.
> Why?

Because I copied the files wholesale from toolkit/skin/gtk2 (now
toolkit/themes/gnomestripe), where they had been copied from toolkit/skin/win
(now toolkit/themes/winstripe).

Comment 6

[Scott MacGregor]

Hey brian, I think this patch broke the thunderbird account wizard on the aviary
1.0 branch. We are failing to get values back from the radio buttons:

See Bug #252655

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 151193 [details] [diff] [review]
patch

a=mkaply