Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

[Bug]The images for the "Take a position" card are not correctly mirrored on RTL #9316

Closed
sv-sdeiac opened this issue Mar 23, 2020 · 8 comments
Assignees
Labels
🐞 bug Crashes, Something isn't working, .. eng:qa:verified QA Verified eng:ready Ready for engineering Feature:Onboarding First Run, Contextual Feature Recommendation/Recommender CFR good first issue Good for newcomers help wanted Help wanted from a contributor. More complex than good first issue. 🌐 L10N:RTL needs:triage Issue needs triage S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist

Comments

@sv-sdeiac
Copy link

sv-sdeiac commented Mar 23, 2020

Prerequisites

The app is freshly installed or cleared data from android app settings.

Steps to reproduce

  1. Change the language from android settings to an RTL language;
  2. Open the app;
  3. On the Onboarding screen, observe the "Take a position" card.

Expected behavior

The icons are displayed correctly in the RTL language.

Actual behavior

The Radion button hovers the context from the image.

Device information

Android device:

  • Google Pixel 3 (Android 10);
  • Samsung Galaxy Tab S6 (Android 9).

Fenix version:

  • Firefox Preview Nightly 3/23 #20830607.

Notes

  • It seems that the Images from the "take a position" card are not mirrored correctly.
    Screenshot_9

┆Issue is synchronized with this Jira Task

@sv-sdeiac sv-sdeiac added 🐞 bug Crashes, Something isn't working, .. Feature:Onboarding First Run, Contextual Feature Recommendation/Recommender CFR S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist 🌐 L10N:RTL labels Mar 23, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Mar 23, 2020
@apbitner
Copy link

Attached is a zip of the assets with correct mirroring.

RTL - Toolbar.zip

@liuche liuche added good first issue Good for newcomers help wanted Help wanted from a contributor. More complex than good first issue. labels Mar 31, 2020
@ekager ekager added the eng:ready Ready for engineering label Mar 31, 2020
@ohkio
Copy link

ohkio commented Apr 1, 2020

Was going to try out closing this issue. It seems fairly straight forward and I've read as much of the new person documentation to feel somewhat confident. Is this issue still available? When I updated the project it looked like the drawable resources were still the old png. Thanks for any support offered!

@BranescuMihai
Copy link
Contributor

BranescuMihai commented Apr 2, 2020

@Benbyday I'm in the process of optimising precisely these resources since there is a fairly large number of pngs required, and I'm using SVG + path attributes for that. This would have been a great first issue, but unfortunately it's incorporated into my task.
I'm encouraging you to look for other issues, as help is always greatly appreciated!

@ohkio
Copy link

ohkio commented Apr 2, 2020

@BranescuMihai Can you go into detail what you mean by optimizing? It sounds like something I might not have accounted for and would like to understand as best I can.

@BranescuMihai
Copy link
Contributor

@Benbyday sure! For the illustrations in question, the only difference between the dark/light versions is the border. As such, we can use only one SVG resource, and have the border as an attribute that corresponds to a different colour for each theme. So basically we'll have 4 SVGs instead of 40 PNGs:

  • 2 for toolbar top(selected/deselected) - switch between light/dark through attribute colour
  • 2 for toolbar bottom(selected/deselected) - switch between light/dark through attribute colour

The same diff is between the selected/unselected state, so I'm currently trying to use colour selectors to reduce it to only two (top/bottom)

@ohkio
Copy link

ohkio commented Apr 2, 2020

@BranescuMihai If I'm understanding correctly. Instead of having a PNG for each scenario, creating an SVG that can be scaled for most scenarios is the basics. If that's the case, it does appear as if the RTL language is not a perfect mirror of the LTR image (the magnifying glass is not mirrored when everything else is) I don't know how crucial that is, but it seemed like possibly an important observation.

Also I'm a little confused how I don't currently see a selected vs deselected PNG for this fragment, how was this being done before?

@BranescuMihai
Copy link
Contributor

Yep, scaling SVGs is basic stuff, and using attributes for path colouring is a small addition. That's correct, we cannot auto-mirror for RTL in this case, this is why we're working with UX to get the needed resources.
Selected/deselected switch wasn't done before the task I'm working on, it was a bug

@BranescuMihai BranescuMihai self-assigned this Apr 6, 2020
@BranescuMihai BranescuMihai added the 🙅 waiting Issues that are blocked or has dependencies that are not ready label Apr 24, 2020
@BranescuMihai BranescuMihai added eng:qa:needed QA Needed and removed 🙅 waiting Issues that are blocked or has dependencies that are not ready labels May 8, 2020
@lobontiumira
Copy link

Verified as fixed on the latest Nightly build from 5/8 with OnePlus 5T (Android 9), and Sony Xperia Z5 Premium (Android 7.1.1).
The "Take a position" options are now displayed mirrored in RTL languages, like in the screenshot below:

123

@lobontiumira lobontiumira added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels May 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Crashes, Something isn't working, .. eng:qa:verified QA Verified eng:ready Ready for engineering Feature:Onboarding First Run, Contextual Feature Recommendation/Recommender CFR good first issue Good for newcomers help wanted Help wanted from a contributor. More complex than good first issue. 🌐 L10N:RTL needs:triage Issue needs triage S2 Major Functionality/product severely impaired and a satisfactory workaround doesn't exist
Projects
None yet
Development

No branches or pull requests

7 participants