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

[iOS] Register effects for ScrollView and WebView #641

Merged
merged 5 commits into from Jan 9, 2017

Conversation

Projects
None yet
7 participants
@adrianknight89
Contributor

adrianknight89 commented Dec 13, 2016

Description of Change

On iOS, ScrollView and WebView can't attach effects. This seems to be due to the fact that SetElement from IVisualElementRenderer is implemented and it does not register effects.

Note that any renderer that does not derive from ViewRenderer has another problem which is that ViewRenderer's OnRegisterEffect is not called and Control is not set. Currently, page renderers implement IVisualElementRenderer and they only set Container to View. Was this by design (i.e. they do not have Control)?

ScrollViewRenderer and WebViewRenderer are the only control renderers that implement the same interface. For these, I'm not exactly sure what to set Container and Control. Right now, they are set to this and NativeView respectively.

I looked at Android. It also has the same issues with several renderers, but Android confuses me even more when it comes to figuring out the values for Container and Control, so this is something you should look into.

Bugs Fixed

API Changes

Changed:

  • IEffectControlProvider is part of IVisualElementRenderer
  • Moved EffectUtilities into Core.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
Show outdated Hide outdated Xamarin.Forms.Core/EffectUtilities.cs
@@ -0,0 +1,16 @@
namespace Xamarin.Forms
{
internal static class EffectUtilities

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 13, 2016

Contributor

This can be moved to Core. Android applies the same functionality.

@adrianknight89

adrianknight89 Dec 13, 2016

Contributor

This can be moved to Core. Android applies the same functionality.

This comment has been minimized.

@rmarinho

rmarinho Dec 19, 2016

Member

We have a internals namespace, maybe move it there.

@rmarinho

rmarinho Dec 19, 2016

Member

We have a internals namespace, maybe move it there.

@@ -73,6 +73,16 @@ protected bool AutoTrack
}
}
public static void RegisterEffect(Effect effect, UIView container, UIView control = null)

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 13, 2016

Contributor

Since RegisterEffect is the same across all renderers, this can be refactored into its own method.

@adrianknight89

adrianknight89 Dec 13, 2016

Contributor

Since RegisterEffect is the same across all renderers, this can be refactored into its own method.

Show outdated Hide outdated Xamarin.Forms.Platform.iOS/IVisualElementRenderer.cs
@@ -3,7 +3,7 @@
namespace Xamarin.Forms.Platform.iOS
{
public interface IVisualElementRenderer : IDisposable, IRegisterable
public interface IVisualElementRenderer : IDisposable, IRegisterable, IEffectControlProvider

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 13, 2016

Contributor

Should any VisualElement have the ability to attach effects? I thought this would be okay. If not, this could be moved back.

@adrianknight89

adrianknight89 Dec 13, 2016

Contributor

Should any VisualElement have the ability to attach effects? I thought this would be okay. If not, this could be moved back.

This comment has been minimized.

@rmarinho

rmarinho Dec 19, 2016

Member

I think that will break existing renderers from other people ..

@rmarinho

rmarinho Dec 19, 2016

Member

I think that will break existing renderers from other people ..

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 19, 2016

Contributor

Can you be a bit more specific as to how it will break existing renderers?

@adrianknight89

adrianknight89 Dec 19, 2016

Contributor

Can you be a bit more specific as to how it will break existing renderers?

This comment has been minimized.

@jassmith

jassmith Dec 20, 2016

Member

because if you made a renderer using the IVisualElementRenderer directly, your code would no longer compile with this change.

@jassmith

jassmith Dec 20, 2016

Member

because if you made a renderer using the IVisualElementRenderer directly, your code would no longer compile with this change.

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 20, 2016

Contributor

Removed.

@adrianknight89

adrianknight89 Dec 20, 2016

Contributor

Removed.

@LRP-sgravel

This comment has been minimized.

Show comment
Hide comment
@LRP-sgravel

LRP-sgravel Dec 22, 2016

Thanks for the initiative, just hit this bug. Hopefully it will be pulled in soon.

LRP-sgravel commented Dec 22, 2016

Thanks for the initiative, just hit this bug. Hopefully it will be pulled in soon.

@hartez hartez removed the waiting-tests label Dec 30, 2016

@rmarinho rmarinho merged commit a041730 into xamarin:master Jan 9, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 350, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3685, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 34…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 344…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 346…
Details

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment