475535 - non-native radio buttons aren't drawn checked.

Status: VERIFIED FIXED

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

Description

[Sylvain Pasche]

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

Comment 1

[Mats Palmgren (inactive)]

Attachment: Like so?

Don't deflate the border for -moz-box-sizing:border-box.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So regression from bug 456219?

Comment 3

[Mats Palmgren (inactive)]

Yes, reversing the "rev 11" patch in that bug (with some tweaks to make it
compile) fixes the testcase.

Comment 4

[Zack Weinberg (:zwol)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The default is content-box.

Comment 7

[Boris Zbarsky [:bzbarsky]]

This is a regression from a 1.9.1 blocker, so needs to block 1.9.1.

Comment 8

[Zack Weinberg (:zwol)]

Comment on attachment 359168 [details] [diff] [review]
Like so?

I'm pretty sure this is the correct fix.  robert: review?

Comment 9

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

I don't understand why this patch fixes the bug. Can someone explain what the bug is and why this fixes it?

Comment 10

[Zack Weinberg (:zwol)]

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.

Comment 11

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

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.

Comment 12

[Zack Weinberg (:zwol)]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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 14

[Zack Weinberg (:zwol)]

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.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: test case broken by attachment 359168

-moz-box-sizing shouldn't override -moz-background-clip, or things like this would break.

Comment 16

[Zack Weinberg (:zwol)]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

I wouldn't mind at all.  Want to kill ::-moz-checkbox too?  :)

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 19

[Zack Weinberg (:zwol)]

Attachment: patch to draw checkboxes & radio buttons directly

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.

Comment 20

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

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?

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[Zack Weinberg (:zwol)]

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

Comment 23

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

(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

Comment 24

[Dão Gottwald [:dao]]

Landed, and going to backout in a few minutes. You're failing the reftest for bug 373381.

Comment 25

[Zack Weinberg (:zwol)]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

Doing that would break websites... Why would we need to do that?

Comment 27

[Zack Weinberg (:zwol)]

Attachment: rev 2: no padding in forms.css for radio buttons

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.

Comment 28

[Zack Weinberg (:zwol)]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

Yeah, if people want to create non-round radios, that's fine.

Comment 30

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

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.

Comment 31

[Zack Weinberg (:zwol)]

Attachment: non-working attempt at HTML+SVG reference for checked checkbox

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

Comment 32

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

Attachment: reference

When I compare your testcase to this reference, it passes on Mac.

Comment 33

[Zack Weinberg (:zwol)]

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

Comment 34

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

We should probably figure out why that is, but I have no moral objection to having the reference be quirks-mode.

Comment 35

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f3d729b22f94

setting in-testsuite? for the comprehensive SVG reftest

Comment 36

[Markus Stange [:mstange]]

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.

Comment 37

[Zack Weinberg (:zwol)]

Attachment: patch for 1.9.1 branch

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.

Comment 38

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/806ce0e44b14

Comment 39

[Aakash Desai [:aakashd]]

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