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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sorina Florean [:sflorean]]

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)

Comment 1

[Max Liu [:maliu]]

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).

Comment 2

[Max Liu [:maliu]]

Attachment: Bug 1323763 - Revert setAutomirrored solution and provide drawable in ldrtl folder,

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

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

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

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 5

[Tomer Cohen :tomer]

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.

Comment 6

[Max Liu [:maliu]]

(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? >_<

Comment 7

[Max Liu [:maliu]]

(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 8

[Max Liu [:maliu]]

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

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(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

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Ahunt, can you take this one?

Comment 11

[Andrzej Hunt :ahunt]

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.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, yeah, maybe we can file a follow-up for replacing those icons with vector drawables?

Comment 13

[Max Liu [:maliu]]

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

Comment 14

[Max Liu [:maliu]]

(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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c6d9f66b68a
Revert setAutomirrored solution and provide drawable in ldrtl folder, r=ahunt

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8c6d9f66b68a

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(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

Comment 18

[Sorina Florean [:sflorean]]

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

[Ioana Chiorean]

Hey Max - will you follow up here or should we file a new bug?

Comment 20

[Max Liu [:maliu]]

(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.

Comment 21

[Sorina Florean [:sflorean]]

Based on comment 20, tested again with LG G4 on latest Nightly and file Bug 1333089.