Closed Bug 1404770 Opened 7 years ago Closed 7 years ago

Minimum widget size of checkbox/radio on Android is wrong

Categories

(Core Graveyard :: Widget: Android, defect, P2)

Product:
Core Graveyard
Component:
Widget: Android
Version:
57 Branch
Type:
defect

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: lochang, Assigned: lochang)

References

(Blocks 3 open bugs)

Details

Attachments

(4 files)

Bug 1404770 Part 1 - Return device pixels instead of css pixels in GetMinimumWidgetSize.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1404770 Part 2 - Add test case for testing minimum size of checkbox/radio.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Testcase
385 bytes, text/html
Details
Bug 1404770 Part 3 - Remove test case checkbox-radio-auto-sized-ref.html.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
Based on discussion on bug 1400050 comment 7, open the bug to fix the minimum widget size issue.
Hi mats,

Could you review the patches? Thanks!

It seems reasonable to return device pixel in GetMinimumWidgetSize as you said since it expects getting device pixel here http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#5277-5301 and the minimum widget size is correct now.

But there is a problem when I try to wrap a checked and unchecked <input> into a <div> on Android. If given a checkbox with height smaller or equal to 13px, there is about 1px offset of y position (see example https://codepen.io/ghoo1125/pen/PJJOVw). Not sure if it is a fuzzy case due to rounding issue. However, I leave the testcase checkbox/radio-clamp.html as it was (only unchecked). And add another test case here for testing the minimum widget size.
Attached file Testcase — 
When using an even size 120px, then if we use top:60px on the abs-pos
checkboxes they should align vertically at the center if they were
zero height, but on Linux the minimum size is 18px, so to get 9px
of that above the center line I use top:51px to make these checkboxes
align vertically for me (I added a little margin-left to make it
easier to see that they are vertically aligned).

Can you try this test with height:113px and top:50px?

I guess on Android with 113px / 2 (56.5) - 13px / 2 (6.5) => 50px
should make them align vertically visually.  If they are not, then
we should probably figure out why.  What is the computed size that
devtools shows?  What's the aRect.y/height you get in the painting
functions (after converting it w. AppUnitsToFloatCSSPixels)?
Flags: needinfo?(lochang)
Comment on attachment 8914667 [details]
Bug 1404770 Part 1 - Return device pixels instead of css pixels in GetMinimumWidgetSize.

https://reviewboard.mozilla.org/r/185988/#review191280
Attachment #8914667 - Flags: review?(mats) → review+
Comment on attachment 8914668 [details]
Bug 1404770 Part 2 - Add test case for testing minimum size of checkbox/radio.

https://reviewboard.mozilla.org/r/185990/#review191282
Attachment #8914668 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #5) 
> Can you try this test with height:113px and top:50px?

Sure. So the result is the same with my test case. There are still some pixels offset of y. I will open another bug to trace this issue and update the detailed information I found in the bug. However, with your test case, the offset is bigger due to the rule: * { margin:0; }. Maybe we can start from here.

Also, do you think the minimum widget size test case (attachment 8914668 [details]) is duplicate to the auto sized test case? http://searchfox.org/mozilla-central/source/layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized.html
Flags: needinfo?(lochang)
Flags: needinfo?(mats)
Rebase patches also annotate the test case as fails-if(OSX) since we do not handle minimum checkbox/radio size correctly on OSX.
(https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ef019ce00228aadae0e0e057f09c08a6303995&selectedJob=134596916)
(In reply to Mats Palmgren (:mats) from comment #5)
> but on Linux the minimum size is 18px, so to get 9px

Speaking about this, what should we do for the test case http://searchfox.org/mozilla-central/source/layout/reftests/forms/input/checkbox/reftest.list#18 on Linux? Since on Linux, the minimum size of checkbox/radio is 18px, but in ref test case we calculate the abs-pos by supposing the minimum widget size 13px. (Although we do not handle clamping correctly on Linux now. e.g. given a checkbox with height:100px and width:30px, it will not expand the checkbox to 30px*30px but remain 18px*18px)
(In reply to Louis Chang [:louis] from comment #9)
> the detailed information I found in the bug. However, with your test case,
> the offset is bigger due to the rule: * { margin:0; }. Maybe we can start
> from here.

Sorry... just ignore this. It is due to the default margin-top:3px of the checkbox is set to 0...
(In reply to Louis Chang [:louis] from comment #9)
> Also, do you think the minimum widget size test case (attachment 8914668 [details]
> [details]) is duplicate to the auto sized test case?
> http://searchfox.org/mozilla-central/source/layout/reftests/forms/input/
> checkbox/checkbox-radio-auto-sized.html

Oh, right, I guess the 11px in that test is probably trumped by
the minimum widget size, so it doesn't actually test what was intended.
Perhaps we should just remove that test for now?

(In reply to Louis Chang [:louis] from comment #13)
> (Although we do not handle clamping
> correctly on Linux now. e.g. given a checkbox with height:100px and
> width:30px, it will not expand the checkbox to 30px*30px but remain
> 18px*18px)

Yeah, please file separate bugs for OSX and Linux to investigate
implementing the same clamping behavior as we did for Android in
bug 1400050.
Flags: needinfo?(mats)
... and Windows too I guess :-)  (i.e. one bug for each widget
backend that doesn't already have that clamping behavior)
Comment on attachment 8915808 [details]
Bug 1404770 Part 3 - Remove test case checkbox-radio-auto-sized-ref.html.

https://reviewboard.mozilla.org/r/187022/#review192082
Attachment #8915808 - Flags: review?(mats) → review+
Try looks good (comment 8).
Keywords: checkin-needed
(In reply to Mats Palmgren (:mats) from comment #16)
> ... and Windows too I guess :-)  (i.e. one bug for each widget
> backend that doesn't already have that clamping behavior)

So I open bug 1406267, bug 1406268 for handling clamping issue on Windows and Linux respectively.
Also open bug 1406273 to handle minimum checkbox/radio size on OSX.
Blocks: 1406334
(In reply to Louis Chang [:louis] from comment #22)
> So I open bug 1406267, bug 1406268 for handling clamping issue on Windows
> and Linux respectively.

And bug 1406334 for OSX.
No longer blocks: 1406334
Blocks: 1406267, 1406268, 1406334
Blocks: 1406273
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8d9aa5783cf4
Part 1 - Return device pixels instead of css pixels in GetMinimumWidgetSize. r=mats
https://hg.mozilla.org/integration/autoland/rev/75069e77df54
Part 2 - Add test case for testing minimum size of checkbox/radio. r=mats
https://hg.mozilla.org/integration/autoland/rev/7ec3c5a446b2
Part 3 - Remove test case checkbox-radio-auto-sized-ref.html. r=mats
Keywords: checkin-needed
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: