Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[RTL] The back arrow is pointing left instead of right

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
Theme and Visual Design
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: sorina, Assigned: maliu)

Tracking

(Blocks: 1 bug)

53 Branch
Firefox 53
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
Environment: 
Device: Huawei Honor (Android 5.1.1);
Build: (AR) Nightly 53.0a1 (2016-12-15);

Steps to reproduce:
1. Launch Fennec; 
2. Go to Settings;
3. Observe the back arrow.

Expected result:
The back arrow is pointing right.

Actual result:
The back arrow is pointing left.

Notes:
1. See the screenshot: https://imgur.com/DJ83rqO
2. I was able to reproduce also on LG G4 (Android 5.1)

Updated

7 months ago
Blocks: 928663
(Assignee)

Comment 1

7 months ago
This symptom should only happen on platform 17 & 18 because the solution api:"Drawable.setAutoMirrored" is not supported until kitkat and above.
According to the test result, LG(Android 5.1) should not have this symptom and also we still need to fix on platform 17 & 18. 
So I'll fallback to solution by providing mirrored drawable to related resource folder(-ldrtl).
Assignee: nobody → max
(Assignee)

Updated

7 months ago
Blocks: 1322119
Comment hidden (mozreview-request)

Comment 3

7 months ago
mozreview-review
Comment on attachment 8819276 [details]
Bug 1323763 - Revert setAutomirrored solution and provide drawable in ldrtl folder,

https://reviewboard.mozilla.org/r/99126/#review99430

1) Do you know why this didn't work on an Android 5.1 device?
2) I'm not really happy about duplicating the resources. Is there a way to create an XML drawable that references the original bitmap and mirrors it?
3) Did you run ImageOptim to reduce the PNG size?

Comment 4

7 months ago
mozreview-review
Comment on attachment 8819276 [details]
Bug 1323763 - Revert setAutomirrored solution and provide drawable in ldrtl folder,

https://reviewboard.mozilla.org/r/99126/#review99876

See questions in bug.

It would be nice if we could find a way to avoid a size hit from duplicating the resources.
Attachment #8819276 - Flags: review?(s.kaspari)

Comment 5

7 months ago
mozreview-review-reply
Comment on attachment 8819276 [details]
Bug 1323763 - Revert setAutomirrored solution and provide drawable in ldrtl folder,

https://reviewboard.mozilla.org/r/99126/#review99876

I guess that `transform:scaleX(-1)` would be enough for flipping images without shadows etc.
(Assignee)

Comment 6

7 months ago
(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> 1) Do you know why this didn't work on an Android 5.1 device?
Couldn't be sure what the root cause is, I observed that the layout direction of the View which drawable attached to is not following parent RTL status on KK & L device. But M device have no such issue.

> 2) I'm not really happy about duplicating the resources. Is there a way to
> create an XML drawable that references the original bitmap and mirrors it?
I'm not comfortable with that neither. :(
There is some other way to do rotate/scale/clip on a drawable, but no matrix(mirror/flip) operation available on resource xml.
Though I can flip the drawable programatically when view is RTL, but that is easy only when it comes to BitmapDrawable.
Our current scenario where this patch includes are:
* StateListDrawable (ex. MenuItemDefault/MenuItemActionBar)
* NinePatchDrawable (ex. urlBarTranslatingEdge)
* ClipDrawable (ex. ToolbarProgressView)
They all have different implementation in android framework, mostly by matrix(1.-1) on draw when ldrtl.

> 3) Did you run ImageOptim to reduce the PNG size?
I did not know we have to do this manually. I thought it's been done by png crunch during build process. Wait... we're not using gradle build right? >_<
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 7

