Closed Bug 1278725 Opened 3 years ago Closed 3 years ago
Bookmark "back" arrow is vertically squished
76.39 KB, image/png
58 bytes, text/x-review-board-request
106.10 KB, image/png
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.
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
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/integration/fx-team/rev/e8d3a286f70ed10ad64ca62287115e55fe26c674 Bug 1278725 - Restore original "up" arrow scaling r=liuche
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.
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.
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
You need to log in before you can comment on or make changes to this bug.