Closed Bug 1278725 Opened 3 years ago Closed 3 years ago

Bookmark "back" arrow is vertically squished

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox48 --- verified
firefox49 --- verified
fennec + ---
firefox50 --- verified

People

(Reporter: liuche, Assigned: ahunt)

Details

(Keywords: regression)

Attachments

(3 files)

Seeing this on the 6/07 Nightly build, but only on a Nexus 5 (not Nexus 9 or Moto X 1st Gen).
ahunt, know of any recent changes that might have caused this?

The History panel icons are all still fine.
Flags: needinfo?(ahunt)
Does this affect 49, or is it a more recent regression?
tracking-fennec: --- → ?
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.
Assignee: nobody → ahunt
Flags: needinfo?(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.
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/
Attachment #8761726 - Flags: review?(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.
Attachment #8761726 - Flags: review?(liuche) → review+
(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.)
https://hg.mozilla.org/mozilla-central/rev/e8d3a286f70e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Attached image Back Button.png
Verified using:
Device: Galaxy S6 Edge(Android 5.1.1)
Build: Firefox for Android Nightly - 50.0a1(2016-06-16)
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 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.
Flags: needinfo?(ahunt)
Attachment #8761726 - Flags: approval-mozilla-beta?
Attachment #8761726 - Flags: approval-mozilla-aurora?
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.
Attachment #8761726 - Flags: approval-mozilla-beta?
Attachment #8761726 - Flags: approval-mozilla-beta+
Attachment #8761726 - Flags: approval-mozilla-aurora?
Attachment #8761726 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
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)
I'm changing the status to verified fixed considering that the bug is verified on all versions.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.