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

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

Attachment: 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.

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Patch: search padding

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.

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Screenshot: padding with brand icon

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Screenshot: padding with "x" button

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Patch: search padding

...forgot to qref.

Comment 6

[: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.

Comment 7

[:Margaret Leibovic]

What's the status here? It would be nice to fix this and uplift it to 35 for the search activity v1 release.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Patch: search padding v2

xml-only way of changing this. I like it better.

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Screenshot: "x" not overlapping

Comment 10

[:Margaret Leibovic]

Comment on attachment 8511357 [details] [diff] [review]
Patch: search padding v2

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

Nice.

::: 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".

Comment 11

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/3cd24f004c1a

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Patch: Search padding

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

Comment 13

[:Margaret Leibovic]

The search activity is only enabled in aurora, so I don't think we need to uplift this to beta.

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8512088 [details] [diff] [review]
Patch: Search padding

Gotcha, thanks!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android robocop NPEs.
https://hg.mozilla.org/integration/fx-team/rev/b547004f9c09

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1006944&repo=fx-team

Comment 16

[Chenxia Liu [:liuche]]

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

Comment 17

[Chenxia Liu [:liuche]]

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

https://hg.mozilla.org/integration/fx-team/rev/9e878bc102e3

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by: waiting for successful landing to central before approving aurora uplift.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9eea6cacf02a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf0c99616666

Comment 21

[Flaviu Cos]

Verified as fixed in builds:
- 36.0a1 2014-10-30;
- 35.0a2 2014-10-30;
Device: Nexus 4 (Android 4.4.4).