Closed Bug 1068411 Opened 10 years ago Closed 10 years ago

Long search text runs under "x" button in search bar


(Firefox for Android Graveyard :: Search Activity, defect)

Not set


(firefox35 verified, firefox36 verified, fennec35+)

Firefox 36
Tracking Status
firefox35 --- verified
firefox36 --- verified
fennec 35+ ---


(Reporter: Margaret, Assigned: liuche)



(Whiteboard: shovel-ready)


(3 files, 5 obsolete files)

Attached image screenshot
We probably just need to add more padding to the right side of the EditText. The only potentially tricky thing here is that the underline is created from a background on the EditText, and we want to make sure the "x" button is still above that underline.
Attached patch Patch: search padding (obsolete) — Splinter Review
Looks like we need to override compound drawable padding (instead of just paddingLeft or something) because we're using setIntrinsicCompoundDrawableBounds. Hard-coded, because we don't know how big the icon is when building the search bar.
Assignee: nobody → liuche
Attachment #8495629 - Flags: review?(margaret.leibovic)
Attached image Screenshot: padding with brand icon (obsolete) —
Attached image Screenshot: padding with "x" button (obsolete) —
Attached patch Patch: search padding (obsolete) — Splinter Review
...forgot to qref.
Attachment #8495629 - Attachment is obsolete: true
Attachment #8495629 - Flags: review?(margaret.leibovic)
Attachment #8496081 - Flags: review?(margaret.leibovic)
Comment on attachment 8496081 [details] [diff] [review]
Patch: search padding

Review of attachment 8496081 [details] [diff] [review]:

::: mobile/android/search/java/org/mozilla/search/ui/
@@ +15,5 @@
>   */
>  public class BackCaptureEditText extends EditText {
> +
> +    // Padding for search engine icon/clear button.
> +    private static final int PADDING_RIGHT = 60;

Won't this need to vary depending on the screen resolution? I think we should put this value in dimens.xml and use getDimensionPixelSize to get a value.

Perhaps we should even create a new attribute for this value, and then specify it in the XML layout, since that would make it easier to look at the layout file to determine what's going on.

@@ +39,5 @@
>      }
> +
> +    @Override
> +    public int getCompoundPaddingRight() {
> +        return PADDING_RIGHT;

I'm okay with us customizing this view like this, but this does make this view more specialized for its use in SearchBar. I'm fine with that, since it's not used for anything else, but maybe we should rename it SearchEditText or something less generic.
Attachment #8496081 - Flags: review?(margaret.leibovic) → feedback+
What's the status here? It would be nice to fix this and uplift it to 35 for the search activity v1 release.
Attached patch Patch: search padding v2 (obsolete) — Splinter Review
xml-only way of changing this. I like it better.
Attachment #8496081 - Attachment is obsolete: true
Attachment #8511357 - Flags: review?(margaret.leibovic)
Attachment #8495630 - Attachment is obsolete: true
Attachment #8495631 - Attachment is obsolete: true
tracking-fennec: --- → ?
Comment on attachment 8511357 [details] [diff] [review]
Patch: search padding v2

Review of attachment 8511357 [details] [diff] [review]:


::: mobile/android/base/resources/drawable/edit_text_default.xml
@@ +9,5 @@
>          android:top="-2dp"
>          android:right="-2dp"
>          android:left="-2dp">
>          <shape>
>              <!-- Padding creates vertical space between the text and the underline -->

Nit: Update this comment to also mention that the right padding accounts for the "x".
Attachment #8511357 - Flags: review?(margaret.leibovic) → review+
Target Milestone: --- → Firefox 36
Approval Request Comment
[Feature/regressing bug #]: Right padding so text doesn't overlap icons
[User impact if declined]: Text will overlap search icon
[Describe test coverage new/current, TBPL]: local
[Risks and why]: very low, adds a tiny layout change
[String/UUID change made/needed]: none
Attachment #8511357 - Attachment is obsolete: true
Attachment #8512088 - Flags: approval-mozilla-beta?
Attachment #8512088 - Flags: approval-mozilla-aurora?
The search activity is only enabled in aurora, so I don't think we need to uplift this to beta.
Comment on attachment 8512088 [details] [diff] [review]
Patch: Search padding

Gotcha, thanks!
Attachment #8512088 - Flags: approval-mozilla-beta?
The S4 test for this push is green, and it looks like it might be an intermittent since it greens up a couple of commits later, before the backout:
Flags: needinfo?(ryanvm)
Re-landing because the fx-team push was green and the NPE seems intermittent:
Flags: needinfo?(ryanvm)
Triage drive-by: waiting for successful landing to central before approving aurora uplift.
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #8512088 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- 36.0a1 2014-10-30;
- 35.0a2 2014-10-30;
Device: Nexus 4 (Android 4.4.4).
tracking-fennec: ? → 35+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.