958109 - Overlapping of checkbox and label on prompt

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Kaply [:mkaply]]

Attachment: Screenshot of problem

On nightly, Using prompt.jsm to display a prompt with a checkbox is overlapping the checkbox and its label.

Also, the dialog overall looks quite scrunched.

It works fine on Firefox 26.

This is an HP Touchpad tablet 1024x768

Comment 1

[:Margaret Leibovic]

Is this a problem on beta or aurora?

Comment 2

[Mike Kaply [:mkaply]]

Works on beta and aurora.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Hopefully this regressed less than a month ago. If it did the regression range can be found via http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/ else we could only get a day resolution from nightly builds. 

Can a sample extension be provided or find the regression range as noted above.

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Simple extension to show problem.

Adds a menu - Show Prompt.

Click it to see the problem.

Comment 5

[Mike Kaply [:mkaply]]

As far as finding a regression goes, is there a bisect tool for android?

Comment 6

[Aaron Train [:aaronmt]]

(In reply to Mike Kaply (:mkaply) from comment #5)
> As far as finding a regression goes, is there a bisect tool for android?

https://github.com/mozilla/mozregression

Comment 7

[Mike Kaply [:mkaply]]

I did a manual bisect.

Based on those builds, it broke between:

1386819523 - WORKS
1386824502 - FAILS

Comment 8

[Aaron Train [:aaronmt]]

Thanks.

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2585c276abf3&tochange=c7b7b00e867f

Wes Johnston — Bug 875750 - Provide a basic color picker for Android. r=mfinkle
http://hg.mozilla.org/integration/mozilla-inbound/rev/9101f35b7733

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Hmm... I Can't seem to reproduce this, but I know I've seen it before. Will try some other devices....

This fixes a small change that occurred when we flipped the color picker on. I'd also like to try and fixup these stylings in general.

Comment 10

[Wesley Johnston (:wesj)]

I can't reproduce this on any devices I have. QA, can you? Otherwise, I'll close.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mike Kaply (:mkaply) from comment #0)

> This is an HP Touchpad tablet 1024x768

Mike - Do you have any info on what version of Android the HP touchpad is using or was built from?

Comment 12

[Mike Kaply [:mkaply]]

Android 4.04.

But Android version should not matter here.

This worked before and now it fails due to a Firefox change.

And it still happens to me on the latest nightly.

How can it be Android version specific if it works on release Firefox but fails on nightly?

Comment 13

[Wesley Johnston (:wesj)]

Its apparently Android AND Firefox version specific :) I refactored some of the styling code at some point, so I'm sure that's what broke this. QA, do we have a touchpad I can use?

Comment 14

[Mike Kaply [:mkaply]]

:wesj:

I'm a dev, so if it's straightforward to do anything locally, I'm willing to do that. I haven't built Fennec yet.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Nope. Figured out what's going on i think. On older Android versions, the text is positioned next to the item by adding padding. On newer versions, the padding is added inside the drawable. BUT the padding is also added inside the background drawable for these widgets, which also isn't what we wanted. The best solution I've found is just to wrap these Views.

I only have my phone handy for testing, so if you want to test this for me mike, here's a build:
http://people.mozilla.org/~wjohnston/padding.apk

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch

Tiny fix.

Comment 17

[Mike Kaply [:mkaply]]

Yes, that looks great.

The whole dialog was looking scrunched as well, and this fixes everything.

Comment 18

[:Margaret Leibovic]

Comment on attachment 8377801 [details] [diff] [review]
Patch

Review of attachment 8377801 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/prompts/Prompt.java
@@ +315,5 @@
>      }
>  
> +
> +    private View wrapView(View v) {
> +        LinearLayout linearLayout = new LinearLayout(mContext);

Nit: final

@@ +317,5 @@
> +
> +    private View wrapView(View v) {
> +        LinearLayout linearLayout = new LinearLayout(mContext);
> +        linearLayout.setOrientation(LinearLayout.VERTICAL);
> +        applyInputStyle(linearLayout, mInputs[0]);

mInputs[0]? This looks wrong.

Instead of passing in a view as a parameter, I think you should pass in the PromptInput as a parameter to this method (which you can use here), and rename the method wrapInput. It already depends on the view being an input view, since its calling applyInputStyle, so I would prefer we be explicit about it.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Sounds good.

Comment 20

[:Margaret Leibovic]

Comment on attachment 8378532 [details] [diff] [review]
Patch v2

Review of attachment 8378532 [details] [diff] [review]:
-----------------------------------------------------------------

Adding extra views to the view hierarchy seems like a sub-optimal solution, but I think it's fine in this case, since this only affects prompts, and the views go away once the prompt goes away.

::: mobile/android/base/prompts/Prompt.java
@@ +314,5 @@
>          mSelected = null;
>      }
>  
> +
> +    private View wrapView(final PromptInput input) {

I think you should rename this method wrapInput. Also, it would be good to add some comments explaining why we need this extra view.

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/6c387fa25e61

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c387fa25e61

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 8378532 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 875750. We uplifted this to 28, so I imagine this bug is present there now.
User impact if declined: visual problems on prompts
Testing completed (on m-c, etc.): landed on mc this week
Risk to taking this patch (and alternatives if risky): pretty simple patch. just wraps these inputs in something and sets the padding there. we could alternatively back out the color picker. there are some alternative patches but this is probably the simplest of them.
String or IDL/UUID changes made by this patch: none.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Based on the regressing bug, marking 28 affected

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e06f378dcd44
https://hg.mozilla.org/releases/mozilla-beta/rev/8025e17b77d5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8025e17b77d5

Comment 27

[Catalin Suciu [:csuciu]]

Verified fixed on Firefox Release 28, Firefox 29 Beta 4 and Aurora 30.0a2 (2014-04-02)