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

Fixes #22914 #22917

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Fixes #22914 #22917

wants to merge 13 commits into from

Conversation

Jon2G
Copy link
Contributor

@Jon2G Jon2G commented Jun 7, 2024

Description of Change

Expecting a specific layout type on ViewExtensions for Android and iOS makes impossible to set backgroundColor to null once it has been set

Issues Fixed

Fixes #

Validate if platformView is not null instead of expecting it to ALWAYS be LayoutViewGroup

@Jon2G Jon2G requested a review from a team as a code owner June 7, 2024 18:32
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jun 7, 2024
@Jon2G
Copy link
Contributor Author

Jon2G commented Jun 7, 2024

@dotnet-policy-service agree

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.

Thanks for this PR! This makes a lot of sense and looks good.

Are you able to add a UI test to confirm this works as expected?

This PR has a good example of what a UI test is: https://github.com/dotnet/maui/pull/21250/files It has 2 new tests added which consist of 32 parts:

  • the page for the issue in the test app
  • the test for appium

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Jon2G
Copy link
Contributor Author

Jon2G commented Jun 7, 2024

@mattleibow Working on it!

@PureWeen PureWeen added the area-controls-general General issues that span multiple controls, or common base classes such as View or Element label Jun 8, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Jun 9, 2024
@jsuarezruiz
Copy link
Contributor

Launching the build again, just applied some small changes in the test adding a couple of pending namespaces and changing the test name.

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

foreach (var elementId in _expectedNullBackgroundColorIds)
{
var element = App.WaitForElement(elementId, $"Timed out waiting for {elementId}");
var elementBackgroundColor = element.GetAttribute<Color?>("BackgroundColor");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not implemented in appium by all the supported .NET MAUI platforms:

System.NotImplementedException : 'BackgroundColor' attribute is unknown for the element. Only the following attributes are supported: [checkable, checked, {class,className}, clickable, {content-desc,contentDescription}, enabled, focusable, focused, {long-clickable,longClickable}, package, password, {resource-id,resourceId}, scrollable, selection-start, selection-end, selected, {text,name}, hint, extras, bounds, displayed, contentSize]

As an alternative, you can use the VerifyScreenshot method. Will create a reference snapshot and the test will compare a new one created every time the test run with the reference one. In that way, can check the BackgroundColor. Other option, is create a Device Tests. Can find some samples accessing the platform control background color here https://github.com/dotnet/maui/blob/main/src/Controls/tests/DeviceTests/Elements/BoxView/BoxViewTests.cs#L29

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its safer to use VerifyScreenshot since we need to assert that property changes visually.
How do you take the screenshots for the test, in order to place them in snapshots folders.

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

var tapAreaResult = App.WaitForElement(ButtonId, $"Timed out waiting for {ButtonId}");
tapAreaResult.Tap();

VerifyScreenshot("Issue22914Test.png");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not necessary to set the extension.

Text="Sample label"
x:Name="Label1"/>

<Button AutomationId="Tap1Button"
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not necessary but it would be nice to have text on the Button.

</ContentView>

<Label HeightRequest="100"
Background="Green"
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, should use BackgroundColor. If not, after tapping the Button and setting it to null, will not impact to the Background.
Issue22914Test png

@Redth
Copy link
Member

Redth commented Jun 21, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/rebase

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-general General issues that span multiple controls, or common base classes such as View or Element community ✨ Community Contribution
Projects
None yet
6 participants