[Core] Propagate IsEnabled property from Layouts to children by jsuarezruiz · Pull Request #6892 · dotnet/maui

jsuarezruiz · 2022-05-06T11:41:04Z

Description of Change

Propagate IsEnabled property from Layouts to childrens.

Issues Fixed

Fixes #4755
Fixes #5287

Redth

@jsuarezruiz There are some failing tests on input transparency as a result of this I think.

mattleibow · 2022-05-07T10:24:39Z

Just had a nasty thought... What happens if we have a layout that we disable, and then manually enable the child in it?

Will it undo the enabled?

Or, if I have a nested grid and disable the outer one and then enable the inner one?

mattleibow

Since this PR makes changes to child/parent views, we also need to make sure we test these cases:

nested layouts
disabling a parent and enabling the child
disabling the child and enabling the parent
disabling a layout and then adding a child
any other properties that also enable/disable controls that could potentially conflict with this property
since this affects the children, we need tests on moving a child from one layout to another

jsuarezruiz · 2022-05-09T12:21:48Z

Added "do-not-merge" label, adding tests.

jsuarezruiz · 2022-05-10T10:39:46Z

Added new sample:

mattleibow · 2022-05-10T12:47:44Z

+			{
+				platformView.IsHitTestVisible = view.IsEnabled && !view.InputTransparent;
+
+				(view as ILayout)?.InvalidateChildrenIsEnabled();


Should this be inside the else block? What happens if Control is also ILayout? If not, then it is a no-op, but if it is...

Redth

I fixed the initialization case in android, and fixed the sample to have the "all children disabled" layout actually specify IsEnabled="False"

I tested this on Windows, Android, and MacCatalyst and am now comfortable this works as expected.

jsuarezruiz · 2022-05-11T14:39:56Z

Added Device tests.

Redth

This one needs some more thought after some conversation with @mattleibow and failing tests...

jsuarezruiz · 2022-05-13T08:28:13Z

/azp run

azure-pipelines · 2022-05-13T08:28:35Z

Azure Pipelines successfully started running 2 pipeline(s).

hartez · 2022-07-19T20:40:14Z

+	{
+		public static void InvalidateChildrenIsEnabled(this ILayout layout)
+		{
+			foreach (var children in layout.OrderByZIndex())


Even if this were the correct place to do it, there's no reason to enable/disable the children in z-index order.

hartez

What happens if we have a layout that we disable, and then manually enable the child in it?

Core is the wrong place to implement these rules. Core should handle mapping IsEnabled -> platformView.IsEnabled (or whatever the platform equivalent is) and nothing else. Any sort of propagation is the business of the implementing SDK.

At the Controls level, we should implement whatever propagation we had in Forms (for consistency). IIRC, in Forms a Layout with IsEnabled == false meant that all child controls are also effectively disabled. Any attempt to "enable" a child of disabled Layout does nothing. We should verify that, and we would need to implement that at the Controls level.

Other SDKs may want more permissive enable/disable rules (like bulk disabling a Layout and then enabling a single child) - and they'd be free to handle it that way.

jsuarezruiz · 2022-12-14T13:07:58Z

Changed target to main. Tested and passed the device tests in the different platforms.

hartez

None of the propagation should be happening in Core; that should be left to the implementing SDK. And Controls should be implementing this in a way that matches Forms, for backward compatibility.

rmarinho · 2023-01-02T12:08:07Z

Not sure if i agree, that the propagation should not be on the Core. What's the reason for that? that's not a sdk rule, that should be a layout rule, if a layout is disable the children are disable, it's a general rule for all sdks. not a form specific.

hartez · 2023-01-03T17:49:06Z

Not sure if i agree, that the propagation should not be on the Core. What's the reason for that? that's not a sdk rule, that should be a layout rule, if a layout is disable the children are disable, it's a general rule for all sdks. not a form specific.

Because reasonable people can disagree on what the rules should be. For example, look at the questions in Matt's comment - there's no obviously right answer to these.

Choosing a set of answers and making Core opionated about how enabling/disabling containers should work adds complexity to Core and impacts performance. And it forces every other SDK implementation to either use those rules or spend a lot of effort working around them.

Core should be providing simple, basic infrastructure that's not especially opinionated. And the simplest possible mapping of IsEnabled to the containers is to do nothing.

This gives us the flexibility to answer all of those questions in Controls in whatever way we choose, whether it's by replicating the weirdness of Forms for backward compatibility, or doing something simpler and less confusing.

hartez · 2023-01-07T02:59:04Z

Okay, after thinking about this some more, I think it's fine for us to put this in Core with some modifications. If we don't force any disabling in the backing layouts themselves, this is easy enough for 3rd party implementations to change if they want to.

hartez

On further reflection, I think this will be fine. We just need to make some adjustments.

hartez · 2023-01-07T03:00:30Z

+	{
+		internal static void InvalidateChildrenIsEnabled(this ILayout layout)
+		{
+			foreach (var children in layout.GetChildren())


layout is already an IList<IView>, so we don't need the GetChildren() method here. And this should be a for loop rather than a foreach, for performance reasons.

hartez · 2023-01-07T03:01:22Z

 {
 	internal static class LayoutExtensions
 	{
+		public static IList<IView> GetChildren(this ILayout layout) => layout;


This method isn't necessary.

hartez · 2023-01-07T03:05:21Z

+			if (platformView is UIControl uiControl)
+				uiControl.Enabled = view.GetIsEnabled();
+
+			if (platformView is LayoutView layout)


We don't need to enable/disable the layout backing controls; the child controls will all be disabled anyway. We can just drop lines 26 & 27,

hartez · 2023-01-07T03:06:14Z

 			return result;
 		}

+		internal bool InternalUserInteractionEnabled


We don't need to disable user interaction, so this property can just go away.

hartez · 2023-01-07T03:08:13Z

+			if (platformView is Control control)
+				control.UpdateIsEnabled(view);
+			else
+				platformView.IsHitTestVisible = view.GetIsEnabled() && !view.InputTransparent;


We don't need to block input to the layout backing control here; lines 34 & 35 can go.

hartez · 2023-01-07T03:09:29Z

+		public static void UpdateIsEnabled(this FrameworkElement platformView, IView view)
+		{
+			if (platformView is Control control)
+				control.UpdateIsEnabled(view);


We can probably change this to control.IsEnabled = view.GetIsEnabled() and avoid creating another extension method.

hartez · 2023-01-07T03:10:04Z

 			platformControl.IsTextScaleFactorEnabled = font.AutoScalingEnabled;
 		}

+		public static void UpdateIsEnabled(this Control platformControl, IView view) =>


I think the only place using this method is ViewExtensions.cs below, we can probably just inline this and avoid adding more API.

mattleibow · 2023-01-12T17:46:00Z

We are probably going to use v10 :) #12488

jsuarezruiz added t/bug Something isn't working area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter labels May 6, 2022

Redth approved these changes May 6, 2022

View reviewed changes

Redth requested changes May 6, 2022

View reviewed changes

mattleibow requested changes May 7, 2022

View reviewed changes

jsuarezruiz added the do-not-merge Don't merge this PR label May 9, 2022

Redth added this to the 6.0.300 milestone May 9, 2022

Redth assigned mattleibow May 9, 2022

mattleibow reviewed May 10, 2022

View reviewed changes

Redth approved these changes May 10, 2022

View reviewed changes

mattleibow approved these changes May 11, 2022

View reviewed changes

mattleibow enabled auto-merge (squash) May 11, 2022 01:57

mattleibow disabled auto-merge May 11, 2022 01:57

mattleibow requested changes May 11, 2022

View reviewed changes

Comment thread src/Core/src/Core/IView.cs Outdated

mattleibow approved these changes May 11, 2022

View reviewed changes

rmarinho removed the do-not-merge Don't merge this PR label May 11, 2022

Redth requested changes May 11, 2022

View reviewed changes

Redth marked this pull request as draft May 11, 2022 20:52

Redth modified the milestones: 6.0.300, 6.0.300-servicing May 11, 2022

This was referenced Jun 15, 2022

IsVisible doesn't work anymore #8044

Closed

Button is clickable even if the IsEnable property of the container control is false. #8169

Closed

mattleibow changed the base branch from main to net6.0 July 19, 2022 00:11

hartez reviewed Jul 19, 2022

View reviewed changes

hartez suggested changes Jul 19, 2022

View reviewed changes

hartez changed the title ~~[Core] Propagate IsEnabled property from Layouts to childrens~~ [Core] Propagate IsEnabled property from Layouts to children Jul 19, 2022

andyjohnson0 mentioned this pull request Jul 23, 2022

Main gui window not busied while extraction is running andyjohnson0/TakeoutExtractor#1

Closed

Redth modified the milestones: 6.0-servicing, .NET 7 Planning Aug 30, 2022

jsuarezruiz changed the base branch from net6.0 to main December 14, 2022 09:07

jsuarezruiz changed the base branch from main to net6.0 December 14, 2022 09:08

Propagate IsEnabled property from Layouts to children

2209f34

jsuarezruiz force-pushed the fix-4755 branch from a7d5c1f to 2209f34 Compare December 14, 2022 13:04

jsuarezruiz changed the base branch from net6.0 to main December 14, 2022 13:04

jsuarezruiz marked this pull request as ready for review December 14, 2022 13:04

jsuarezruiz added 2 commits December 14, 2022 17:12

Merge branch 'main' into fix-4755

5940e0a

Fix build errors

672ef96

hartez suggested changes Dec 14, 2022

View reviewed changes

Comment thread src/Core/src/Extensions/IViewExtensions.cs Outdated

Updated GetIsEnabled method based on PR feedback

bf14582

hartez mentioned this pull request Dec 30, 2022

Toggle IsEnabled on Layout children when IsEnabled changes #12365

Closed

hartez suggested changes Jan 7, 2023

View reviewed changes

mattleibow mentioned this pull request Jan 7, 2023

Propagate VisualElement.IsEnabled to children #12488

Merged

3 tasks

mattleibow closed this Jan 12, 2023

mattleibow deleted the fix-4755 branch January 12, 2023 17:46

github-actions Bot locked and limited conversation to collaborators Dec 22, 2023

Uh oh!

Conversation

jsuarezruiz commented May 6, 2022

Description of Change

Issues Fixed

Uh oh!

Redth left a comment

Choose a reason for hiding this comment

Uh oh!

mattleibow commented May 7, 2022

Uh oh!

mattleibow left a comment

Choose a reason for hiding this comment

Uh oh!

jsuarezruiz commented May 9, 2022

Uh oh!

jsuarezruiz commented May 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Redth left a comment

Choose a reason for hiding this comment

Uh oh!

jsuarezruiz commented May 11, 2022

Uh oh!

Uh oh!

Redth left a comment

Choose a reason for hiding this comment

Uh oh!

jsuarezruiz commented May 13, 2022

Uh oh!

azure-pipelines Bot commented May 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hartez left a comment

Choose a reason for hiding this comment

Uh oh!

jsuarezruiz commented Dec 14, 2022

Uh oh!

hartez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rmarinho commented Jan 2, 2023

Uh oh!

hartez commented Jan 3, 2023

Uh oh!

hartez commented Jan 7, 2023

Uh oh!

hartez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattleibow commented Jan 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants