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

Status: RESOLVED FIXED

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

Description

[Louis Chang [:louis]]

Attachment: testcase

Based on discussion in Bug 1404770, open the bug to trace the issue.

Comment 1

[Louis Chang [:louis]]

Attachment: checkbox.png

screenshot of the test case on Android.

Comment 2

[Louis Chang [:louis]]

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.

Comment 3

[Louis Chang [:louis]]

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.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Hmm, maybe the whitespace between the <input>s affects the line-height.
Does this test work better?

Comment 5

[Louis Chang [:louis]]

(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?

Comment 6

[Louis Chang [:louis]]

Attachment: Bug 1405986 - Add test cases for clamping checkbox/radio.

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

Comment 7

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 8

[Louis Chang [:louis]]

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.

Comment 9

[Louis Chang [:louis]]

Try result fo test case 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d83833207f5d4be5fbfa76a0b5d1b6989bfad29

Comment 10

[Louis Chang [:louis]]

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)?

Comment 11

[Mats Palmgren (inactive)]

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.

Comment 12

[Louis Chang [:louis]]

(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...

Comment 13

[Mats Palmgren (inactive)]

> 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.

Comment 14

[Louis Chang [:louis]]

Comment on attachment 8916281 [details]
Bug 1405986 - Add test cases for clamping checkbox/radio.

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

Comment 15

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=795b5a338974d1f3f8c7eee19fe03ec31f201f85

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8916281 [details]
Bug 1405986 - Add test cases for clamping checkbox/radio.

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

Thanks, r=mats

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ebd11851adf
Add test cases for clamping checkbox/radio. r=mats

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2ebd11851adf