Fix #3934, #3958, and #3919: [RTL] High-fi Align TextViews, description text and toolbar marquee text.

[veena14cs]

Explanation

This PR fixes #3934, #3958 and #3919 . Topic description and Continue description is right aligned. Marquee text for toolbar title is RTL compatible.

Screenshot LTR and RTL

Mobile-

...........

....

...

.....

....

.....

...

....
....
...
......

Tablet

....

....

....

.....

Marquee for Toolbar title LTR and RTL

new-ltr.mp4

new-rtl.mp4

Essential Checklist

• The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
• Any changes to scripts/assets files have their rationale included in the PR explanation.
• The PR follows the style guide.
• The PR does not contain any unnecessary code changes from Android Studio (reference).
• The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
• The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

• Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
• For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
• Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
• Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

[BenHenning]

Thanks @veena14cs! Just had a few comments, otherwise the fix looks excellent.

I think the main remaining point is the one brought up below regarding whether we should be ensuring all TextViews in the app have styles set up. I'll work with Rajat to make a decision on this quickly so that we can get the rest of this PR wrapped up.

Thanks @veena14cs. This principally is looking excellent.
@rt4914 do you have any thoughts on to what extent we should be using styles for the non-body text views in this PR?

@BenHenning Unable to understand what exactly you mean here.

Sorry, some context was probably missing since @veena14cs & I have been chatted about this outside the PR. @rt4914 I'm wondering whether we should be ensuring that all TextViews have a predetermined style (like Body) so that we can centrally manage things like RTL for most scenarios just by ensuring that TextViews always have a style that has the correct LTR/RTL setup. We could even use a CI check in the future to enforce that TextViews properly use a predefined style (which is outside thes cope of this PR, but ensuring that all TextViews are styled seems within scope here). What are your thoughts?

[rt4914]

Thanks @veena14cs! Just had a few comments, otherwise the fix looks excellent.

I think the main remaining point is the one brought up below regarding whether we should be ensuring all TextViews in the app have styles set up. I'll work with Rajat to make a decision on this quickly so that we can get the rest of this PR wrapped up.

Thanks @veena14cs. This principally is looking excellent.
@rt4914 do you have any thoughts on to what extent we should be using styles for the non-body text views in this PR?

@BenHenning Unable to understand what exactly you mean here.

Sorry, some context was probably missing since @veena14cs & I have been chatted about this outside the PR. @rt4914 I'm wondering whether we should be ensuring that all TextViews have a predetermined style (like Body) so that we can centrally manage things like RTL for most scenarios just by ensuring that TextViews always have a style that has the correct LTR/RTL setup. We could even use a CI check in the future to enforce that TextViews properly use a predefined style (which is outside thes cope of this PR, but ensuring that all TextViews are styled seems within scope here). What are your thoughts?

@BenHenning yes that makes sense and I completely agree with this as it would make things a lot more easier from RTL prospective.

[veena14cs]

Thanks @veena14cs! Just had a few comments, otherwise the fix looks excellent.
I think the main remaining point is the one brought up below regarding whether we should be ensuring all TextViews in the app have styles set up. I'll work with Rajat to make a decision on this quickly so that we can get the rest of this PR wrapped up.

Thanks @veena14cs. This principally is looking excellent.
@rt4914 do you have any thoughts on to what extent we should be using styles for the non-body text views in this PR?

@BenHenning Unable to understand what exactly you mean here.

Sorry, some context was probably missing since @veena14cs & I have been chatted about this outside the PR. @rt4914 I'm wondering whether we should be ensuring that all TextViews have a predetermined style (like Body) so that we can centrally manage things like RTL for most scenarios just by ensuring that TextViews always have a style that has the correct LTR/RTL setup. We could even use a CI check in the future to enforce that TextViews properly use a predefined style (which is outside thes cope of this PR, but ensuring that all TextViews are styled seems within scope here). What are your thoughts?

@BenHenning yes that makes sense and I completely agree with this as it would make things a lot more easier from RTL prospective.

@BenHenning @rt4914 can we have a new issue and PR tracking this. As this PR is growing and if the correction on textviews looks good here lets make it merge ready and have another PR to ensure all textviews have style attribute.

[rt4914]

Thanks @veena14cs! Just had a few comments, otherwise the fix looks excellent.
I think the main remaining point is the one brought up below regarding whether we should be ensuring all TextViews in the app have styles set up. I'll work with Rajat to make a decision on this quickly so that we can get the rest of this PR wrapped up.

Thanks @veena14cs. This principally is looking excellent.
@rt4914 do you have any thoughts on to what extent we should be using styles for the non-body text views in this PR?

@BenHenning Unable to understand what exactly you mean here.

