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 FrameworkElement.[Measure|Arrange]Override with Windows #13839

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Sep 27, 2023

GitHub Issue (If applicable): closes #1554

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 2b75eac

This pull request refactors the layout logic for FrameworkElement and its subclasses by introducing helper methods to measure and arrange the first child element, and by returning the default size in the base FrameworkElement methods. This simplifies the code and improves the consistency with the WinUI behavior. The pull request also adds the necessary assembly attributes and using directives to support the media player implementations.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@github-actions github-actions bot added platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform platform/macos 🍏 Categorizes an issue or PR as relevant to the macOS platform platform/ios 🍎 Categorizes an issue or PR as relevant to the iOS platform labels Sep 27, 2023
@Youssef1313 Youssef1313 force-pushed the measure-override branch 2 times, most recently from c528346 to ab9dcb8 Compare September 27, 2023 10:09
@Youssef1313
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@github-actions github-actions bot added the area/skia ✏️ Categorizes an issue or PR as relevant to Skia label Sep 28, 2023
@Youssef1313 Youssef1313 marked this pull request as ready for review September 28, 2023 12:07
@Youssef1313
Copy link
Member Author

@jeromelaban Is this good to merge?

@jeromelaban
Copy link
Member

jeromelaban commented Oct 12, 2023

I don't know yet if it is, this is a significant change that needs screenshot validations. Did you check those as well? There are also impacts on native controls, which may not be tested automatically.

@Youssef1313
Copy link
Member Author

Yup I checked screenshot comparison a while ago when I have opened the PR. Nothing was looking interesting there.

Note that a behavior change here can only happen for classes that inherit from FrameworkElement but not from Control. That is why I added few more MeasureOverride and ArrangeOverride to maintain the existing behavior for those classes, and for other cases avoiding base.[MeasureOverride|ArrangeOverride] calls where base is referring to FrameworkElement.

@jeromelaban
Copy link
Member

Ok, let's merge as-is, though we'll need to validate the behavior a bit more with the deployed samples app.

@jeromelaban jeromelaban merged commit 26b9e38 into unoplatform:master Oct 12, 2023
85 checks passed
@Youssef1313 Youssef1313 deleted the measure-override branch October 12, 2023 13:19
@Youssef1313
Copy link
Member Author

Yup sure!
I'll keep an eye on new issues and will investigate if something felt related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/skia ✏️ Categorizes an issue or PR as relevant to Skia platform/ios 🍎 Categorizes an issue or PR as relevant to the iOS platform platform/macos 🍏 Categorizes an issue or PR as relevant to the macOS platform platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FrameworkElement default Measure/Arrange override imeplementation
2 participants