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

VERIFIED FIXED in Firefox 35


5 years ago
2 years ago


(Reporter: Margaret, Assigned: liuche)


Firefox 36
Dependency tree / graph

Firefox Tracking Flags

(firefox35 verified, firefox36 verified, fennec35+)


(Whiteboard: shovel-ready)


(3 attachments, 5 obsolete attachments)

Posted 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.
Duplicate of this bug: 1071608
Posted 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)
Posted image Screenshot: padding with brand icon (obsolete) —
Posted image Screenshot: padding with "x" button (obsolete) —
Posted 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/BackCaptureEditText.java
@@ +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.
Posted 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: https://treeherder.mozilla.org/ui/#/jobs?repo=fx-team&revision=a06771fa469d
Flags: needinfo?(ryanvm)
Re-landing because the fx-team push was green and the NPE seems intermittent: https://treeherder.mozilla.org/ui/#/jobs?repo=fx-team&revision=3cd24f004c1a

Flags: needinfo?(ryanvm)
Triage drive-by: waiting for successful landing to central before approving aurora uplift.
Closed: 5 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.