User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060702 Firefox/3.0a1 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060702 Firefox/3.0a1 The checkmark of a checkbox input element is never visible if the checkbox has height and width styles either through css or the style attribute. For example: <input style="height: 15px; width: 15px;" type="checkbox"> I see this with a self-built non-cairo gtk2 trunk build on Linux: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060702 Firefox/3.0a1 FF 1.5.0.4 on Linux does not exhibit this problem as the checkmark is visible, so this would seem to be a regression. Reproducible: Always

Also happens with nightly Minefield build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060703 Minefield/3.0a1

The mark appears if you resize the window slightly... (in the top left corner, outside its borders) Regression range: 2006-01-25-05 -- 2006-01-26-04 http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-01-25+03%3A00&maxdate=2006-01-26+06%3A00&cvsroot=%2Fcvsroot In that range bug 317375 looks the most likely to have caused it.

This fixes it. This absurd "twips == 165" check is triggering the PaintFixedCheckMark path, which happens to ignore aRect's top-left. The fix is easy. We can get rid of Push/PopState because we're not changing anything in the rendering context other than the current color, which is allowed.

From left to right: Firefox 2006-01-25-05 (before the regression) Trunk with suggested fix IE6/XP2 Firefox 1.5.0.4/XP Maybe we want to adjust the 15x15 mark to be 1px left and up? The patch looks good otherwise, maybe we should even remove nsFormControlHelper::PaintFixedSizeCheckMarkBorder - it's dead code AFAICT.

Yes that is dead code. I'll bury it.

Why is the 15x15 checkmark to the right of the 16x16 checkmark? Are we taking the 165-twips codepath for that case, on your system? How about we kill the 165-twips codepath? Even if it's doing something useful for someone, it's a horrible hack.

(In reply to comment #8) > Are we taking > the 165-twips codepath for that case, on your system? Yes, and this is why it is further right than the 16x16 mark. Regarding you comment on the 165-twips codepath - wouldn't we take this path also on systems with twips-per-pixel != 15 for other checkbox sizes? > How about we kill the 165-twips codepath? A matter of taste which you prefer. I fixed the position and took a screenshot of both paths. From left to right: 165-twips path generic path 165-twips path zoomed to 400% generic path zoomed to 400% Either is fine with me. The fix I did for the position: - const PRUint32 ox = 3; - const PRUint32 oy = 3; + const PRUint32 ox = 2; + const PRUint32 oy = 2;

> > Are we taking > > the 165-twips codepath for that case, on your system? > > Yes, and this is why it is further right than the 16x16 mark. > > Regarding you comment on the 165-twips codepath - wouldn't we take this > path also on systems with twips-per-pixel != 15 for other checkbox sizes? Sure, but the factors of 165 are only 1, 3, 5, 11, 15, 33, 55 and 165. Of those, only 11 and 15 are realistic t2p values, so the path is only taken for 15px checkboxes at t2p==11, and 11px checkboxes at t2p==15. Why they deserve special treatment is entirely inexplicable. > > How about we kill the 165-twips codepath? > > A matter of taste which you prefer. I fixed the position and took > a screenshot of both paths. From left to right: Let's kill it. Can you make the patch?

(In reply to comment #10) > Let's kill it. Can you make the patch? > Ok, but I looked a bit closer on nsFormControlHelper and found that several other methods are dead code as well... the only ones that actually does anything useful are: PaintCheckMark() used by GfxCheckboxControl GetFrameFontFM() used by TextControl, Combobox, ListControl PlatformToDOMLineBreaks() and GetWrapPropertyEnum() used by TextControl The other are one-liners that can be inlined where used. (GetFrameFontFM() is only used once from each file so we could possibly inline that as well - it's a two-liner) So, why not move GetFrameFontFM() to LayoutUtils and the other two to where they are used and then remove nsFormControlHelper altogether (cvs rm)? Something like this...

Comment on attachment 228131 [details] [diff] [review] Patch rev. 2 that makes my day!

Thanks! Checked in to trunk at 2006-07-06 03:43 PDT -> FIXED