1406268 - Handle clamping width/height of checkbox/radio on Linux

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement, P3)

Description

[Louis Chang [:louis]]

Attachment: testcase.html

Currently, we do not handle clamping width/height of checkbox/radio on Windows. We expect a 30px*30px checkbox in the test case. But it is a 18px*18px (minimum size) checkbox.

Comment 1

[Louis Chang [:louis]]

(In reply to Louis Chang [:louis] from comment #0)
> Currently, we do not handle clamping width/height of checkbox/radio on
> Windows. We expect a 30px*30px checkbox in the test case. But it is a
> 18px*18px (minimum size) checkbox.

s/Windows/Linux/

Comment 2

[Mats Palmgren (inactive)]

It would be good to fix this before bug 1368555, so that when we start to
recognize -webkit-appearance:checkbox/radio it actually renders the same
as in Chrome.  It's probably not a blocking issue though...

Comment 3

[Louis Chang [:louis]]

Attachment: Bug 1406268 - Use rect width/height instead of minimum width/height and clamp the rect on Linux.

Review commit: https://reviewboard.mozilla.org/r/198034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198034/

Comment 4

[Louis Chang [:louis]]

Hi mats,

Could you review the patch for me? Thanks.

I change to use rect->widht/height instead of minimum width/height when painting checkbox/radio.

Comment 5

[Louis Chang [:louis]]

Test result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7ab8a028f4cf02e55aba80505605387f5cb19a1&selectedJob=143270972

The patch breaks test gtk-theme-width-height.html. I will look into it.

Comment 6

[Louis Chang [:louis]]

Since now we accept the specified size instead of always using minimum size on Linux. It seems reasonable that the left part of checkbox (<input checked="" style="width:400px; height:100px; outline:none;" type="checkbox">) will show up in the testcase gtk-theme-width-height.html. Maybe we should somehow fix the testcase?

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8926781 [details]
Bug 1406268 - Use rect width/height instead of minimum width/height and clamp the rect on Linux.

https://reviewboard.mozilla.org/r/198034/#review205642

Looks great, r=mats

Could also make the same change to widget/gtk/gtk2drawing.c please?
https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/widget/gtk/gtk2drawing.c#1047-1056

Comment 8

[Mats Palmgren (inactive)]

(In reply to Louis Chang [:louis] from comment #6)
> Since now we accept the specified size instead of always using minimum size
> on Linux. It seems reasonable that the left part of checkbox (<input
> checked="" style="width:400px; height:100px; outline:none;"
> type="checkbox">) will show up in the testcase gtk-theme-width-height.html.
> Maybe we should somehow fix the testcase?

Yeah, the checkbox rendering bleeds in a little on the right side
in the box at the top.  I think we can just clip it a bit narrower
to avoid that.  s/160px/120px/ or something?

Comment 9

[Mats Palmgren (inactive)]

BTW, can you add this to your .hgrc file unless you have it please?
(to make it a bit easier to see which function is being changed):

[diff]
showfunc = true

Comment 10

[Louis Chang [:louis]]

Comment on attachment 8926781 [details]
Bug 1406268 - Use rect width/height instead of minimum width/height and clamp the rect on Linux.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198034/diff/1-2/

Comment 11

[Louis Chang [:louis]]

(In reply to Mats Palmgren (:mats) from comment #9)
> BTW, can you add this to your .hgrc file unless you have it please?
> (to make it a bit easier to see which function is being changed):
> 
> [diff]
> showfunc = true

I already set showfunc = 1 in my .hgrc file, not sure why you could not see the changed function name. Anyway I have changed it to showfunc = true.

Comment 12

[Louis Chang [:louis]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=650a8e02ca8174a859eb56d9be3384cfa47342d1

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37707fcd8a55
Use rect width/height instead of minimum width/height and clamp the rect on Linux. r=mats

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/37707fcd8a55

Comment 15

[Jan Horak [:jhorak]]

This depends on the theme which is actually used. I don't see the checkbox 30px*30px with the adwaita. This should be done by fixing the metrics (if there is a mistake in the computing), see how it is computed and why [1]. What theme is triggering this problem?

[1] https://searchfox.org/mozilla-central/rev/08df4e6e11284186d477d7e5b0ae48483ecc979c/widget/gtk/gtk3drawing.cpp#2862