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.
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)
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. :(
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.
Attachment #825580 - Flags: review?(tnikkel)
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.
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
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
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
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.