402940 - Labels of radio buttons and checkboxes are drawn too low

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Frédéric Buclin]

Attachment: testcase

See the testcase attached, labels for radio buttons and checkboxes are too low compared to the control. This problem doesn't exist for text fields.

Comment 1

[Boris Zbarsky [:bzbarsky]]

When did this regress?  Does it happen if you style those controls with "-moz-appearance:none" ?

Comment 2

[Frédéric Buclin]

Attachment: testcase with -moz-appearance:none

bz, see yourself. :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

See what for myself?  Checkbox looks fine to be in both testcases.  Radio looks OK without moz-apperance, not so great with.

Perhaps you want to post your screenshots of both?

Comment 4

[Frédéric Buclin]

Attachment: screenshots

The first half is with no style, the 2nd half with -moz-appearance: none.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, that's definitely quite different from what I see.  Sounds like the native theming code needs to give us a better baseline, or something.  Right now we use the bottom of the frame rect, and have no way I see to tell that the theme is painting everything a lot higher than that.

Comment 6

[Michael Ventnor]

I don't see a way to specify a good baseline in the native theme code. We give that spacing due to the indicator-spacing property of GTK. Some themes like Ubuntu's like to have the checkbox tick stick out a little bit, so we need to give that spacing so we don't clip that bit of the checkbox tick.

We also need that spacing to give a reasonable gap between a toggle and its label in XUL.

I'll have another look soon.

Comment 7

[Michael Ventnor]

This seems like normal priority rather than major to me...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Indicator-spacing applies all around, not just on left and right?  Would it make sense to make the descent equal to indicator-spacing?  We'd need another API on nsITheme to get this information, of course...

Comment 9

[Frédéric Buclin]

The reason I set it to major is that when you have several radio buttons on several consecutive lines (as for knobs in show_bug.cgi on b.m.o), labels look more or less mid-way between their real radio button and the next one. Really not good, IMO.

Comment 10

[Michael Ventnor]

