Closed Bug 475535 Opened 11 years ago Closed 11 years ago

non-native radio buttons aren't drawn checked.

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: sylvain.pasche, Assigned: zwol)

References

()

Details

(Keywords: regression, verified1.9.1)

Attachments

(5 files, 2 obsolete files)

See URL, I tested on Windows and Linux.

works:20090107 / 670a3b50dfe0
fails:20090108 / 28488df9e75e

range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=670a3b50dfe0&tochange=28488df9e75e
Flags: blocking1.9.2?
Attached patch Like so? (obsolete) — Splinter Review
Don't deflate the border for -moz-box-sizing:border-box.
So regression from bug 456219?
Blocks: 456219
Yes, reversing the "rev 11" patch in that bug (with some tweaks to make it
compile) fixes the testcase.
What's the default for -moz-box-sizing?  Not border-box, I presume, or the patch in comment 1 would break every use of -moz-background-clip.  The thing I don't understand is, the code before the patch in bug 456219 was not looking at -moz-box-sizing either, so if that's the right fix, it should have been broken before.
The default is content-box.
Duplicate of this bug: 480719
This is a regression from a 1.9.1 blocker, so needs to block 1.9.1.
Flags: blocking1.9.1?
Assignee: nobody → zweinberg
Flags: blocking1.9.1? → blocking1.9.1+
Attachment #359168 - Flags: superreview?(roc)
Attachment #359168 - Flags: review?(roc)
Comment on attachment 359168 [details] [diff] [review]
Like so?

I'm pretty sure this is the correct fix.  robert: review?
I don't understand why this patch fixes the bug. Can someone explain what the bug is and why this fixes it?
I believe the bug is that a CSS background image is used to draw the checkmark on a checked non-native radio button, but the patch in bug 456219 caused the area occupied by the background image to be zero, because the radio button has (for whatever reason) -moz-background-clip: padding, -moz-box-sizing: border-box, and a padding area of size zero.

Thinking about that a bit more, maybe this is not the right patch; maybe the right patch is to change the radio button styling to not use -moz-background-clip:padding.
If I'm not mistaken, the checkmark is drawn in the call to nsCSSRendering::PaintBackgroundWithSC from nsGfxRadioControlFrame::PaintRadioButtonFromStyle, which uses the style data for -moz-radio:

*|*::-moz-radio {
  width: 4px;
  height: 4px;
  background-color: -moz-FieldText ! important;
  -moz-border-radius: 3px;
}

I don't see why that fails to paint.
This is pretty darn subtle.

There is no anonymous frame for the checkmark; PaintRadioButtonFromStyle calls PaintBackgroundWithSC and PaintBorder using the radio button itself as the frame, but style structs derived from the special style declaration you quoted.  The nsStyleBorder passed in has an actual border width of zero.  IsSolidBorder treats this as a solid border, so PaintBackgroundWithSC tells SetupBackgroundClip to reduce the area painted to the padding-box.  SetupBackgroundClip then uses the border size from the *frame*, not the border style, to do the reduction.  And the frame has a 2px border width, because the circle around the button is drawn as a border (declared a bit above the code you quoted).  So SetupBackgroundClip cuts two pixels off each side of what was supposed to be a 4px content-box, leaving nothing.

I'm hesitant to change SetupBackgroundClip - there are other things besides the used border style that can change the used border of a frame, right?  Rather, I think what PaintRadioButtonFromStyle is doing represents an abuse of the internal API and I'd like to have it do its own drawing.  There's an XXXldb in the function saying "This should use FillEllipse, but that's not round enough" -- well, we could use RoundedRectangle, which is what PaintBackgroundWithSC is using anyway, so it should come out the same.  The disadvantage of this is, of course, that not everything one could specify in the ::-moz-radio rule would work, but I think we can live with that ...?
I think so.  Fundamentally, the ::-moz-radio thing seems like a cross between misplaced extensibility and a hack to leverage existing drawing code to me... I have no problem with nixing it.
Comment on attachment 359168 [details] [diff] [review]
Like so?

