1325096 - Activity stream - Favicons and context menu icons are wrongly displayed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Carmen Fat [:cfat] - Ecosystem QA (PTO until Oct 11th, please contact [e-mail] instead for any requests)]

Attachment: Asus Transformer Pad UI.png

Environment: 
Device:  Asus Transformer Pad (Android 4.2.1);
Build: latest Nightly 53.0a1 (2016-12-20);

Steps to reproduce:
1. Launch Firefox;
2. Observe the UI of Activity stream panel.

Expected result:
Activity stream panel is properly displayed.

Actual result:
- Favicons from the Highlights area are displayed on the down side in the website’s frame;
- Context menu icon is bigger than it should be and it’s displayed to the limit of the website’s frame. 

Notes:
This only happens on Asus Transformer Pad, on Nexus 9 (Android 7.0) and Asus ZenPad (6.0.1) the Activity stream panel is displayed properly.
Please see the screenshot attached.

Regression range:

Last good build: 2016-11-11
First bad build: 2016-11-12

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d38d06f85ef59c5dbb5d4a1a8d895957a78714de&tochange=b37be3d705d929ee52280051d58cedc70a47626f

Comment 1

[Andrzej Hunt :ahunt]

I'm able to reproduce this on a TF201T ("Transfomer Prime"), so I could debug/fix locally.

I don't have any Asus phones locally, so I've got no idea if this is specific to Asus tablets, or a generic Asus bug.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #1)
> I'm able to reproduce this on a TF201T ("Transfomer Prime"), so I could
> debug/fix locally.

That would be perfect.

> I don't have any Asus phones locally, so I've got no idea if this is
> specific to Asus tablets, or a generic Asus bug.

Maybe it comes apparent when you know the reason for it. But according to comment 0 it works on an Asus ZenPad. Maybe it's an Android 4 + Asus thing?

Comment 3

[Andrzej Hunt :ahunt]

Also reproduceable on a Motorola Droid 4 running 4.1.2 (which is a phone).

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Bug 1325096 - Fix menu button padding on select devices

On some devices, ImageView appears to lose it's padding when calling
setBackground[Drawable](). We therefore need to save and restore the
padding on those devices.

Known affected devices are various Asus Transformer tablets,
and also the Motorola Droid 4, running various Android 4.X versions.
We therefore make this workaround conditional on Android 4 to minimise
the number of devices where this workaround is used. (Having the conditional
should also make it easy to notice when we can remove the workaround, i.e.
whenever Android 5 becomes the baseline.)

Review commit: https://reviewboard.mozilla.org/r/106886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106886/

Comment 5

[Andrzej Hunt :ahunt]

Attachment: Bug 1325096 - Wrap FaviconView to workaround ignored margin

This seems to be an Android platform bug affecting Android 4.X, which
is visible primarily on tablets (on phones, page titles are often broken
over two lines, which isn't the case on tablets, hence it's easy
to spot if this bug is happening).

Review commit: https://reviewboard.mozilla.org/r/106888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106888/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: Bug 1325096 - Post: only use deprecated setBackgroundDrawable on older platforms

This has no benefit right now, but this lets us avoid forgetting to upgrade to the non-deprecated
method once we no longer need to support api 15.

Review commit: https://reviewboard.mozilla.org/r/106890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106890/

Comment 7

[Max Liu [:maliu]]

Comment on attachment 8829922 [details]
Bug 1325096 - Fix menu button padding on select devices

https://reviewboard.mozilla.org/r/106886/#review108166

nit: Maybe seperate padding preserve things from enableTouchRipple to make it reusable for other view reference to setBackgroundDrawable. See comment on 3/3 patch

Comment 8

[Max Liu [:maliu]]

Comment on attachment 8829923 [details]
Bug 1325096 - Wrap FaviconView to workaround ignored margin

https://reviewboard.mozilla.org/r/106888/#review108172

LGTM

Comment 9

[Max Liu [:maliu]]

Comment on attachment 8829924 [details]
Bug 1325096 - Post: only use deprecated setBackgroundDrawable on older platforms

https://reviewboard.mozilla.org/r/106890/#review108158

