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

Triggering a change with the ContainerView results in incorrect things #12116

Closed
mattleibow opened this issue Dec 14, 2022 · 2 comments · Fixed by #12259
Closed

Triggering a change with the ContainerView results in incorrect things #12116

mattleibow opened this issue Dec 14, 2022 · 2 comments · Fixed by #12259
Assignees
Labels
area-gestures Gesture types fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Milestone

Comments

@mattleibow
Copy link
Member

mattleibow commented Dec 14, 2022

Description

Some aspects of the view, such as Gestures on Android (and maybe iOS if this is merged: #12108), are attached to the container view. As a result, when the container view is removed the whole gesture set is missing.

Steps to Reproduce

  1. File | New | Maui app (I lie, I used the sandbox app)
  2. Paste this code:
    var stack = new VerticalStackLayout();
    
    var view1 = new Label
    {
    	Text = "Started with NO container",
    	// no shadow
    	GestureRecognizers = { new TapGestureRecognizer { Command = new Command(() => Debug.WriteLine("view 1")) } }
    };
    stack.Add(view1);
    
    var view2 = new Label
    {
    	Text = "Started WITH a container",
    	Shadow = new Shadow(),
    	GestureRecognizers = { new TapGestureRecognizer { Command = new Command(() => Debug.WriteLine("view 2")) } }
    };
    stack.Add(view2);
    
    var view3 = new Image
    {
    	Source = ImageSource.FromFile("dotnet_bot.png"),
    	WidthRequest = 80,
    	HeightRequest = 80,
    	// no background
    	GestureRecognizers = { new TapGestureRecognizer { Command = new Command(() => Debug.WriteLine("view 3")) } }
    };
    stack.Add(view3);
    
    var view4 = new Image
    {
    	Source = ImageSource.FromFile("dotnet_bot.png"),
    	WidthRequest = 80,
    	HeightRequest = 80,
    	Background = new SolidColorBrush(Colors.Red),
    	GestureRecognizers = { new TapGestureRecognizer { Command = new Command(() => Debug.WriteLine("view 4")) } }
    };
    stack.Add(view4);
    
    var btn = new Button
    {
    	BackgroundColor = Colors.Orange,
    	Text = "OK",
    	Command = new Command(() =>
    	{
    		view1.Shadow = new Shadow(); // causes a container view to be added
    		view2.Shadow = null; // causes the container view to be removed
    		view3.Background = new SolidColorBrush(Colors.Red); // causes a container view to be added
    		view4.Background = null; // causes the container view to be removed
    	})
    };
    stack.Add(btn);
    
    Content = stack;
  3. Tap the labels and observe both log to the console
  4. Tap the button
  5. Observe that only the first label logs

NOTE: there is a bug on iOS where setting the background to null does not actually remove the background... (#12117) so for now replace with Shadow and you get the same issue.

This is because in the first case, a container was added and the gestures are still attached to the label. In the second case, the gestures were attached to the container that is now removed from the view hierarchy.

Even if this was perfect, I am not sure how this plays with controls that respond to taps - for example buttons. Will attaching gestures stop normal button things? If so, will it change when it is on the container? Is this a valid case of a button and a tap gesture on that button? Should adding a gesture trigger a new container?

In a perfect world, we have 2 behaviours - with and without containers!

Link to public reproduction project repository

:)

Version with bug

7.0 (current)

Last version that worked well

Unknown/Other

Affected platforms

iOS, Android, Windows, macOS, Other (Tizen, Linux, etc. not supported by Microsoft directly)

Affected platform versions

All

Did you find any workaround?

No response

Relevant log output

No response

@mattleibow
Copy link
Member Author

This is probably also causing memory leaks. The gesture manager holds on to the container views even after removed.

@jsuarezruiz jsuarezruiz added area-gestures Gesture types legacy-area-perf Startup / Runtime performance labels Dec 15, 2022
@jsuarezruiz jsuarezruiz added this to the Backlog milestone Dec 19, 2022
@ghost
Copy link

ghost commented Dec 19, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@PureWeen PureWeen self-assigned this Jan 11, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! label Feb 22, 2023
@samhouts samhouts modified the milestones: Backlog, 8.0-preview1 Feb 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2023
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-gestures Gesture types fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants