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

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=ee535e75b523

Comment 2

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=4c46bf4e1d53

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 4

[Mats Palmgren (inactive)]

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)

Comment 5

[Mats Palmgren (inactive)]

Actually, I found one other UA that scales like that: IE10 on Windows 8.
I'm a bit ambivalent, maybe it's a feature...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Native theme stuff is black voodoo.  I have no particular opinion on it.  :(

Comment 7

[Mats Palmgren (inactive)]

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.

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix+tests

https://tbpl.mozilla.org/?tree=Try&rev=4c46bf4e1d53

Comment 9

[Mats Palmgren (inactive)]

(I filed bug 933503 (GTK) and bug 933507 (win32) to implement scaling for themed checkbox/radio.)

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 825580 [details] [diff] [review]
fix+tests

Hmm, maybe Karl would be able to review this quicker than me?

Comment 11

[Karl Tomlinson (:karlt)]

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?

Comment 12

[Mats Palmgren (inactive)]

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.

Comment 13

[Mats Palmgren (inactive)]

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 14

[Karl Tomlinson (:karlt)]

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

Comment 15

[Karl Tomlinson (:karlt)]

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

Comment 16

[Mats Palmgren (inactive)]

Attachment: fix+tests

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/cb757cda1837 for these crashtest assertions: https://tbpl.mozilla.org/php/getParsedLog.php?id=29994876&tree=Mozilla-Inbound

Comment 18

[Mats Palmgren (inactive)]

Attachment: fix+tests

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

Comment 19

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08ad8ae130cb

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08ad8ae130cb