::: mobile/android/base/java/org/mozilla/gecko/util/ViewUtil.java:67
(Diff revision 1)
>          // This call is deprecated, but the replacement setBackground(Drawable) isn't available
>          // until API 16.
> -        view.setBackgroundDrawable(backgroundDrawableArray.getDrawable(0));
> +        if (AppConstants.Versions.feature16Plus) {
> +            view.setBackground(backgroundDrawable);
> +        } else {
> +            view.setBackgroundDrawable(backgroundDrawable);
> +        }

nit: Since padding lost is a framework bug cound happen to any where else, how about wrap this block with padding preserve and new api compatible together?

```java
    @TargetApi(Build.VERSION_CODES.JELLY_BEAN)
    public static void setBackground(View view, Drawable drawable) {
        // On certain older devices (e.g. ASUS TF200T, Motorola Droid 4), setting a background
        // drawable results in the padding getting wiped. We work around this by saving the pre-existing
        // padding, followed by restoring it at the end in view.setPadding().

        final int paddingLeft = view.getPaddingLeft();
        final int paddingTop = view.getPaddingTop();
        final int paddingRight = view.getPaddingRight();
        final int paddingBottom = view.getPaddingBottom();

        if(AppConstants.Versions.preJB) {
            // This call is deprecated, but the replacement setBackground(Drawable) isn't available
            // until API 16.
            view.setBackgroundDrawable(drawable);
        } else {
            view.setBackground(drawable);
        }

        if (AppConstants.Versions.preLollipop) {
            view.setPadding(paddingLeft, paddingTop, paddingRight, paddingBottom);
        }
    }
```

Comment 10

[Max Liu [:maliu]]

Comment on attachment 8829924 [details]
Bug 1325096 - Post: only use deprecated setBackgroundDrawable on older platforms

https://reviewboard.mozilla.org/r/106890/#review108468

::: mobile/android/base/java/org/mozilla/gecko/util/ViewUtil.java:59
(Diff revision 1)
>          }
>      }
>  
> +    @TargetApi(Build.VERSION_CODES.JELLY_BEAN)
>      private static void setTouchRipple(final View view) {
>          final TypedArray backgroundDrawableArray;

nit: backgroundDrawableArray.recycle()

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8829924 [details]
Bug 1325096 - Post: only use deprecated setBackgroundDrawable on older platforms

https://reviewboard.mozilla.org/r/106890/#review109662

::: mobile/android/base/java/org/mozilla/gecko/util/ViewUtil.java:59
(Diff revision 1)
>          }
>      }
>  
> +    @TargetApi(Build.VERSION_CODES.JELLY_BEAN)
>      private static void setTouchRipple(final View view) {
>          final TypedArray backgroundDrawableArray;

BTW feel free to use the "open an issue" checkbox: that then forces the patch author to "fix" (or drop) the issue before pushing, which helps ensure that your comment gets addressed!

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8829922 [details]
Bug 1325096 - Fix menu button padding on select devices

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106886/diff/1-2/

Comment 13

[Andrzej Hunt :ahunt]

Comment on attachment 8829923 [details]
Bug 1325096 - Wrap FaviconView to workaround ignored margin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106888/diff/1-2/

Comment 14

[Andrzej Hunt :ahunt]

Comment on attachment 8829924 [details]
Bug 1325096 - Post: only use deprecated setBackgroundDrawable on older platforms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106890/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e13b9d48a99a
Fix menu button padding on select devices r=maliu
https://hg.mozilla.org/integration/autoland/rev/bbde80c84235
Wrap FaviconView to workaround ignored margin r=maliu
https://hg.mozilla.org/integration/autoland/rev/83dc8e87b382
Post: only use deprecated setBackgroundDrawable on older platforms r=maliu

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e13b9d48a99a
https://hg.mozilla.org/mozilla-central/rev/bbde80c84235
https://hg.mozilla.org/mozilla-central/rev/83dc8e87b382

Comment 17

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 54.0a1 (2017-2-2), on the following devices:
- Asus Transformer Pad TF300T (Android 4.2.1)
- Motorola Razr (Android 4.4.4)
- Lenovo Yoga Tablet 2 (Android 4.4.2)
- Samsung Galaxy Note 4 (Android 5.0.1)