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.

Sign up for GitHub

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

Jump to bottom

Fix Frame offsets inner content view by 1pt #24582

Merged
merged 29 commits into from
Sep 25, 2024
Merged

Fix Frame offsets inner content view by 1pt #24582

merged 29 commits into from
Sep 25, 2024

Conversation

devanathan-vaithiyanathan
Copy link
Contributor

@devanathan-vaithiyanathan devanathan-vaithiyanathan commented Sep 3, 2024 •
edited
Loading

Root Cause

Frame layout calculations included the border width even when no border color was set, causing incorrect frames and layout inconsistencies.

Description of Change

To fix this issue, a condition was added to check if the BorderColor is not null before applying the border width. This ensures that the bounds are only adjusted when a border color is actually set.

Issues Fixed

Fixes #23333

Validated the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Output Screenshot

Before After

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Sep 3, 2024
@mattleibow
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@devanathan-vaithiyanathan devanathan-vaithiyanathan marked this pull request as ready for review September 5, 2024 06:43
@PureWeen
Copy link
Member

PureWeen commented Sep 5, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow mattleibow added the s/pr-needs-author-input PR needs an update from the author label Sep 9, 2024
mattleibow
mattleibow previously requested changes Sep 9, 2024
View reviewed changes
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.

The before and after look very much the same to me, so maybe I am missing something.

Also, the images from CI have the tab area still:

iOS Android
image image

@devanathan-vaithiyanathan
Copy link
Contributor Author

devanathan-vaithiyanathan commented Sep 10, 2024 •
edited
Loading

Hi @mattleibow ,
The border width value is 1, so we don't see much difference. We verified the changes by adjusting the border width in our source. Please check the BorderWidth value set to 10 in the images below,
demo

@PureWeen
Copy link
Member

PureWeen commented Sep 10, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Sep 17, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen requested changes Sep 24, 2024
View reviewed changes
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

The iOS failure on Issue17366Test feels like a valid failure

https://dev.azure.com/xamarin/public/_build/results?buildId=123856&view=ms.vss-test-web.build-test-results-tab&runId=3157597&resultId=100030&paneView=debug

Might need to regenerate the screen shot for that test now that frame has changed or see if this is a bug caused by this pr

@devanathan-vaithiyanathan
Copy link
Contributor Author

Hi @PureWeen ,
I have added the newly generated screen shot for failing test case. Could you please check once?

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen merged commit 3ef2d92 into dotnet:main Sep 25, 2024
97 checks passed
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 1, 2024
@samhouts samhouts added fixed-in-9.0.0-rc.2.24503.2 and removed fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Oct 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PureWeen PureWeen PureWeen approved these changes

@jsuarezruiz jsuarezruiz Awaiting requested review from jsuarezruiz jsuarezruiz is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@mattleibow mattleibow Awaiting requested review from mattleibow

Assignees
No one assigned
Labels
area-controls-frame Frame community ✨ Community Contribution fixed-in-9.0.0-rc.2.24503.2 s/pr-needs-author-input PR needs an update from the author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Frame offsets inner content view by 1pt
4 participants
@devanathan-vaithiyanathan @mattleibow @PureWeen @samhouts