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: Match TextBox.VerticalContentAlignment with Windows #13810

Merged
merged 3 commits into from
Apr 18, 2024

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Sep 25, 2023

GitHub Issue (If applicable): closes #4502, closes #14705.

This is a revert of #4180

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Copilot Summary

🤖 Generated by Copilot at 5dd22d4

This pull request fixes a layout bug in TextBox controls and adds a unit test to verify the fix. It also improves the XAML loading logic by using .NET 7.0 source generators and a helper method LoadXaml.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@Youssef1313
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Youssef1313 Youssef1313 marked this pull request as draft September 26, 2023 16:10
@Youssef1313
Copy link
Member Author

There is a problem on Skia where the native Gtk.Entry (shown only on focus), has the text centered vertically, and Gtk.Entry provides no way to change this behavior.

So:

  • Existing behavior on Skia: On both focused & unfocused state, the text is centered vertically.
  • New behavior on Skia: On focus, it's centered vertically. On unfocus, the text is top-aligned.
  • Windows behavior: Text is top-aligned.

So while this PR gets closer to Windows behavior on Skia, I think it could be considered worse because of the inconsistency between focused and unfocused states.

@Youssef1313
Copy link
Member Author

I guess @ramezgerges work will probably help unblock this. Let's see :)

@MartinZikmund
Copy link
Member

@Youssef1313 I think the new behavior is acceptable - it will work properly with rendered TextBox and it works well with WPF

@Youssef1313
Copy link
Member Author

@MartinZikmund Yup it should work well with the new TextBox implementation, but not the old one which is still the "default":

public static bool UseOverlayOnSkia { get; set; } = true;

@MartinZikmund
Copy link
Member

@Youssef1313 but that is the default only for now and will "change" soon, and WPF works fine so I don't see this as a big issue

@MartinZikmund
Copy link
Member

@Youssef1313 I think this can be revived now after 5.2

@Youssef1313
Copy link
Member Author

Yup. Sorry forgot about it earlier

@Youssef1313 Youssef1313 force-pushed the issues/4502 branch 3 times, most recently from 3bdf399 to 826c6e9 Compare April 7, 2024 10:54
@Youssef1313 Youssef1313 marked this pull request as ready for review April 7, 2024 10:57
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-13810/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-13810/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-13810/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-13810/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-13810/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-13810/index.html

@MartinZikmund
Copy link
Member

Ready to be merged!

@Youssef1313 Youssef1313 enabled auto-merge April 18, 2024 15:22
@Youssef1313 Youssef1313 merged commit a3c179e into unoplatform:master Apr 18, 2024
103 checks passed
@Youssef1313 Youssef1313 deleted the issues/4502 branch April 18, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants