Closed Bug 958109 Opened 7 years ago Closed 7 years ago

Overlapping of checkbox and label on prompt

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 + verified
firefox29 + verified
firefox30 --- verified
b2g-v1.3 --- fixed
fennec 29+ ---

People

(Reporter: mkaply, Assigned: wesj)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Attached image 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
Is this a problem on beta or aurora?
tracking-fennec: --- → ?
Works on beta and aurora.
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.
Adds a menu - Show Prompt.

Click it to see the problem.
As far as finding a regression goes, is there a bisect tool for android?
(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
I did a manual bisect.

Based on those builds, it broke between:

1386819523 - WORKS
1386824502 - FAILS
Assignee: nobody → wjohnston
Attached patch Patch v1 (obsolete) — Splinter Review
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.
tracking-fennec: ? → 29+
I can't reproduce this on any devices I have. QA, can you? Otherwise, I'll close.
Keywords: qawanted
(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?
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?
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?
: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.
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attachment #8360109 - Attachment is obsolete: true
Attachment #8377693 - Flags: review?(margaret.leibovic)
Attached patch Patch (obsolete) — Splinter Review
Tiny fix.
Attachment #8377693 - Attachment is obsolete: true
Attachment #8377693 - Flags: review?(margaret.leibovic)
Attachment #8377801 - Flags: review?(margaret.leibovic)
Yes, that looks great.

The whole dialog was looking scrunched as well, and this fixes everything.
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.
Attachment #8377801 - Flags: review?(margaret.leibovic) → review-
Attached patch Patch v2Splinter Review
Sounds good.
Attachment #8377801 - Attachment is obsolete: true
Attachment #8378532 - Flags: review?(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.
Attachment #8378532 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/6c387fa25e61
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Keywords: qawanted
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.
Attachment #8378532 - Flags: approval-mozilla-beta?
Attachment #8378532 - Flags: approval-mozilla-aurora?
Attachment #8378532 - Flags: approval-mozilla-beta?
Attachment #8378532 - Flags: approval-mozilla-beta+
Attachment #8378532 - Flags: approval-mozilla-aurora?
Attachment #8378532 - Flags: approval-mozilla-aurora+
Based on the regressing bug, marking 28 affected
Verified fixed on Firefox Release 28, Firefox 29 Beta 4 and Aurora 30.0a2 (2014-04-02)
You need to log in before you can comment on or make changes to this bug.