Specified width and height on <input type=checkbox/radio> is ignored

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({testcase})

unspecified
mozilla28
All
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Follow-up from bug 932506.

We should honor the specified width and height, and center the checkbox/radio
drawing inside it, like we do on OSX.
Posted file Testcase #1
I noticed that on OSX we scale the drawing to fill the smaller dimension.
We don't do that on Linux and Windows.  This inconsistency seems like a bug.
Other UAs don't scale like that, and transform:scale() could be used to get
a similar effect, so I think we should remove the scaling we do in the widget
layer on OSX.  What do you think?  (see Testcase #1 on OSX)
Flags: needinfo?(bzbarsky)
Actually, I found one other UA that scales like that: IE10 on Windows 8.
I'm a bit ambivalent, maybe it's a feature...
Native theme stuff is black voodoo.  I have no particular opinion on it.  :(
Flags: needinfo?(bzbarsky)
We do scale on all platforms if -moz-appearance:none; is specified, so yeah I think
we should try to fix that for a themed checkbox/radio on Linux/windows too.

Searching a bit I found bug 446646 (filed by you :-p), although that's more about
the size which I'm fixing in this bug.  Vlad's patch in that bug, and some comments,
seems to support the idea that we should scale.
Posted patch fix+tests (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=4c46bf4e1d53
Attachment #825580 - Flags: review?(tnikkel)
(I filed bug 933503 (GTK) and bug 933507 (win32) to implement scaling for themed checkbox/radio.)
Comment on attachment 825580 [details] [diff] [review]
fix+tests

Hmm, maybe Karl would be able to review this quicker than me?
Comment on attachment 825580 [details] [diff] [review]
fix+tests

The code changes look good, but I don't understand the reftests.
Can you explain what is happening with the absolute positioning, please?

>+<div style="position:absolute;">
>+<div style="position:absolute; top:1px; left:3px; bottom:2px; width:300px; background:white; z-index:1; "></div>

I don't know what it means to specify bottom as well as top.
I thought bottom would only affect position, not size, and the containing block is not sized.

Where these top and left and bottom offsets, and the width come from?
I don't see them specified in gtk-theme-width-height.html

>+<input type="radio" checked style="width:160px; visibility:hidden"><br>

Why is the width important on this input?
The outermost abs.pos. has shrink-wrap size, so it'll get the size of the
checkbox child.  The inner abs.pos. masks the checkbox where we expect
it to be.  The top/bottom is to make the inner slightly smaller in height than
the outer, so that the outline becomes visible.  The test will pass if the
checkbox is centered and will fail if the checkbox is either left-aligned or
is centered but not respecting the specified width, because in the test file
it will then be rendered in the 160px space inside the overflow:hidden.

I would expect top/left/bottom to be 1px to reveal an 1px outline but
apparently the themeing adds some extra space around the outline.
I don't see a way to avoid this dependency.

The visibility:hidden <input> is just to take up some space in normal
flow so that the boxes below are placed the same as in the test file.
The specified width there isn't important, I'll remove it.
BTW, do you know how to draw a checkbox larger than default in GTK?
The "indicator-size" style property is Read/Write but I don't see
a way to set it for a widget.
http://www.gtk.org/api/2.6/gtk/GtkCheckButton.html#GtkCheckButton--indicator-size
Comment on attachment 825580 [details] [diff] [review]
fix+tests

Thanks for the explanation.

(In reply to Mats Palmgren (:mats) from comment #12)
> I would expect top/left/bottom to be 1px to reveal an 1px outline but
> apparently the themeing adds some extra space around the outline.
> I don't see a way to avoid this dependency.

Would explicitly setting the margin of the input remove theming effects here?
If not can you make the top/left/bottom significantly larger please to make
this less susceptable to theme differences?

Can you also explicity set the background of the input to white please to
match the inner abs.pos.?
Attachment #825580 - Flags: review?(tnikkel) → review+
(In reply to Mats Palmgren (:mats) from comment #13)
> BTW, do you know how to draw a checkbox larger than default in GTK?
> The "indicator-size" style property is Read/Write but I don't see
> a way to set it for a widget.
> http://www.gtk.org/api/2.6/gtk/GtkCheckButton.html#GtkCheckButton--indicator-
> size

AIUI style properties come from the theme, and so are not meant to be set by the application.  See
https://developer.gnome.org/gtk2/2.24/GtkWidget.html#GtkWidget.description

gtk_paint_check and gtk_paint_option do accept width and height parameters, so if there is a way to draw a checkbox larger then that would be it.
gtk_cell_renderer_toggle_render (in GTK) passes values for these parameters that are larger than indicator-size, so it should do something reasonable.  Depending on the theme engine, it may just center, but I expect it is OK to let the theme engine choose what to do here.
Posted patch fix+tests (obsolete) — Splinter Review
I realized I could simplify the tests that were using abs.pos.
wrappers, so I did.

https://hg.mozilla.org/integration/mozilla-inbound/rev/03b936cdbf73
Attachment #825580 - Attachment is obsolete: true
Attachment #826091 - Flags: review+
Flags: in-testsuite+
Posted patch fix+testsSplinter Review
Ah, right, I forgot that I needed to run the full set of tests because
of the modified assertion, sorry about that.

The problem is that layout/generic/crashtests/769303-1.html
triggers bug 369581 and that fails the assertion condition
I added.  I removed it and added a comment about it instead
so we can add it later.  The intra-diff is:

< +    NS_ASSERTION(rect->width >= indicator_size &&
< +                 rect->height >= indicator_size,
---
> +    // XXX we should assert rect->height >= indicator_size too
> +    // after bug 369581 is fixed.
> +    NS_ASSERTION(rect->width >= indicator_size,

(for both widget/gtk/gtk2drawing.c and gtk3drawing.c)

No other changes from what was landed.

Full suite of tests on Try this time:
https://tbpl.mozilla.org/?tree=Try&rev=4fb7e831ee7e
Attachment #826091 - Attachment is obsolete: true
Attachment #826220 - Flags: review?(karlt)
Attachment #826220 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/08ad8ae130cb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.