1400050 - Distorted checkbox + radio when descendant of display:flex wrapper and associated label line-wraps

Status: RESOLVED FIXED

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

Description

[Mike Taylor [:miketaylr]]

Originally reported @ https://github.com/webcompat/web-bugs/issues/9736.

When a checkbox or radio input is a child of a display:flex container, and its associated label is super long and wraps, the widget is stretched vertically.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: screenshot

Comment 2

[Louis Chang [:louis]]

I will investigate the bug. Leave the ni to remind myself.

Comment 3

[Mats Palmgren (inactive)]

This is an interesting case, since I think Android is closer to what
we should do also on other platforms.  Compare the following test
in Chrome and FF on Linux:
data:text/html,<input type=checkbox style="width:100px; height:50px; outline:1px solid red">

The box size in both UAs is the specified size, but only Chrome
uses the specified size for the control, FF makes it 15x15.

I think we should do what Chrome Linux does on all platforms.
That is, clamp the width/height to the smaller of the two and
then center it (in the larger dimension).

Comment 4

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=852c1dbb79437566c58a0c462cd997cb43684043

Comment 5

[Louis Chang [:louis]]

Attachment: Bug 1400050 Part 1 - Clamp the width/height of checkbox/radio to the smaller one and then center it on Android.

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

Comment 6

[Louis Chang [:louis]]

Attachment: Bug 1400050 Part 2 - Add test case for testing clamping issue of checkbox/radio.

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

Comment 7

[Louis Chang [:louis]]

Hi mats, could you review the patches, thanks.

In part 1 patch, in order to reuse the clampped and deflated rect, I move out the deflate part from every painting functions and call it in DrawWidgetBackground also clamp the rect here. Then pass the calculated rect to each painting functions.

In part 2 patch, I try to add test cases for clamping width/height of checkbox/radio. Not sure if it is reasonable?

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8910868 [details]
Bug 1400050 Part 1 - Clamp the width/height of checkbox/radio to the smaller one and then center it on Android.

https://reviewboard.mozilla.org/r/182350/#review187652

r=mats if you agree with the "/ 2" issue.

::: widget/android/nsNativeThemeAndroid.cpp:19
(Diff revision 1)
>  NS_IMPL_ISUPPORTS_INHERITED(nsNativeThemeAndroid, nsNativeTheme, nsITheme)
>  
>  using namespace mozilla::gfx;
>  
>  static void
> +ClampRectAndMoveToCenter(nsRect& aRect) {

style nit: the opening brace should be on a line by itself

::: widget/android/nsNativeThemeAndroid.cpp:21
(Diff revision 1)
> +    float offset = (aRect.height - aRect.width) / 2.0;
> +    aRect.y += offset;

FYI, 2.0 is of type double, so I think you meant 2.0f here.

But, I don't see why we need to first do floating point division only to truncate the result when assigning it to aRect.y.  I guess we could round the result, but that doesn't seem necessary here (1 nscoord is typically 1/60px).

So, can we just do this instead?:
aRect.y += (aRect.height - aRect.width) / 2;

::: widget/android/nsNativeThemeAndroid.cpp:23
(Diff revision 1)
> +    aRect.height = aRect.width;
> +  }
> +
> +  if (aRect.height < aRect.width) {

nit: I think we should add a return at the end of the first if-statement.  While that last assignment always make the condition that follows false, a return makes the code simpler since it removes that dependency.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8910869 [details]
Bug 1400050 Part 2 - Add test case for testing clamping issue of checkbox/radio.

https://reviewboard.mozilla.org/r/182352/#review187658

This looks good, but could you also add <input checked type=...> to the tests you have?
Also, it would be good to test <input style="height:0" type=...> to check that our minimum widget size is working properly.
(i.e. test all 4 combinations of 0px/113px and checked/unchecked)

Comment 10

[Louis Chang [:louis]]

Comment on attachment 8910868 [details]
Bug 1400050 Part 1 - Clamp the width/height of checkbox/radio to the smaller one and then center it on Android.

https://reviewboard.mozilla.org/r/182350/#review187652

> FYI, 2.0 is of type double, so I think you meant 2.0f here.
> 
> But, I don't see why we need to first do floating point division only to truncate the result when assigning it to aRect.y.  I guess we could round the result, but that doesn't seem necessary here (1 nscoord is typically 1/60px).
> 
> So, can we just do this instead?:
> aRect.y += (aRect.height - aRect.width) / 2;

Ok, that make sense.

Comment 11

[Louis Chang [:louis]]

(In reply to Mats Palmgren (:mats) from comment #9)
> This looks good, but could you also add <input checked type=...> to the
> tests you have?
> Also, it would be good to test <input style="height:0" type=...> to check
> that our minimum widget size is working properly.
> (i.e. test all 4 combinations of 0px/113px and checked/unchecked)

The checkbox with 0px height is distorted (size is very small). It seems GetMinimumWidgetSize only work for non-sized checkbox/radio. Still looking the for root cause.

Comment 12

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=870a81f1ca9497445d9a4788ba18978532b875e8

Comment 13

[Louis Chang [:louis]]

Attachment: android_checkbox.png

Comment 14

[Louis Chang [:louis]]

Hi Mats,

I am a little bit confused about what the default size of checkbox now... could you help me out? Thanks.

1) On physical machine it seems the default checkbox size is 9px * 9px (see attachment 8912084 [details]). In the test case, the y position of checkbox is (113 - 9) / 2 + 8 = 60, where 8 is the default margin of body. But in try test harness, it seems the default size is 13px * 13px, the y position of checkbox is (113 - 13) / 2 + 8 = 58. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a632ab0785dcfd9ff272f4c5ff4b9711048a4e5a&selectedJob=133136057
2) Also, there are two existing test cases that both passed currently. But the first one expect checkbox size of 13px * 13px and the second one expech checkbox size of 11px * 11px. (I do remove the fuzzy-if(Android,1,18) of ua-style-sheet-checkbox-radio-1.html and the test case is still passed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=25ecbed817a0afe197a2590a0d67bb855a656592)
http://searchfox.org/mozilla-central/source/layout/reftests/writing-mode/ua-style-sheet-checkbox-radio-1-ref.html
http://searchfox.org/mozilla-central/source/layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized-ref.html

