Create borders on demand #1

rozele · 2017-07-17T17:57:12Z

Creating this pull request to start a dialog.

rozele · 2017-07-17T18:05:14Z

ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs

+                border = new Border { BorderBrush = s_defaultBorderBrush, Tag = 123 };
+                view.Children.Insert(0, border);
+
+                border.SetBinding(FrameworkElement.WidthProperty, new Binding


Not sure what would be more of a hack, but the ViewManager class has the SetDimensions method that you can override to intercept the width/height settings of the view. #Closed

Fixed #Closed

rozele · 2017-07-17T18:07:51Z

ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs

+            return GetBorder(view) != null;
+        }
+
+        private Border GetBorder(TCanvas view)


GetBorder [](start = 23, length = 9)

I can see this being problematic if the actual first child is a border for some custom native component. #Closed

One approach might be to subclass Canvas and maintain the state about whether the first child is a built-in Border there.

In reply to: 127780765 [](ancestors = 127780765)

Fixed #Closed

rozele · 2017-07-17T18:37:42Z

ReactWindows/ReactNative.Shared/UIManager/BorderedViewParentManager.cs

-            view.Background = new SolidColorBrush(ColorHelpers.Parse(color));
+            if (!HasBorder(view) && ColorHelpers.IsTransparent(color))
+                return;
+


I think at some point we relied on transparent borders because without them the views weren't picked up as click targets. However, we've since done a bunch of work with the TouchHandler, so it may no longer be a problem, can you confirm if you have buttons with transparent backgrounds that they can still be targeted with and w/o a border #Closed

Need to check that... #Closed

rozele · 2017-07-17T18:42:59Z

Would be good to clean up styles to follow the rest of the code base, I haven't got around to enabling any kind of linter, but just off the top of my head I saw some protected / private methods that needed to be ordered in the class and use { } on all blocks (even one liners) #Closed

rozele · 2017-07-17T19:04:35Z

You probably need to remove this hack as well - https://github.com/Microsoft/react-native-windows/blob/67cf0d5db79a6665e4243f4e321c274ca1bbba0e/ReactWindows/ReactNative.Shared/UIManager/NativeViewHierarchyOptimizer.cs#L400-L415

In reply to: 315844695 [](ancestors = 315844695)

antonkh · 2017-07-18T02:07:54Z

ReactWindows/ReactNative.Shared/UIManager/BorderedViewParentManager.cs

-            view.BorderBrush = color.HasValue
-                ? new SolidColorBrush(ColorHelpers.Parse(color.Value))
-                : s_defaultBorderBrush;
+            if (HasBorder(view) || color.HasValue && !ColorHelpers.IsTransparent(color.Value))


This optimization needs to be done more carefully. If we first set border color to 0 (a fully transparent black color), this if will ignore the request because the border wouldn't be visible. However if later we set border width to 2, the border will be created with the default border brush, which is an opaque black color, while it should be transparent. #Closed

Fair enough, why not just eliminate the optimization?

In reply to: 127868591 [](ancestors = 127868591)

rozele · 2017-07-18T13:12:29Z

ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs

+            {
+                border.Width = dimensions.Width;
+                border.Height = dimensions.Height;
+            }


Just curious, do you think this is a better approach? XAML binding also worked, I wasn't sure what felt like more of a hack. Do you think there's any performance benefits to either? #Closed

SetDimensions looks simpler. IMO, difference in performance would be hard to measure. #Closed

rozele · 2017-07-18T13:14:43Z

ReactWindows/ReactNative.Shared/UIManager/BorderedViewParentManager.cs

-
-            return (TFrameworkElement)parent.Child;
+            var pointerEvents = EnumHelpers.ParseNullable<PointerEvents>(pointerEventsValue) ?? PointerEvents.Auto;
+            view.SetPointerEvents(pointerEvents);
        }


I'm not sure why these props are here and not on either:
a) ReactViewManager
b) BaseViewManager

I'll have to do some digging. #Closed

Ah, yeah these two props, accessible and pointerEvents should be on ReactViewManager (they currently are in the master branch of react-native-windows).

In reply to: 127970487 [](ancestors = 127970487)

collapsible should also be moved, I added a PR - microsoft#1208

In reply to: 127974737 [](ancestors = 127974737,127970487)

