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

Fix Android text alignment in migrated projects #16986

Merged
merged 7 commits into from Aug 27, 2023
Merged

Conversation

jknaudt21
Copy link
Contributor

@jknaudt21 jknaudt21 commented Aug 24, 2023

Problem

Android projects that either used an API level below 17 or have android:supportsRtl="false" (or not included) in the app manifest would not properly align text. This particularly impacted migrated projects, since their manifests didn't supportsRtl.

The issue stemmed from the fact that in cases where Rtl was not suppported, we relied on a TextView's Gravity flags. We ended up overwriting the horizontal alignment settings when updating vertical alignment.

Solution

When updating vertical or horizontal alignment settings, the following logic is followed:

  1. Create a mask for the bits pertaining to the setting we want to change.
  2. Get the complement of said mask and do a bitwise AND with the current settings. This ensures that we only clean the Gravity bits for the setting we want to modify
  3. Do a bitwise OR with the new settings and any additional setting we want to pass
Before After
image image

Issues Fixed

Fixes #11251

Tests added

I added a couple of Device tests to SearchBar, Editor, Entry, and Label to test that this works. The only drawback is that these tests will only run if the Android Manifest of the Controls.DeviceTests project is manually edited to have android:supportsRtl="false". This seemed to be the most efficient solution without requiring high amounts of engineering effort considering that this is a fairly niche case that primarily occurs on migrating projects.

Note: These screenshots are displaying slightly outdated display names for the tests. They are all consistent now (similar to the label screenshots)

image image
image image

Work remaining

  • Write test

@jknaudt21 jknaudt21 requested a review from hartez August 24, 2023 19:25
@jknaudt21 jknaudt21 requested a review from a team as a code owner August 24, 2023 19:25
@jknaudt21 jknaudt21 self-assigned this Aug 24, 2023
@mattleibow
Copy link
Member

Do you have a link to an issue to create the manual test for future us?

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Looks great, maybe a small change to reduce some duplicated code and semi-future-proof the tests.

@Eilon Eilon added the area/controls 🎮 Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Aug 25, 2023
Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

Question about the masks.

src/Core/src/Platform/Android/TextAlignmentExtensions.cs Outdated Show resolved Hide resolved
@jknaudt21 jknaudt21 requested a review from hartez August 25, 2023 22:33
hartez
hartez previously approved these changes Aug 25, 2023
@jknaudt21
Copy link
Contributor Author

@mattleibow added the manual test issue: #17014

Comment on lines +15 to +17
{
view.Gravity = (view.Gravity & ~HorizontalGravityMask) | alignment.ToHorizontalGravityFlags() | orMask;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
view.Gravity = (view.Gravity & ~HorizontalGravityMask) | alignment.ToHorizontalGravityFlags() | orMask;
}
view.Gravity = (view.Gravity & ~HorizontalGravityMask) | alignment.ToHorizontalGravityFlags() | orMask;

@rmarinho rmarinho merged commit 8f56077 into main Aug 27, 2023
38 checks passed
@rmarinho rmarinho deleted the dev/jd/androidalign branch August 27, 2023 21:21
rmarinho added a commit that referenced this pull request Aug 29, 2023
* [Android] Fix Keyboard.Numeric issue (#16163)

* Fix Keyboard.Numeric issue on DisplayPromptAsync on Android

* Remove AlertsPage sample

---------

Co-authored-by: Rachel Kang <rachel.j.kang@gmail.com>

* Invalidate shell tab title on Windows (#16593)

Co-authored-by: Rui Marinho <me@ruimarinho.net>

* Fix Flyout Crash (Windows) (#16800)

* Fix flyout crash due to invalid casting of child

* Add tests

* Add additional tests, PR feedback

* Fix missing call to `RemoveLogicalChild`

* Update Clear

---------

Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>

* [core] factory methods for registering services, part 2 (#17004)

b0bba51 was incomplete, in that it missed registrations with 1 type
argument:

    builder.Services.AddScoped<HideSoftInputOnTappedChangedManager>();

So we need to add entries with "`1" for all of the banned methods:

    ++M:Microsoft.Extensions.DependencyInjection.ServiceCollectionServiceExtensions.AddScoped`1(Microsoft.Extensions.DependencyInjection.IServiceCollection);Use a Factory method to register the service instead
    M:Microsoft.Extensions.DependencyInjection.ServiceCollectionServiceExtensions.AddScoped`2(Microsoft.Extensions.DependencyInjection.IServiceCollection);Use a Factory method to register the service instead

We can improve startup time by using a factory method instead:

    builder.Services.AddScoped(_ => new HideSoftInputOnTappedChangedManager());

This also found a couple more calls to fix throughout .NET MAUI.

* [create-pull-request] automated change (#17017)

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Ignore failures from newly added UITests temporarily (#17003)

* Ignore failures from WhenQueryingCarouselItemsInViewThenSingleItemIsRetrieved

* Update Ignore to include all platformss

* Center window on WinUI, so it's always fully on screen in CI

* Set agent screen resolution bigger for UI tests

* Update baseline snapshots for bigger screen

* Just set screen resolution on Windows

* Ignore Issue16320 on Android

* Fix Android text alignment in migrated projects (#16986)

* Fix Android text alignment in migrated projects

* Add (manual) device tests

* Add clarifying comment

* [tentative] Move input tests to generic code

* Wrap up tests

* Make masks constants

* Fix title to be consistent with other test names

* Fix the order of logical modifications (#17020)

* Fix the order of logical modifications

* - keep in sync while removing

* - fix clear

---------

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Co-authored-by: Rachel Kang <rachel.j.kang@gmail.com>
Co-authored-by: Mike Corsaro <corsarom@gmail.com>
Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Bret Johnson <bret.johnson@microsoft.com>
Co-authored-by: Juan Diego Herrera <juherrera@microsoft.com>
Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
@primozcerar
Copy link

primozcerar commented Oct 20, 2023

This needs to be ported to net7. Currently you can't set both HorizontalTextAlignment and VerticalTextAlignment on a Label. Only the last one set is respected and the first one is then ignored, so for example Center, Center is not possible and you have to add another layout around the Label to achieve that. This is a core functionality that just wasted an hour of my time and it the bug is a regression that was introduced in the last half year or so since it only appeared after I updated Visual Studio and with it the MAUI version.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/controls 🎮 Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor platform/android 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text alignment not respected in migrated project
7 participants