(In reply to comment #8)
> Indicator-spacing applies all around, not just on left and right?  Would it
> make sense to make the descent equal to indicator-spacing?  We'd need another
> API on nsITheme to get this information, of course...
> 

What part of layout determines the descent?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Reflow().  It's one of the metrics that get set in aDesiredSize.

In particular, both checkbox and radio (and nothing else) go through nsFormControlFrame::Reflow, so that would be a good spot to get the number.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems to me we could subtract indicator-spacing from the control's border-rect and add it to the control's overflow-rect, at least in the vertical direction.

When did this regress?

Comment 13

[Frédéric Buclin]

As reported in bug 329846 comment 35, bug 329846 seems to be the culprit of this regression. Note the existence of a similar bug on Windows, bug 192896. Maybe a dupe despite bug 192896 has been filed before the checkin of bug 329846.

Comment 15

[David Baron :dbaron:]

Should checkboxes and radios be vertical-align middle instead of baseline?  What do other browsers do?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Actually, right now checkboxes and radios are vertical-align:text-bottom.  The baseline they report is pretty bogus; vertical-aligning them to baseline would put them above where they are now with "vertical-align: top"...

Opera appears to do vertical-align:middle the checkboxes, and its checkboxes have a few px less vertical crap around them than ours do.x

For what it's worth, setting vertical-align:middle on our radios makes them be way too low (both themed and unthemed): the halfway point (which we align with the middle of the text) is about at the top of the radio circle.

It would certainly make sense to use vertical-align: middle if we fixed the baselines to be sane.

Comment 17

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I think this should be P3 or higher.  I had to stop using native theming for radio buttons on trunk because I kept picking the wrong radio buttons in bugzilla due to this bug...

Comment 18

[u294409]

What about vertical-align: middle?

Comment 19

[David Baron :dbaron:]

OK, bumping to P3.

I'd also note I remember discussing this with Josh a while ago; it's possible we have a Mac-only fix (or hack) somewhere rather than a general one.

Comment 20

[David Baron :dbaron:]

Attachment: more examples

Comment 21

[David Baron :dbaron:]

Attachment: more examples

Oops, didn't switch the first row from using -moz-appearance manually to a class.

Comment 22

[David Baron :dbaron:]

(In reply to comment #16)
> Actually, right now checkboxes and radios are vertical-align:text-bottom.

radios are baseline, checkboxes are text-bottom.

Opera does middle for both; IE and Safari do baseline for both.

> For what it's worth, setting vertical-align:middle on our radios makes them be
> way too low (both themed and unthemed): the halfway point (which we align with
> the middle of the text) is about at the top of the radio circle.

I'm not sure I'm seeing this.  It looks pretty close to me, although I could imagine it being a pixel or two off.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Screenshot of what I see on the "more examples" testcase

> I'm not sure I'm seeing this.

Are you seeing something pretty different from this screenshot?

Comment 24

[David Baron :dbaron:]

Mine's a little different -- the difference between 'top' and 'bottom' isn't as dramatic for me (especially for the native case), but it is there, and that's probably a better place to look than eyeing middle.

Comment 25

[David Baron :dbaron:]

Attachment: more examples, this time with explicit zero margins

Our UA style sheet has weird margins on both radio buttons and checkboxes.  These don't get along particularly well with native themes.

Comment 26

[David Baron :dbaron:]

So my current thinking is:
  1. we should definitely move this indicator spacing into GetWidgetOverflow

  2. we should probably make radios and checkboxes implement the necessary methods so they have a baseline that's at their bottom border edge

  3. we should probably add a boolean parameter to GetWidgetOverflow that says that the widget's margin should not be allowed to be smaller than the overflow.  (But if we do this without doing (2), it would undo (1).)

  4. we should probably change checkboxes from text-bottom (which nobody does) to baseline, and maybe change both from baseline to middle (which would match only Opera, and might be a little risky for this point in the cycle)

But I want to write a few more testcases and think about this a little more.

Comment 27

[David Baron :dbaron:]

Attachment: examples of items packed together

Comment 28

[David Baron :dbaron:]

Attachment: patch to put indicator spacing in widget overflow

This does (1) above.  I think the changes to the clip rect in DrawWidgetBackground are correct; otherwise we'd set the clip rect to hide the overflow, which matters for the focus rect drawing code in moz_gtk_toggle_paint.  Hopefully the extra pixel of clip rect won't cause any problem for scrollbar thumbs (the only other thing with nonzero GetExtraSizeForWidget).

Comment 29

[David Baron :dbaron:]

Attachment: give radios and checkboxes a meaningful baseline

This does (2) above and also part of (4) (using baseline rather than text-bottom).

This gives radios and checkboxes a meaningful baseline (bottom content edge), and prophylactically (since forms.css has the same with !important) makes the native theme implementations insist on zero padding so that this produces sensible results.  That said, I think that's sort of moot since to do that we'd need to change the native theme API so that the thing drawn by the theme is only in the content area, at least optionally.  So maybe it's not worth the bother and I just just go back to border edge.  Thoughts?

(I felt it was sort of a tossup between bottom border edge or bottom content edge, but I picked content, in case we let radios/checkboxes have padding/border in the future.)

Comment 30

[David Baron :dbaron:]

And I'd be interested in opinions on whether we ought to do (3).

Comment 31

[Michael Ventnor]

Comment on attachment 307850 [details] [diff] [review]
patch to put indicator spacing in widget overflow

>+#include "nsDebug.h"

...

>+    /* centered vertically within the rect */
>+    NS_ASSERTION(rect->height == indicator_size &&
>+                 rect->width == indicator_size,
>+                 "GetMinimumWidgetSize was ignored");

Don't do this. Webkit-GTK uses this code because it is quite standard and portable and thats a virtue I'd like to keep. I don't think this is a serious enough issue to warrant an assertion and the introduction of ns* headers, the worst that can happen is extra whitespace in the frame, AFAICT.

With that removed, and assuming this works, r=me.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 307853 [details] [diff] [review]
give radios and checkboxes a meaningful baseline

Makes sense.  I do think the content edge is a reasonable place to put the baseline.

Comment 33

[David Baron :dbaron:]

(In reply to comment #30)
> And I'd be interested in opinions on whether we ought to do (3).

I think I'm leaning against, now.  And Boris seems to agree that we shouldn't.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 307850 [details] [diff] [review]
patch to put indicator spacing in widget overflow

+  gfxRect gfx_clip = ConvertToGfxRect(drawingRect - drawingRect.TopLeft(), p2a);

For this kind of thing, I prefer nsRect(nsPoint(0, 0), drawingRect.Size())

As for nsDebug.h, it'd be pretty easy for Webkit to work around this by providing their own nsDebug.h with a definition for NS_ASSERTION. That probably makes more sense for everyone than just leaving possibly useful diagnostics on the floor.

Comment 35

[David Baron :dbaron:]

How about, as a compromise:

#ifdef MOZILLA_CLIENT
#include "nsDebug.h"
#define GTKDRAWING_ASSERT(e_) NS_ASSERTION(e_, #e_)
#else
#include <assert.h>
#define GTKDRAWING_ASSERT(e_) assert(e_)
#endif

?

Comment 36

[David Baron :dbaron:]

(In reply to comment #34)
> (From update of attachment 307850 [details] [diff] [review])
> +  gfxRect gfx_clip = ConvertToGfxRect(drawingRect - drawingRect.TopLeft(),
> p2a);
> 
> For this kind of thing, I prefer nsRect(nsPoint(0, 0), drawingRect.Size())

I actually was going to write that, but I decided not to because I wanted to leave the parallelism between the two adjacent lines, which I think is useful.

Comment 37

[Michael Ventnor]

On second thought, yeah, forget my comment. I'd be OK with it as is.

Comment 38

[David Baron :dbaron:]

OK, both patches checked in to trunk, 2008-03-07 09:57/58 -0800.

Filed bug 421543 as a followup on considering vertical-align:middle in the future.

Comment 39

[Frédéric Buclin]

It's still not great, but looks better than before. Verified using Fx3b5.

Comment 40

[Paweł Kondzior]

Attachment: Firefox 3 testcase screenshot

Comment 41

[Paweł Kondzior]

Attachment: Safari testcase screenshot

Default vertical-align in safari still looks much better than in firefox 3

Comment 42

[David Baron :dbaron:]

So this was a Linux bug; it may well be worth filing a separate Mac bug.

Comment 43

[philippe (part-time)]

(In reply to comment #42)
> So this was a Linux bug; it may well be worth filing a separate Mac bug.
>

see also bug 448251. 

Comment 44

[editor.buzzfeed]

Make GTK2 checkbox and radio widgets put their indicator spacing in GetWidgetOverflow rather than inside their size.  b=402940

diff --git a/widget/src/gtk2/gtk2drawing.c b/widget/src/gtk2/gtk2drawing.c
--- a/widget/src/gtk2/gtk2drawing.c
+++ b/widget/src/gtk2/gtk2drawing.c
@@ -47,6 +47,7 @@
 #include <gdk/gdkprivate.h>
 #include <string.h>
 #include "gtkdrawing.h"
+#include "nsDebug.h"
 
 #include <math.h>
 
@@ -797,6 +798,7 @@ moz_gtk_toggle_paint(GdkDrawable* drawab
     GtkShadowType shadow_type = (selected)?GTK_SHADOW_IN:GTK_SHADOW_OUT;
     gint indicator_size, indicator_spacing;
     gint x, y, width, height;
+    gint focus_x, focus_y, focus_width, focus_height;
     GtkWidget *w;
     GtkStyle *style;
 
@@ -808,11 +810,19 @@ moz_gtk_toggle_paint(GdkDrawable* drawab
         w =  / https://www.rebelmouse.com/ps3mouse/ ;
     }
 
-    /* offset by indicator_spacing, and centered vertically within the rect */
-    x = rect->x + indicator_spacing;
-    y = rect->y + (rect->height - indicator_size) / 2;
+    /* centered vertically within the rect */
+    NS_ASSERTION(rect->height == indicator_size &&
+                 rect->width == indicator_size,
+                 "GetMinimumWidgetSize was ignored");
+    x = rect->x;
+    y = rect->y;
     width = indicator_size;
     height = indicator_size;
+
+    focus_x = x - indicator_spacing;
+    focus_y = y - indicator_spacing;
+    focus_width = width + 2 * indicator_spacing;
+    focus_height = height + 2 * indicator_spacing;
   
     style = w->style;
     TSOffsetStyleGCs(style, x, y);
@@ -827,8 +837,8 @@ moz_gtk_toggle_paint(GdkDrawable* drawab
                          width, height);
         if (state->focused) {
             gtk_paint_focus(style, drawable, GTK_STATE_ACTIVE, cliprect,
-                            gRadiobuttonWidget, "radiobutton", rect->x, rect->y,
-                            rect->width, rect->height);
+                            gRadiobuttonWidget, "radiobutton", focus_x, focus_y,
+                            focus_width, focus_height);
         }
     }
     else {
@@ -836,8 +846,8 @@ moz_gtk_toggle_paint(GdkDrawable* drawab
                         gCheckboxWidget, "checkbutton", x, y, width, height);
         if (state->focused) {
             gtk_paint_focus(style, drawable, GTK_STATE_ACTIVE, cliprect,
-                            gCheckboxWidget, "checkbutton", rect->x, rect->y,
-                            rect->width, rect->height);
+                            gCheckboxWidget, "checkbutton", focus_x, focus_y,
+                            focus_width, focus_height);
         }
     }
 
diff --git a/widget/src/gtk2/nsNativeThemeGTK.cpp b/widget/src/gtk2/nsNativeThemeGTK.cpp
--- a/widget/src/gtk2/nsNativeThemeGTK.cpp
+++ b/widget/src/gtk2/nsNativeThemeGTK.cpp
@@ -685,6 +685,27 @@ GetExtraSizeForWidget(PRUint8 aWidgetTyp
   case NS_THEME_SCROLLBAR_THUMB_HORIZONTAL:
     aExtra->left = aExtra->right = 1;
     return PR_TRUE;
+
+  // Include the indicator spacing (the padding around the control).
+  case NS_THEME_CHECKBOX:
+  case NS_THEME_CHECKBOX_SMALL:
+  case NS_THEME_RADIO:
+  case NS_THEME_RADIO_SMALL:
+    {
+      gint indicator_size, indicator_spacing;
+
+      if (IsCheckboxWidgetType(aWidgetType)) {  / https://www.penflip.com/trends /

+        moz_gtk_checkbox_get_metrics(&indicator_size, &indicator_spacing);
+      } else {
+        moz_gtk_radio_get_metrics(&indicator_size, &indicator_spacing);
+      }
+
+      aExtra->top = indicator_spacing;
+      aExtra->right = indicator_spacing;
+      aExtra->bottom = indicator_spacing;
+      aExtra->left = indicator_spacing;
+      return PR_TRUE;
+    }
   default: / http://blog.saved.io/

     return PR_FALSE;
   }
@@ -777,7 +798,7 @@ nsNativeThemeGTK::DrawWidgetBackground(n
   // need to draw outside this rect if you don't feel like it!"
   GdkRectangle gdk_rect, gdk_clip;
   gfxRect gfx_rect = ConvertToGfxRect(aRect - drawingRect.TopLeft(), p2a);
-  gfxRect gfx_clip = ConvertToGfxRect(aClipRect - drawingRect.TopLeft(), p2a);
+  gfxRect gfx_clip = ConvertToGfxRect(drawingRect - drawingRect.TopLeft(), p2a);
   if (ctx->http://contentcurator.withknown.com/
 UserToDevicePixelSnapped(gfx_rect) &&
       ctx->UserToDevicePixelSnapped(gfx_clip)) {
     gfxPoint currentTranslation = current.GetTranslation();
@@ -786,7 +807,7 @@ nsNativeThemeGTK::DrawWidgetBackground(n
   }
   else {
     gdk_rect = ConvertToGdkRect(aRect - drawingRect.TopLeft(), p2a);
-    gdk_clip = ConvertToGdkRect(aClipRect - drawingRect.TopLeft(), p2a);
+    gdk_clip = ConvertToGdkRect(drawingRect - drawingRect.TopLeft(), p2a);
   }
   ThemeRenderer renderer(state, gtkWidgetType, flags, direction, gdk_rect, gdk_clip);
 
@@ -1088,8 +1109,8 @@ nsNativeThemeGTK::GetMinimumWidgetSize(n
       }
 
       /    */ http://www.mommyliciousmaternity.com/collections/plus-size-maternity-dresses *// Include space for the indicator and the padding around it.
-      aResult->width = indicator_size + 2 * indicator_spacing;
-      aResult->height = indicator_size + 2 * indicator_spacing;
+      aResult->width = indicator_size;
+      aResult->height = indicator_size;
       *aIsOverridable = PR_FALSE;
     }
     break;