I've also moved collapsable to ReactViewManager #Closed

rozele · 2017-07-18T13:27:30Z

ReactWindows/ReactNative.Shared/UIManager/NativeViewHierarchyOptimizer.cs

-
-            if (borderParent != null)
-            {
-                x -= borderParent.GetLeftBorderWidth();


GetLeftBorderWidth [](start = 34, length = 18)

Can you also delete these helpers, which are now no longer needed? I.e., delete ReactShadowNodeExtensions.cs. #Closed

Fixed #Closed

rozele · 2017-07-18T13:29:12Z

ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs

    {
        /// <summary>
+        /// Checks if the Canvas has a Border already.
+        /// </summary>
+        protected override bool HasBorder(TCanvas view)


HasBorder [](start = 32, length = 9)

Maybe we should just leave HasBorder and GetOrCreateBorder abstract, then you can implement them on ReactViewManager. If we're going to keep this base class, I'd rather not tie it to a specific Canvas subclass. #Closed

Fixed #Closed

rozele · 2017-07-20T21:52:05Z

ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs

@@ -12,16 +13,37 @@ namespace ReactNative.UIManager
    /// View parent manager for bordered canvases.
    /// </summary>
    public abstract class BorderedCanvasManager<TCanvas> : BorderedViewParentManager<TCanvas>
-        where TCanvas : Canvas
+        where TCanvas : BorderedCanvas


BorderedCanvas [](start = 24, length = 14)

This can be reverted to Canvas. #Closed

Fixed. I had to move SetDimensions to ReactViewManager as it changes the border size.

rozele · 2017-07-20T22:07:58Z

ReactWindows/ReactNative.Shared/UIManager/BorderedViewParentManager.cs

+        /// Adds a Border to a Canvas if it hasn't been added yet.
+        /// </summary>
+        protected abstract Border GetOrCreateBorder(TFrameworkElement view);
+



Extra newline. #Closed

Fixed #Closed

rozele · 2017-07-20T22:12:35Z

ReactWindows/ReactNative.Shared/UIManager/BorderedViewParentManager.cs

@@ -1,4 +1,5 @@
-using ReactNative.UIManager.Annotations;
+using ReactNative.Reflection;
+using ReactNative.UIManager.Annotations;


Delete this? #Closed

rozele · 2017-07-20T22:12:52Z

ReactWindows/ReactNative.Shared/UIManager/ColorHelpers.cs

+        {
+            return (color & ~Transparent) == 0;
+        }
+


Is this still being used or can it be deleted? #Closed

Yup, it can be deleted too. #Closed

rozele · 2017-07-20T23:45:39Z

ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs

+            for (var i = GetChildCount(parent) - 1; i >= 0; i--)
+            {
+                RemoveChildAt(parent, i);
+            }


Can you inject a HasBorder check and just clear children in case there is no border? That way there's no net performance impact on views without a border. #Closed

Fixed #Closed

It turns out we use RemoveAllChildren quite frequently to tear down views via a depth-first recursive call to this method for the entire subtree being removed. Most views will not have a border, so it is a reasonable optimization.

In reply to: 128656455 [](ancestors = 128656455)

rozele · 2017-07-21T00:32:54Z

    where TCanvas : Canvas

Can you use HasBorder and GetOrCreateBorder to get the border reference and set the dimensions from the base class? I.e.,

        public override void SetDimensions(BorderedCanvas view, Dimensions dimensions)
        {
            base.SetDimensions(view, dimensions);

            if (HasBorder(view))
            {
                var border = GetOrCreateBorder(view);           
                border.Width = dimensions.Width;
                border.Height = dimensions.Height;
            }
        }

For that matter, you could put it in the BorderedViewParentManager base class where the abstracts first show up.

Refers to: ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs:16 in e5f9f64. [](commit_id = e5f9f64, deletion_comment = False)

rozele · 2017-07-27T23:44:16Z

    where TCanvas : Canvas

Is there something blocking you from putting this in the BorderedViewParentManager base class? Honestly the number of base classes here is quite useless at this point. I'm not even sure why all these base classes were introduced 😕

In reply to: 316868201 [](ancestors = 316868201)

Refers to: ReactWindows/ReactNative.Shared/UIManager/BorderedCanvasManager.cs:16 in e5f9f64. [](commit_id = e5f9f64, deletion_comment = False)

rozele · 2017-07-29T01:06:24Z

@antonkh - It turns out SetDimensions won't work after all, because the LayoutAnimation bypasses the SetDimensions API. We'll need to use the XAML binding approach.

rozele · 2017-08-08T21:23:19Z

Closed, thanks for your iterations on this @antonkh

* Integrate 5/14 * Change files * Fix CI #1 * Re-add BoundedConsumableBuffer.h * Integrate 5/18

Create borders on demand

55dbda1

rozele commented Jul 17, 2017

View reviewed changes

antonkh added 7 commits July 17, 2017 17:01

Replaced XAML bindings with SetDimensions

898cccb

Removed a hack from NativeViewHierarchyOptimizer

b4fe16b

Somewhat better way to detect a Border in a Canvas

904bfea

Subclass Canvas to keep a referenece to its Border

b666c67

Fixed code style

23969c6

Fixed code style

702579e

Fixed code style

76639d0

antonkh reviewed Jul 18, 2017

View reviewed changes

rozele commented Jul 18, 2017

View reviewed changes

antonkh added 5 commits July 18, 2017 12:19

Removed the optimization

69e9428

Removed shadow node extension methods

68720bd

Moved the two methods to ReactViewManager

4ed65f5

Moved accessible and pointerEvents to ReactViewManager

b7b14e8

Moved collapsable to ReactViewManager

8e5e6cd

rozele commented Jul 20, 2017

View reviewed changes

antonkh added 3 commits July 20, 2017 15:40

Removed the unused IsTransparent function

9cf5a2f

Moved SetDimensions to ReactViewManager

60e8e2f

Removed unnecessary usings

4a625f2

rozele commented Jul 20, 2017

View reviewed changes

A more efficient version of RemoveAllChildren

e5f9f64

Moved SetDimensions back and fixed it a bit

9831d0f

antonkh added 2 commits July 31, 2017 13:35

Replaced SetDimensions with XAML bindings

00a5e42

Inherit ReactViewManager from ViewParentManager directly

8ba79f0

rozele closed this Aug 8, 2017

acoates-ms pushed a commit that referenced this pull request Jun 4, 2024

Integrate RN Nightly Build 5/18 (microsoft#13282)

c097678

* Integrate 5/14 * Change files * Fix CI #1 * Re-add BoundedConsumableBuffer.h * Integrate 5/18

Create borders on demand #1

Create borders on demand #1

Conversation

rozele commented Jul 17, 2017 • edited Loading

rozele Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

rozele Jul 17, 2017

Choose a reason for hiding this comment

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele commented Jul 17, 2017 • edited Loading

rozele commented Jul 17, 2017

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 18, 2017

Choose a reason for hiding this comment

rozele Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

rozele Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

rozele Jul 18, 2017

Choose a reason for hiding this comment

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 18, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 20, 2017

Choose a reason for hiding this comment

rozele Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 20, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

rozele Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 20, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

antonkh Jul 20, 2017 • edited by rozele Loading

Choose a reason for hiding this comment

rozele Jul 20, 2017

Choose a reason for hiding this comment

rozele commented Jul 21, 2017

rozele commented Jul 27, 2017

rozele commented Jul 29, 2017 • edited Loading

rozele commented Aug 8, 2017

rozele commented Jul 17, 2017 •

edited

Loading

rozele Jul 17, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele Jul 17, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele Jul 17, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele commented Jul 17, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele Jul 18, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele Jul 18, 2017 •

edited

Loading

rozele Jul 18, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele Jul 18, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele Jul 18, 2017 •

edited

Loading

antonkh Jul 18, 2017 •

edited by rozele

Loading

rozele Jul 20, 2017 •

edited

Loading

rozele Jul 20, 2017 •

edited

Loading

antonkh Jul 20, 2017 •

edited by rozele

Loading

rozele Jul 20, 2017 •

edited

Loading

rozele Jul 20, 2017 •

edited

Loading

antonkh Jul 20, 2017 •

edited by rozele

Loading

rozele Jul 20, 2017 •

edited

Loading

antonkh Jul 20, 2017 •

edited by rozele

Loading

rozele commented Jul 29, 2017 •

edited

Loading