Closed Bug 1406268 Opened 2 years ago Closed 2 years ago

Handle clamping width/height of checkbox/radio on Linux

Categories

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

57 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: lochang, Assigned: lochang)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files)

Attached file 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.
(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/
Depends on: 1404770
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...
Assignee: nobody → lochang
Status: NEW → ASSIGNED
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.
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.
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 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
Attachment #8926781 - Flags: review?(mats) → review+
(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?
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
(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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/37707fcd8a55
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1453886
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
Depends on: 1448682
You need to log in before you can comment on or make changes to this bug.