Skip to content

Commit

Permalink
new removeClippedSubviews implementation (take 2 - recursive)
Browse files Browse the repository at this point in the history 
Reviewed By: mmmulani

Differential Revision: D4081700

fbshipit-source-id: d4079138dc070565e475831e82651c9b2d5b8d59
  • Loading branch information
@majak
majak authored and Facebook Github Bot committed Nov 11, 2016
1 parent a3ad34c commit 625c8cb
Show file tree
Hide file tree
Showing 9 changed files with 620 additions and 217 deletions.
4 changes: 4 additions & 0 deletions Examples/UIExplorer/UIExplorer.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
2DD323EA1DA2DE3F000FE1B8 /* libReact-tvOS.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 2DD323D91DA2DD8B000FE1B8 /* libReact-tvOS.a */; };
3578590A1B28D2CF00341EDB /* libRCTLinking.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 357859011B28D2C500341EDB /* libRCTLinking.a */; };
39AA31A41DC1DFDC000F7EBB /* RCTUnicodeDecodeTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 39AA31A31DC1DFDC000F7EBB /* RCTUnicodeDecodeTests.m */; };
397D6A731DB12C1100E99986 /* RCTSubviewClippingTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 397D6A711DB12C1100E99986 /* RCTSubviewClippingTests.m */; };
3D13F8481D6F6AF900E69E0E /* ImageInBundle.png in Resources */ = {isa = PBXBuildFile; fileRef = 3D13F8441D6F6AF200E69E0E /* ImageInBundle.png */; };
3D13F84A1D6F6AFD00E69E0E /* OtherImages.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 3D13F8451D6F6AF200E69E0E /* OtherImages.xcassets */; };
3D299BAF1D33EBFA00FA1057 /* RCTLoggingTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 3D299BAE1D33EBFA00FA1057 /* RCTLoggingTests.m */; };
Expand Down Expand Up @@ -390,6 +391,7 @@
2DD323A51DA2DD8B000FE1B8 /* UIExplorer-tvOSUnitTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = "UIExplorer-tvOSUnitTests.xctest"; sourceTree = BUILT_PRODUCTS_DIR; };
357858F81B28D2C400341EDB /* RCTLinking.xcodeproj */ = {isa = PBXFileReference; lastKnownFileType = "wrapper.pb-project"; name = RCTLinking.xcodeproj; path = ../../Libraries/LinkingIOS/RCTLinking.xcodeproj; sourceTree = "<group>"; };
39AA31A31DC1DFDC000F7EBB /* RCTUnicodeDecodeTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTUnicodeDecodeTests.m; sourceTree = "<group>"; };
397D6A711DB12C1100E99986 /* RCTSubviewClippingTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTSubviewClippingTests.m; sourceTree = "<group>"; };
3D13F83E1D6F6AE000E69E0E /* UIExplorerBundle.bundle */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = UIExplorerBundle.bundle; sourceTree = BUILT_PRODUCTS_DIR; };
3D13F8401D6F6AE000E69E0E /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; name = Info.plist; path = ../Info.plist; sourceTree = "<group>"; };
3D13F8441D6F6AF200E69E0E /* ImageInBundle.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = ImageInBundle.png; sourceTree = "<group>"; };
Expand Down Expand Up @@ -616,6 +618,7 @@
143BC57C1B21E18100462512 /* UIExplorerUnitTests */ = {
isa = PBXGroup;
children = (
397D6A711DB12C1100E99986 /* RCTSubviewClippingTests.m */,
13B6C1A21C34225900D3FAF5 /* RCTURLUtilsTests.m */,
68FF44371CF6111500720EFD /* RCTBundleURLProviderTests.m */,
1497CFA41B21F5E400C1F8F2 /* RCTAllocationTests.m */,
Expand Down Expand Up @@ -1341,6 +1344,7 @@
1497CFAE1B21F5E400C1F8F2 /* RCTJSCExecutorTests.m in Sources */,
13129DD41C85F87C007D611C /* RCTModuleInitNotificationRaceTests.m in Sources */,
1497CFAD1B21F5E400C1F8F2 /* RCTBridgeTests.m in Sources */,
397D6A731DB12C1100E99986 /* RCTSubviewClippingTests.m in Sources */,
134CB92A1C85A38800265FA6 /* RCTModuleInitTests.m in Sources */,
1497CFB11B21F5E400C1F8F2 /* RCTEventDispatcherTests.m in Sources */,
1497CFB31B21F5E400C1F8F2 /* RCTUIManagerTests.m in Sources */,
Expand Down
Loading

7 comments on commit 625c8cb

@majak
Copy link
Contributor Author

@majak majak commented on 625c8cb Nov 11, 2016

Choose a reason for hiding this comment

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

Looks like I've forgot to make my summary public. So here it goes:

This is a new implementation written from scratch. Important points:

  • it's recursive, so parity with Android is kept and doesn't require any changes in product code
  • is hooked into plain uiviews through a category

This solution traverses all subviews if they are not clipped. That could be wasteful.
We could improve it by tracking if a not clipped view was completely visible before and is still completely visible, since in that case we don't have to recursively evaluate clipping downwards.
Let's do it if we see current approach is not fast enough.

There is one issue: uiview hierarchy doesn't have to match react view hierarchy. In that case we could end up in a state where view unclipping messes up a view hierarchy.
I plan to address this by not clipping subviews that don't match reach hierarchy. If they are not removed they cannot be incorrectly added back. That happens in the next diff.

This recursive approach is inherently slower than what we were doing before, but it's not too bad.
Timings on real iPhone5 for for how long clipping on scroll in our most complex RN surface takes:

  • the old implementation: clipping took on average 1.899764 ms (2000 samples)
  • this new implementation: clipping took on average 2.977569 ms (2000 samples)

@nihgwu
Copy link
Contributor

@nihgwu nihgwu commented on 625c8cb Nov 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@majak so are these commits there for #8607 ？Thank you for your great work on this, I'll test it tomorrow, and could those commits be cherry picked to 0.38-stable?

@majak
Copy link
Contributor Author

@majak majak commented on 625c8cb Nov 11, 2016

Choose a reason for hiding this comment

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

@nihgwu exactly! Please try it out and let me know how it works for you.

I'm not sure if it makes sense to pick it into 0.38-stable. It's been broken for a while and I'd rather let it get more testing before going into release.

@ptomasroos
Copy link
Contributor

Choose a reason for hiding this comment

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

@majak great work! will try it out during this week.

@majak
Copy link
Contributor Author

@majak majak commented on 625c8cb Nov 21, 2016

Choose a reason for hiding this comment

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

Heads up: we've uncovered this solution doesn't work well with modals, so I've reverted the change on master until we come up with a proper fix 😞

@ptomasroos
Copy link
Contributor

Choose a reason for hiding this comment

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

@majak are you working on getting a new version out which works with the internals or do FB not use modals internally?

@xutm
Copy link

@xutm xutm commented on 625c8cb Jan 14, 2020

Choose a reason for hiding this comment

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

whether it can clip ImageView?

Please sign in to comment.