Closed Bug 1405986 Opened 2 years ago Closed 2 years ago

The checkbox/radio is not aligned vertically after clamping on Android

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: lochang, Assigned: lochang)

References

Details

Attachments

(5 files)

Attached file testcase
Based on discussion in Bug 1404770, open the bug to trace the issue.
Attachment #8915510 - Attachment mime type: text/plain → text/html
Summary: The checkbox/radio is not align vertically after clamping on Android → The checkbox/radio is not aligned vertically after clamping on Android
Attached image checkbox.png
screenshot of the test case on Android.
Based on comment 5 in bug 1404770:

The computed size of checkbox is 13.0333px * 113px for test and 13.0333px * 13.0333px for ref on dev tools.
And aRect.y for test is 49.98 and 54.25 for ref. Both aRect.height are 13.0333 the same as computed size.
Flags: needinfo?(mats)
I also found that the offset only occur in the abs-pos div element. Furthermore, If I remove one of the <input> from abs-pos div, they are aligned.
Attached file Testcase #2
Hmm, maybe the whitespace between the <input>s affects the line-height.
Does this test work better?
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #4)
> Created attachment 8915654 [details]
> Testcase #2
> 
> Hmm, maybe the whitespace between the <input>s affects the line-height.
> Does this test work better?

Yes, It's aligned! So i guess it's not a bug just a problem due to line-height.
I will just modify the test case here?
Also, maybe wrote a specific test case for Linux since it's minimum widget size is 18px other than 13px on other platforms?
Flags: needinfo?(mats)
Attached file Testcase #3
Actually, I wonder if we could use something like this instead.
The first grid has checkboxes that center themselves in a 100px height.
As a reference, the second grid has intrinsically sized checkboxes that
are vertically aligned in a 100px row.  These two grids should look
the same.

This should work on all platforms regardless of what the default
checkbox size happens to be.
Flags: needinfo?(mats)
Comment on attachment 8916281 [details]
Bug 1405986 - Add test cases for clamping checkbox/radio.

Ok, I will modify my test case and ask for review again.
Attachment #8916281 - Flags: review?(mats)
The test case fails on Windows and OSX as shown in try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15bedc16c012e3c2d5234474f258939f2943a87e&selectedJob=135691161

I haven't look into it yet. However, there are some offset on both OSX and Windows but it seems nothing to do with line-height. Maybe just annotate the test case as fails-if(OSX||WinWidget)?
Flags: needinfo?(mats)
Hmm, some pixel rounding issue?  Anyway, perhaps we could land your original
test with skip-if(gtkWidget) and the latter test with skip-if(OSX||WinWidget)?
Then we should be covered on all platforms.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #11)
> Hmm, some pixel rounding issue?  Anyway, perhaps we could land your original
> test with skip-if(gtkWidget) and the latter test with
> skip-if(OSX||WinWidget)?
> Then we should be covered on all platforms.

Sure.
However, I am wondering should we also write a clamping test case to test those platforms (Windows, Linux) that always use the minimum widget size? IIRC, given a checkbox with height:100px and width:30px for example, we will paint a checkbox with 13px*13px on Windows and 18px*18px on Linux at center. But we expect a checkbox with 30px*30px at center?
Even though I do not have any idea yet...
Flags: needinfo?(mats)
> But we expect a checkbox with 30px*30px at center?

Yes, but we can add reftests when we fix those bugs?
Not much point in testing for the wrong behavior before then IMO.
Flags: needinfo?(mats)
Comment on attachment 8916281 [details]
Bug 1405986 - Add test cases for clamping checkbox/radio.

https://reviewboard.mozilla.org/r/187496/#review193206

Thanks, r=mats
Attachment #8916281 - Flags: review?(mats) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ebd11851adf
Add test cases for clamping checkbox/radio. r=mats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2ebd11851adf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.