Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed text alignment for RTL selects #5689

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Jul 20, 2023

Closes #5684

What has been done to verify that this works as intended?

I've tested the fix manually.

Why is this the best possible solution? Were any other approaches considered?

There is nothin to discuss here.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Please test the fix on many different Android versions. It's important to check those older - Android 5.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@@ -137,8 +137,6 @@ void setUpButton(TextView button, int index) {
button.setTextSize(TypedValue.COMPLEX_UNIT_DIP, QuestionFontSizeUtils.getQuestionFontSize());
button.setText(HtmlUtils.textToHtml(prompt.getSelectChoiceText(filteredItems.get(index))));
button.setTag(items.indexOf(filteredItems.get(index)));
button.setGravity(LocalizedApplicationKt.isLTR(context) ? Gravity.START : Gravity.END);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we no longer need to do that programmatically.

@grzesiek2010 grzesiek2010 requested a review from seadowg July 20, 2023 11:51
@dbemke
Copy link

dbemke commented Jul 26, 2023

In All widgets in selects with RTL applied radio buttons are to the right but choices are too the left.
Verified on the CI built app on Android 10,13. On the master version the issue doesn't occur.
rtlSelect

@srujner
Copy link

srujner commented Jul 26, 2023

I've also got few screens to compare behaviors on Android 5.1 and 13

Android 5.1 at the top, 13 at the bottom
Screenshot from 2023-07-26 15-34-34

@grzesiek2010
Copy link
Member Author

I think this is the right behavior and forcing text that comes from an LTR language like English to be displayed on the right was wrong.
See the last point
image
from https://m2.material.io/design/usability/bidirectionality.html#mirroring-layout

@seadowg do you agree?

@seadowg
Copy link
Member

seadowg commented Aug 3, 2023

Not sure I understand the problem here. Either way, I reckon this would be better to discuss with @alyblenkin.

@grzesiek2010
Copy link
Member Author

Not sure I understand the problem here.

We used to force text that is from LTR languages to be displayed on the right side if the layout is RTL and the other way around. To me, this approach was wrong and also caused issues like #5684

@alyblenkin
Copy link
Collaborator

Hey @grzesiek2010, sorry for the slow response on this one! I agree that it was wrong to display LTR language on the right with RTL layouts. So we should always default to the language direction:

LTR language + RTL layout = displayed on the left
RTL language + LTR layout = displayed on the right

@grzesiek2010
Copy link
Member Author

Thanks @alyblenkin so I'm adding the needs testing label back.

@srujner
Copy link

srujner commented Aug 8, 2023

@grzesiek2010 And what about differences between Android 5.1 and 13 (at the screenshots above)? Which one is the correct behavior?

@grzesiek2010
Copy link
Member Author

13 is the correct one and that is what you should see on most of the Android versions. If there is something wrong with the oldest versions Android 5 or 5 and 6 that's acceptable I would say.

@dbemke
Copy link

dbemke commented Aug 8, 2023

Selects with columns, texts and images look like that with RTL in Collect settings and All widgets form - is it ok? Users might find this case confusing - which picture is in A or B answer.
selectImages

@grzesiek2010
Copy link
Member Author

To me, it's a correct behavior. It's in RTL so elements are displayed from right to left but the labels come from an LTR language so they are on the left (in their containers). I would say that using LTR labels with RTL layout doesn't make sense so that's why it's confusing. I hope our users won't do that.

@dbemke
Copy link

dbemke commented Aug 8, 2023

Tested with Success!

Verified on device with Android 10

Verified cases:

@srujner
Copy link

srujner commented Aug 8, 2023

Tested with Success!

Verified on device with Android 5.1 and 13

@grzesiek2010 grzesiek2010 merged commit b6c65fc into getodk:master Aug 8, 2023
@alyblenkin
Copy link
Collaborator

It's a good point @dbemke, this would be very confusing for users. I think having a two-column grid on mobile also adds to the confusion here. Ideally, we stick to one column, so it's very clear what to select - I've added this to my notes as something to explore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad text alignment for RTL selects
5 participants