1278725 - Bookmark "back" arrow is vertically squished

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Squished bookmark folder back arrow

Seeing this on the 6/07 Nightly build, but only on a Nexus 5 (not Nexus 9 or Moto X 1st Gen).

Comment 1

[Chenxia Liu [:liuche]]

ahunt, know of any recent changes that might have caused this?

The History panel icons are all still fine.

Comment 2

[:Margaret Leibovic]

Does this affect 49, or is it a more recent regression?

Comment 3

[Andrzej Hunt :ahunt]

Most likely caused by Bug 1269001 where we switched to TwoLinePageRow. I can look into this?

I believe that affects 48, 49, 50, I'll try and verify that too.

Comment 4

[Andrzej Hunt :ahunt]

I've got a patch which restores the original appearance, although I don't really understand all the padding calculations involved which set the size of the icons before the change (20dp drawable padding, for a layout with 64dp height, which leaves 24dp - but to get the correct height with our new layout I'm using 18dp height for the I).

Testing on both the N5 and N7 (both affected) this patch restores the original (46) appearance, on an N9 (unaffected) the appearance of the arrow or folders doesn't change with the patch.

Comment 5

[Andrzej Hunt :ahunt]

Attachment: Bug 1278725 - Restore original "up" arrow scaling

Before introducing the folder count, the bookmark folder icon
was set as the drawable on a TextView. This actually resulted
in scaling that doesn't preserve the aspect ratio. To reproduce
this in the new ImageView (added when we switched to a two-line
folder layout), we simply need to set the icon size explicitly
and scale to fit the whole area (as opposed to setting two
constraints and fitting while preserving aspect ratio).

The folder/reading-list icons are unaffected as they already
have the expected aspect-ratio, the back-arrow is now scaled
as expected.

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

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8761726 [details]
Bug 1278725 - Restore original "up" arrow scaling

https://reviewboard.mozilla.org/r/58844/#review55808

::: mobile/android/base/resources/layout/two_line_folder_row.xml:15
(Diff revision 1)
>  
>      <ImageView android:id="@+id/icon"
>                 android:src="@drawable/folder_closed"
>                 android:layout_width="24dp"
> -               android:layout_height="24dp"
> -               android:scaleType="fitCenter"
> +               android:layout_height="18dp"
> +               android:scaleType="fitXY"

Does this even need to be scaled? If the image is the right size you could just use center (which doesn't center) and not need to adjust the height.

This is fine too, but if the other is simpler (e.g., no scaling) I'd prefer that.

Comment 7

[Andrzej Hunt :ahunt]

(In reply to Chenxia Liu [:liuche] from comment #6)
> Comment on attachment 8761726 [details]
> Bug 1278725 - Restore original "up" arrow scaling
> 
> https://reviewboard.mozilla.org/r/58844/#review55808
> 
> ::: mobile/android/base/resources/layout/two_line_folder_row.xml:15
> (Diff revision 1)
> >  
> >      <ImageView android:id="@+id/icon"
> >                 android:src="@drawable/folder_closed"
> >                 android:layout_width="24dp"
> > -               android:layout_height="24dp"
> > -               android:scaleType="fitCenter"
> > +               android:layout_height="18dp"
> > +               android:scaleType="fitXY"
> 
> Does this even need to be scaled? If the image is the right size you could
> just use center (which doesn't center) and not need to adjust the height.
> 
> This is fine too, but if the other is simpler (e.g., no scaling) I'd prefer
> that.

Without scaling the image is quite small, i.e. an even worse situation than what we have right now - I think we end up scaling these images on most devices. (I don't entirely understand why the scaling is breaking on the N5 though - with fitCenter the aspect ratio should have been preserved but it isn't preserved.)

Comment 8

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/e8d3a286f70ed10ad64ca62287115e55fe26c674
Bug 1278725 - Restore original "up" arrow scaling r=liuche

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8d3a286f70e

Comment 10

[Alex Caziuc]

Attachment: Back Button.png

Verified using:
Device: Galaxy S6 Edge(Android 5.1.1)
Build: Firefox for Android Nightly - 50.0a1(2016-06-16)

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Is this something you would like to uplift to aurora? I am marking it fix-optional for both aurora and beta, because it doesn't seem like a big regression --  but let me know if you think it is worth uplifting & safe to do.

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8761726 [details]
Bug 1278725 - Restore original "up" arrow scaling

This is a simple and low-risk change, so I think it's worth uplifting!

Approval Request Comment
[Feature/regressing bug #]: Bug 1269001
[User impact if declined]: Bookmark folder back/up arrow has changed appearance in 2 releases.
[Describe test coverage new/current, TreeHerder]: manual testing.
[Risks and why]: low risk: layout attributes for bookmark folder icons has been changed, resulting in different scaling (which restores the original appearance).
[String/UUID change made/needed]: none.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8761726 [details]
Bug 1278725 - Restore original "up" arrow scaling

This patch fixes a UI regression. Take it in 48 beta 8 and aurora. Should be in fennec 48 beta 8. This also needs to be verified.

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f9e8e67acda

Comment 16

[u549602]

Verified as fixed on 48.0b9, Aurora 49.0a2/2016-07-18 using a Samsung Galaxy S6 Edge (Android 6.0.1) and on a Samsung Galaxy Tab S2(Android 5.0.2)

Comment 17

[Alex Caziuc]

I'm changing the status to verified fixed considering that the bug is verified on all versions.