I'm now convinced Mats' patch is wrong -- will attach a test case that it breaks.
Attachment #359168 - Flags: superreview?(roc)
Attachment #359168 - Flags: review?(roc)
Attachment #359168 - Flags: review-
-moz-box-sizing shouldn't override -moz-background-clip, or things like this would break.
(In reply to comment #13)
> I think so.  Fundamentally, the ::-moz-radio thing seems like a cross between
> misplaced extensibility and a hack to leverage existing drawing code to me... I
> have no problem with nixing it.

Ok, I'll come up with a patch after lunch.  Do I understand correctly that you wouldn't mind me killing off ::-moz-radio altogether?
I wouldn't mind at all.  Want to kill ::-moz-checkbox too?  :)
For what it's worth, last time I tried using FillEllipse, there weren't any parameters I could give it that would cause it to fill pixels with the pattern:

  XX
 XXXX
 XXXX
  XX

which was what we used for radio buttons at the time.  I thought I'd filed a bug on that, but I can't find it.  I suspect cairo is capable of drawing circles better than X, though.

Getting rid of these anonymous boxes or pseudo-elements or whatever they are sounds good to me.
Here is a patch which makes nsGfxRadioControlFrame draw the 'checked' state with direct calls to gfx.  I wound up using FillEllipse after all -- it's much simpler than the hoops you have to jump through to use the Thebes RoundedRectangle method, and it seems to be drawing nice circles at all zoom levels.  At a few zoom levels the circle is slightly displaced from where it ought to be but I'm not sure it's possible to do any better.

nsGfxCheckboxControlFrame already had direct-drawing code, used unless there was a ::-moz-checkbox style, which there wasn't.  So I just ripped out the alternative code used for ::-moz-checkbox, which should have no visible effect.
Attachment #359168 - Attachment is obsolete: true
Attachment #370546 - Flags: review?(bzbarsky)
Attachment #370546 - Flags: superreview+
Attachment #370546 - Flags: review?(bzbarsky)
Attachment #370546 - Flags: review+
Comment on attachment 370546 [details] [diff] [review]
patch to draw checkboxes & radio buttons directly

Stealing review.

Can't we actually now construct reftests for non-native radio buttons and checkboxes, both checked and unchecked, where we compare the rendering to a reference SVG image that happens to use the same set of Thebes calls?
r=me too, for what it's worth.  I love the code removal.

It's weird that there's shifting by zoom level.  Does FillEllipse pixel-snap or something?
(In reply to comment #20)
> Can't we actually now construct reftests for non-native radio buttons and
> checkboxes, both checked and unchecked, where we compare the rendering to a
> reference SVG image that happens to use the same set of Thebes calls?

Probably, but my SVG-fu is inadequate.  ->clint?

(In reply to comment #21)
> It's weird that there's shifting by zoom level.  Does FillEllipse pixel-snap or
> something?

It looks like it doesn't.  I dunno.  Rounding errors?
Keywords: checkin-needed
Status: NEW → ASSIGNED
(In reply to comment #22)
> (In reply to comment #20)
> > Can't we actually now construct reftests for non-native radio buttons and
> > checkboxes, both checked and unchecked, where we compare the rendering to a
> > reference SVG image that happens to use the same set of Thebes calls?
> 
> Probably, but my SVG-fu is inadequate.  ->clint?

Eh, it's pretty easy. There are lots of examples to work from. See e.g. http://www.w3schools.com/svg/svg_ellipse.asp
Landed, and going to backout in a few minutes. You're failing the reftest for bug 373381.
Keywords: checkin-needed
<meta http-equiv="msthemecompatible" content="no"> disables the native theme on Mac but not on Linux (bug 486551).  I can reproduce the reftest failure if I change the test to use style="-moz-appearance:none".

Turns out the fix in bug 373381 works if and only if the declared border and padding in forms.css agree with the size reported by nsFormControlFrame::GetIntrinsic(Width|Height).  Revised patch shortly.  Should we maybe be setting !important on the width and height of radio buttons and checkboxes in forms.css?
Doing that would break websites... Why would we need to do that?
Revised patch, the meat of which is to set padding:0 on radio buttons (as we used to do) and hardwire a 2px gap between the button border and the dot for the checked state in nsGfxRadioControlFrame.  I also factored more identical declarations from the input[type="radio"] and input[type="checkbox"] blocks in forms.css down to the existing common-features block, ripped a bunch more stale code out of nsGfxCheckboxControlFrame, changed the tests for bug 373381 to use -moz-appearance:none rather than msthemecompatible, and added native and checkbox tests to that set.

Requesting re-review due to substantial changes beyond what was strictly necessary to fix the problem that caused the backout.
Attachment #370546 - Attachment is obsolete: true
Attachment #370711 - Flags: review?(roc)
(In reply to comment #26)
> [Setting !important on width and height of checkboxes and radio buttons]
> would break websites... Why would we need to do that?

Well, you can just as easily get the wacky rounded-rectangle radio with

<input type="radio" style="moz-appearance:none; width:17px">

or a non-square checkbox similarly, although I suppose we could just say "don't do that then".  Height=width!=13px does seem to work sanely.
Yeah, if people want to create non-round radios, that's fine.
Comment on attachment 370711 [details] [diff] [review]
rev 2: no padding in forms.css for radio buttons

Looks good.

I still think it's worth having SVG references for the non-native look, though.
Attachment #370711 - Flags: review?(roc) → review+
(In reply to comment #30)
> I still think it's worth having SVG references for the non-native look, though.

Ok, I tried, and I failed.  This new attachment *ought to* render the same as reftests/forms/checkbox-checked.html but does not, because the border colors you get when you specify "border: 2px inset ThreeDFace" in an XHTML document are the same as you get with that in a quirks mode HTML document but NOT the same as what you get in a standards mode HTML document.  I looked for, and did not find, code causing this divergence.  The top-left corner of the fake checkbox may also be slightly off from where it's supposed to be, I can't tell by eye in the presence of the above.  (It does not help that the margins actually applied to a real checkbox do not match the margins declared in forms.css, and Firebug doesn't work in trunk builds right now.)

You can get at the "ThreeDFace" color in pure SVG but you can't get at the color transformation that "border:inset" applies, nor can you style a <rect> to be outlined in the same way that any of the 3D-effect HTML border styles do.
Keywords: checkin-needed
Attached file reference
When I compare your testcase to this reference, it passes on Mac.
(In reply to comment #32)
> 
> When I compare your testcase to this reference, it passes on Mac.

Aha, but you're in quirks mode!  Stick <!doctype html> at the top of it and watch the border colors change.
We should probably figure out why that is, but I have no moral objection to having the reference be quirks-mode.
http://hg.mozilla.org/mozilla-central/rev/f3d729b22f94

setting in-testsuite? for the comprehensive SVG reftest
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing] [baking]
Target Milestone: --- → mozilla1.9.2a1
Zack, could you create a 1.9.1 branch patch? This patch needs heavy merging, e.g. with bug 396185, bug 473390 (part 5) and bug 123836.
Whiteboard: [needs 1.9.1 landing] [baking] → [needs branch patch] [needs 1.9.1 landing] [baking]
This patch should apply cleanly to the 1.9.1 branch.  There are internal interface cleanups that are possible afterward -- for instance, nsICheckboxControlFrame and nsIRadioControlFrame are now identical -- but they're all superseded by the queryframe refactor on trunk so I don't think we would gain anything from making that change on the branch.

Reftests on the branch look good.
Keywords: checkin-needed
Whiteboard: [needs branch patch] [needs 1.9.1 landing] [baking] → [needs 1.9.1 landing]
verified FIXED on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090618 Shiretoko/3.5pre ID:20090618042848

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090610 Shiretoko/3.5pre ID:20090610031356

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre ID:20090618031143
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.