Closed Bug 343495 Opened 14 years ago Closed 14 years ago

Checkmark is not visible on checkbox with height and width styles

Categories

(Core :: Layout: Form Controls, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: stephen.moehle, Assigned: roc)

References

Details

(Keywords: regression, testcase)

Attachments

(5 files, 1 obsolete file)

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
Keywords: regression
Version: unspecified → Trunk
Product: Firefox → Core
QA Contact: general → general
Component: General → Layout: Form Controls
QA Contact: general → layout.form-controls
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Attached patch fix (obsolete) — Splinter Review
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #228001 - Flags: superreview?(bryner)
Attachment #228001 - Flags: review?(mats.palmgren)
Attached file Testcase #2
Attached image Screenshot
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.
Attached image Screenshot #2
(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?
Attached patch Patch rev. 2Splinter Review
(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!
Attachment #228131 - Flags: superreview+
Attachment #228131 - Flags: review+
Attachment #228001 - Attachment is obsolete: true
Attachment #228001 - Flags: superreview?(bryner)
Attachment #228001 - Flags: review?(mats.palmgren)
Thanks!

Checked in to trunk at 2006-07-06 03:43 PDT

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.