Do I misunderstand anything?

Comment 15

[Louis Chang [:louis]]

I guess there is something wrong here: http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#5277-5301
So I test on my device, the |widget| got from GetMinimumWidgetSize is acutally 13px but when translating to css pixel, it multiply the device pixel by 17. Which result in the |size| is smaller than the |result|, and use the original |result| in the end. I thought the AppUnitsPerDevPixel() should be 60 anyway...

Comment 16

[Louis Chang [:louis]]

(In reply to Louis Chang [:louis] from comment #15)
> I guess there is something wrong here:
> http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#5277-
> 5301
> So I test on my device, the |widget| got from GetMinimumWidgetSize is
> acutally 13px but when translating to css pixel, it multiply the device
> pixel by 17. Which result in the |size| is smaller than the |result|, and
> use the original |result| in the end. I thought the AppUnitsPerDevPixel()
> should be 60 anyway...

By the way, I hack it by declaring LogicalSize size(aWM, nsSize(widget.width * 60, widget.height * 60)) and the size of checkbox becomes normal (have minimum 13px * 13px size).

Comment 17

[Mats Palmgren (inactive)]

I think the problem is the hard-coded "13" in nsNativeThemeAndroid::GetMinimumWidgetSize.
Try making that "aPresContext->CSSPixelsToDevPixels(13)" instead.
I'm guessing the reason why you see a different result on your phone is that
it's got a high-ppi screen, so its Dev/CSS pixel ratio isn't 1.

I think it's probably better to land the reviewed patches here as is,
and file a new bug for this problem.

Comment 18

[Louis Chang [:louis]]

(In reply to Mats Palmgren (:mats) from comment #17)
> I think it's probably better to land the reviewed patches here as is,
> and file a new bug for this problem.

Ok I will then first land the patches here with the test case 113px height and unchecked/checked.
And handle the minimum widget size issue in bug Bug 1404770 also add test case of 0px height and unchecked/checked.

Comment 19

[Louis Chang [:louis]]

Also, it seems on Linux we do not handle clamping issue for checkbox/radio. So I will annotate the test case as fails-if(gtkWidget).

Comment 20

[Louis Chang [:louis]]

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

Comment 21

[Louis Chang [:louis]]

Comment on attachment 8910868 [details]
Bug 1400050 Part 1 - Clamp the width/height of checkbox/radio to the smaller one and then center it on Android.

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

Comment 22

[Louis Chang [:louis]]

Comment on attachment 8910869 [details]
Bug 1400050 Part 2 - Add test case for testing clamping issue of checkbox/radio.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/032c8557e0fd
Part 1 - Clamp the width/height of checkbox/radio to the smaller one and then center it on Android. r=mats
https://hg.mozilla.org/integration/autoland/rev/4d7d1fd7d3ad
Part 2 - Add test case for testing clamping issue of checkbox/radio. r=mats

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/032c8557e0fd
https://hg.mozilla.org/mozilla-central/rev/4d7d1fd7d3ad

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 26

[Louis Chang [:louis]]

Comment on attachment 8910868 [details]
Bug 1400050 Part 1 - Clamp the width/height of checkbox/radio to the smaller one and then center it on Android.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1352238
[User impact if declined]: checkbox/radio is distorted on Android when height or width is larger then default size.
[Is this code covered by automated tests?]: Yes, reftest included in Part 2 patch.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: All the patches in this bug.
[Is the change risky?]: NO.
[Why is the change risky/not risky?]: Only checkbox/radio on android will be affected.
[String changes made/needed]: no

Comment 27

[Louis Chang [:louis]]

Comment on attachment 8910869 [details]
Bug 1400050 Part 2 - Add test case for testing clamping issue of checkbox/radio.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1352238
[User impact if declined]: checkbox/radio is distorted on Android when height or width is larger then default size.
[Is this code covered by automated tests?]: Yes, reftest included in Part 2 patch.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: All the patches in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only checkbox/radio on android will be affected.
[String changes made/needed]: No.

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8910868 [details]
Bug 1400050 Part 1 - Clamp the width/height of checkbox/radio to the smaller one and then center it on Android.

Recent regression, Beta57+

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ad08a0e40c67
https://hg.mozilla.org/releases/mozilla-beta/rev/1552f4dc444a

Comment 30

[Andrei Vaida [:avaida]]

(In reply to Louis Chang [:louis] from comment #27)
> [Is this code covered by automated tests?]: Yes, reftest included in Part 2
> patch.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Louis's assessment on manual testing needs and the fact that this fix has automated coverage.