7 months ago
(In reply to Tomer Cohen :tomer from comment #5)
> I guess that `transform:scaleX(-1)` would be enough for flipping images
> without shadows etc.
I guess this syntax(transform:scaleX) is from CSS right? 

We can also do this(scaleX:-1) on an ImageView and wrap the value into ldrtl resource folder or make it styleable.
Unfortunately, lot of our scenario are not ImageView, and it would be perfect if we can do this on Drawable.
Comment hidden (mozreview-request)
(In reply to Max Liu [:maliu] from comment #6)
> > 3) Did you run ImageOptim to reduce the PNG size?
> I did not know we have to do this manually. I thought it's been done by png
> crunch during build process. Wait... we're not using gradle build right? >_<

Yeah, and even for gradle builds we realized that we can do better, see:
https://medium.com/@duhroach/smaller-pngs-and-android-s-aapt-tool-4ce38a24019d
Flags: needinfo?(s.kaspari)
tracking-fennec: --- → ?
tracking-fennec: ? → ---
Blocks: 1319302
Ahunt, can you take this one?
Attachment #8819276 - Flags: review?(ahunt)
Attachment #8819276 - Flags: review?(s.kaspari)

Comment 11

7 months ago
mozreview-review
Comment on attachment 8819276 [details]
Bug 1323763 - Revert setAutomirrored solution and provide drawable in ldrtl folder,

https://reviewboard.mozilla.org/r/99126/#review101344

Looks good to me!

I wonder what the apk size impact will be though - it would be good to get VectorDrawables for our icons if we have to have duplicates.
Attachment #8819276 - Flags: review?(ahunt) → review+
Oh, yeah, maybe we can file a follow-up for replacing those icons with vector drawables?
(Assignee)

Comment 13

7 months ago
mozreview-review-reply
Comment on attachment 8819276 [details]
Bug 1323763 - Revert setAutomirrored solution and provide drawable in ldrtl folder,

https://reviewboard.mozilla.org/r/99126/#review101344

Total size of pngs after ImageOptim is about 7.5k
(Assignee)

Comment 14

7 months ago
(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> Oh, yeah, maybe we can file a follow-up for replacing those icons with
> vector drawables?

VectorDrawable is a great choice with this kind of images, but that requires api level 21 & above. I would recommend replacing all png to webp first, unfortunately, fully support(transparent/loseless) of webp also requires api level 17 & above.

Here is a great talk of dealing with images from Colt. Skip to 38m20s for an easy decision tree.
https://youtu.be/r_LpCi6DQME?t=38m20s

Maybe we can use "apk split" function from android gradle plugin to generate multiple flavor of apks all at once.
If we deploy multiple apks with different minSdkVersion. We can put different format of images accordingly.
That can benefit to both Google Play users(download/upgrade size) and bundle in-rom customers(charge with apk size).

Maybe we should file a follow-up bug to remember remove these duplicated images when someday minSdkVersion is raised to 19 up or we deploy multiple apks by minSdkVersion.
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 15

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8c6d9f66b68a
Revert setAutomirrored solution and provide drawable in ldrtl folder, r=ahunt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c6d9f66b68a
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(In reply to Max Liu [:maliu] from comment #14)
> (In reply to Sebastian Kaspari (:sebastian) from comment #12)
> > Oh, yeah, maybe we can file a follow-up for replacing those icons with
> > vector drawables?
> 
> VectorDrawable is a great choice with this kind of images, but that requires
> api level 21 & above. I would recommend replacing all png to webp first,
> unfortunately, fully support(transparent/loseless) of webp also requires api
> level 17 & above.

We have been using VectorDrawables in Activity Stream via the support library implementation. Ahunt has also researched using webp for some of our images. More here: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-December/002187.html
(Reporter)

Comment 18

7 months ago
Tested with LG G4 (Android 5.1), build: (AR - http://archive.mozilla.org/pub/mobile/nightly/2017/01/2017-01-04-03-02-14-mozilla-central-android-api-15-l10n/) Nightly 53.0a1 (2017-01-04) and the issue is reproducible.

Comment 19

6 months ago
Hey Max - will you follow up here or should we file a new bug?
Flags: needinfo?(max)
(Assignee)

Comment 20

6 months ago
(In reply to Ioana Chiorean from comment #19)
> Hey Max - will you follow up here or should we file a new bug?

Yes, file a new bug please.
Flags: needinfo?(max)
(Reporter)

Comment 21

6 months ago
Based on comment 20, tested again with LG G4 on latest Nightly and file Bug 1333089.
You need to log in before you can comment on or make changes to this bug.