Labels of radio buttons and checkboxes are drawn too low

VERIFIED FIXED

Status

()

defect
P2
major
VERIFIED FIXED
12 years ago
4 years ago

People

(Reporter: LpSolit, Assigned: dbaron)

Tracking

(Blocks 1 bug, {regression})

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 1 obsolete attachment)

522 bytes, text/html
Details
602 bytes, text/html
Details
13.74 KB, image/png
Details
10.39 KB, text/html; charset=UTF-8
Details
107.00 KB, image/png
Details
10.41 KB, text/html; charset=UTF-8
Details
10.01 KB, text/html; charset=UTF-8
Details
4.88 KB, patch
ventnor.bugzilla
: review+
Details | Diff | Splinter Review
5.22 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
132.42 KB, image/jpeg
Details
110.07 KB, image/jpeg
Details
Reporter

Description

12 years ago
Posted file 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.
Flags: blocking1.9?
When did this regress?  Does it happen if you style those controls with "-moz-appearance:none" ?
Reporter

Comment 2

12 years ago
bz, see yourself. :)
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?
Reporter

Comment 4

12 years ago
Posted image screenshots
The first half is with no style, the 2nd half with -moz-appearance: none.
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.
Blocks: 329846

Comment 6

12 years ago
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

12 years ago
This seems like normal priority rather than major to me...
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...
Reporter

Comment 9

12 years ago
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

12 years ago
(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?
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.
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?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Reporter

Comment 13

12 years ago
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.

Updated

12 years ago
Duplicate of this bug: 405797
Should checkboxes and radios be vertical-align middle instead of baseline?  What do other browsers do?
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.
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

11 years ago
What about vertical-align: middle?
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.
Priority: P4 → P3
Flags: wanted-next+
Flags: blocking1.9-
Flags: wanted-next+
Flags: blocking1.9-
Flags: blocking1.9+
Priority: P3 → P2
Assignee: nobody → dbaron
Posted file more examples
Oops, didn't switch the first row from using -moz-appearance manually to a class.
Attachment #307149 - Attachment is obsolete: true
(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.
> I'm not sure I'm seeing this.

Are you seeing something pretty different from this screenshot?
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.
Our UA style sheet has weird margins on both radio buttons and checkboxes.  These don't get along particularly well with native themes.
Flags: tracking1.9+
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.
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).
Attachment #307850 - Flags: superreview?(roc)
Attachment #307850 - Flags: review?(ventnor.bugzilla)
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.)
Attachment #307853 - Flags: superreview?(bzbarsky)
Attachment #307853 - Flags: review?(bzbarsky)
And I'd be interested in opinions on whether we ought to do (3).

Comment 31

11 years ago
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.
Attachment #307850 - Flags: review?(ventnor.bugzilla) → review+
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.
Attachment #307853 - Flags: superreview?(bzbarsky)
Attachment #307853 - Flags: superreview+
Attachment #307853 - Flags: review?(bzbarsky)
Attachment #307853 - Flags: review+
(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 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.
Attachment #307850 - Flags: superreview?(roc) → superreview+
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

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

11 years ago
On second thought, yeah, forget my comment. I'd be OK with it as is.
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Depends on: 421734
Depends on: 424873
Reporter

Comment 39

11 years ago
It's still not great, but looks better than before. Verified using Fx3b5.
Status: RESOLVED → VERIFIED

Comment 41

11 years ago
Default vertical-align in safari still looks much better than in firefox 3
So this was a Linux bug; it may well be worth filing a separate Mac bug.
(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. 

Depends on: 537860
Depends on: 444870
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.