Sorry, some context was probably missing since @veena14cs & I have been chatted about this outside the PR. @rt4914 I'm wondering whether we should be ensuring that all TextViews have a predetermined style (like Body) so that we can centrally manage things like RTL for most scenarios just by ensuring that TextViews always have a style that has the correct LTR/RTL setup. We could even use a CI check in the future to enforce that TextViews properly use a predefined style (which is outside thes cope of this PR, but ensuring that all TextViews are styled seems within scope here). What are your thoughts?

@BenHenning yes that makes sense and I completely agree with this as it would make things a lot more easier from RTL prospective.

@BenHenning @rt4914 can we have a new issue and PR tracking this. As this PR is growing and if the correction on textviews looks good here lets make it merge ready and have another PR to ensure all textviews have style attribute.

@veena14cs That does sound good to me. We can merge this PR and solve other things in separate PR. Make sure you file an issue for the same. Thanks.

[veena14cs]

@BenHenning @rt4914 can we have a new issue and PR tracking this. As this PR is growing and if the correction on textviews looks good here lets make it merge ready and have another PR to ensure all textviews have style attribute.

@veena14cs That does sound good to me. We can merge this PR and solve other things in separate PR. Make sure you file an issue for the same. Thanks.

Thanks @rt4914. I have filed an issue #3971 tracking this. Once this PR is merged I can find it easy to solve this issue.

[BenHenning]

Thanks @veena14cs. Just have one comment to clarify, and then this PR will LGTM. Please ping me once you get a chance to review it--I'm happy to prioritize getting this PR merged today (my time).

@BenHenning @rt4914 can we have a new issue and PR tracking this. As this PR is growing and if the correction on textviews looks good here lets make it merge ready and have another PR to ensure all textviews have style attribute.

@veena14cs That does sound good to me. We can merge this PR and solve other things in separate PR. Make sure you file an issue for the same. Thanks.

Thanks @rt4914. I have filed an issue #3971 tracking this. Once this PR is merged I can find it easy to solve this issue.

SGTM.

[veena14cs]

Thanks @veena14cs. Just have one comment to clarify, and then this PR will LGTM. Please ping me once you get a chance to review it--I'm happy to prioritize getting this PR merged today (my time).

I have changed constraint to parent instead of on view. The width is set to 0dp that is why it is not aligned in the center and it is always at left for LTR and right for RTL.

[BenHenning]

Thanks @veena14cs. Just have one comment to clarify, and then this PR will LGTM. Please ping me once you get a chance to review it--I'm happy to prioritize getting this PR merged today (my time).

I have changed constraint to parent instead of on view. The width is set to 0dp that is why it is not aligned in the center and it is always at left for LTR and right for RTL.

Thanks @veena14cs

I'm having difficulty understanding this. Per https://stackoverflow.com/a/46692576 and https://stackoverflow.com/a/40262227 it's my understanding that setting start/end constraints to a view's parent (or another view) with 0dp width always centers that view relative to the other one (in the case of the parent, within the parent) since "0dp" forces the constraint's dimensions to be used as the width of the view.

That being said, because it's a TextView I think its text alignment is always start & top by default, so it seems that we don't need to do anything extra here. Can you confirm that we don't need to specify view alignment as start? And, if so, why is that not the case for other text views where we had to explicitly define it as viewStart?

[veena14cs]

Thanks @veena14cs. Just have one comment to clarify, and then this PR will LGTM. Please ping me once you get a chance to review it--I'm happy to prioritize getting this PR merged today (my time).

I have changed constraint to parent instead of on view. The width is set to 0dp that is why it is not aligned in the center and it is always at left for LTR and right for RTL.

Thanks @veena14cs

I'm having difficulty understanding this. Per https://stackoverflow.com/a/46692576 and https://stackoverflow.com/a/40262227 it's my understanding that setting start/end constraints to a view's parent (or another view) with 0dp width always centers that view relative to the other one (in the case of the parent, within the parent) since "0dp" forces the constraint's dimensions to be used as the width of the view.

That being said, because it's a TextView I think its text alignment is always start & top by default, so it seems that we don't need to do anything extra here. Can you confirm that we don't need to specify view alignment as start? And, if so, why is that not the case for other text views where we had to explicitly define it as viewStart?

I think you got confused. Here I am not setting text to start/end but the entire TextView. You can check the screenshot below to fix the UI I have added constraint or I can add constraint bias with 0%.

Old Screenshot LTR and RTL
....

New Screenshot LTR and RTL

....

Earlier it wasn't constrained to parent or any other view and so it was left aligned even in RTL device. To fix this I added constraint.

[BenHenning]

Got it. So the TextView is actually being centered now, and that's correct because the default alignment is viewStart. This would be easier to see if the bounds of the text view were rendered, or the constraints, but I think the screenshots help illustrate this point more clearly. Thanks @veena14cs

[BenHenning]

Per the discussion thread, this LGTM. Thanks @veena14cs!