Summary:
With Nodes, we want to support recursive clipping of subviews.
Without this, surfaces like Marketplace won't properly handle subviews.
Reviewed By: sriramramani
Differential Revision: D3904721
Summary:
This fixes a crash for the case when we try to drop a view that has already been dropped.
**The Problem**
We got reports of a crash (t12912526) that occurs when the resolveViewManager method can't resolve a ViewManager for a View being dropped.
Investigating this, one thing in common between all the stack traces for this is that dropView is called from line 210 of FlatNativeViewHierarchyManager. This part of the code is specifically the part we added to remove strong references to any clipped children (from views that have subview clipping enabled).
So this is a problem specifically with Nodes and clipSubviews, which brings up some questions:
**when can this happen?**
The only situation this can possibly happen is when we drop a child (which is clipped) followed by dropping its parent in the same cycle. Consider a tree where each view only has one child, such as: A - B - C - D. This crash would happen if D is clipped, and we removed it, followed by removing any of its parents in the same frame.
**if the removes happen in different frames, does this bug occur?**
No - the reason is that before we execute the DropView operations, we run through StateBuilder, which traverses the shadow tree and marks updates, thus removing the view going away (such that the delete in the next frame doesn't try to re-delete it).
So why doesn't this happen when we're dropping in the same frame? The reason is that manageChildren (where this all starts) asks to remove some views. We handle this by removing said Nodes and their children from the shadow tree. Consequently, when StateBuilder iterates over the shadow tree, it can't do the right thing because said nodes no longer exist.
As a more concrete example, consider A - B - C - D again, and consider that both D and B are removed. StateBuilder only sees A, and realizes that it now has 0 children (whereas before it has 1), so it removes B from its children. However, this process isn't recursive, so C never gets cleaned up.
**why doesn't this happen with Nodes without clipping containers?**
The answer to this is that NativeViewHierarchyManager's dropView method checks the existance of each child before deeply dropping that child and its subtree. So in this case, we drop D and all its children, and when we come to drop B, we try to drop C (which exists) and then its children (D, which doesn't exist because we already dropped it, so we ignore it).
**why doesn't this happen with non-Nodes?**
The reason is that non-Nodes handles removes differently - every remove is enqueued in a call to NativeViewHierarchy's manageChildren, which explicitly asks the parent to remove said child. Consequently, we never try to remove a child that is already removed.
**Fix**
The initial fix was to check whether or not the view exists, but this updated patch just does the right thing at drop time - i.e. whenever a view is dropped, we notify the parent of this fact so that it can clear the reference from clipped views.
**One last Note**
There are two reasons for switching `super.dropView` to `dropView` - first, the comment is only partially correct - calling `super.dropView` will avoid looking at clipped children (as an aside, that could cause a leak in the case of nested clipping subviews), but will look at clipped grandchildren, because of the super class's iteration across the set of children.
Reviewed By: astreet
Differential Revision: D3815485
Summary:
This should probably be two separate diffs, sorry. It takes forever to test these things on fb4a though.
The nodes GK was turned up in fb4a, so I had to make a few changes to make the existing integration work.
Reviewed By: lexs, emilsjolander
Differential Revision: D3863226
Summary:
@public
Introduce `overflow:scroll` so that scrolling can be implemented without the current overflow:visible hackiness. Currently we use AT_MOST to measure in the cross axis but not in the main axis. This was done to enable scrolling containers where children are not constraint in the main axis by their parent. This caused problems for non-scrolling containers though as it meant that their children cannot be measured correctly in the main axis. Introducing `overflow:scroll` fixes this.
Reviewed By: astreet
Differential Revision: D3855801
Summary:
@public
Introduce `overflow:scroll` so that scrolling can be implemented without the current overflow:visible hackiness. Currently we use AT_MOST to measure in the cross axis but not in the main axis. This was done to enable scrolling containers where children are not constraint in the main axis by their parent. This caused problems for non-scrolling containers though as it meant that their children cannot be measured correctly in the main axis. Introducing `overflow:scroll` fixes this.
Reviewed By: astreet
Differential Revision: D3855801
Summary:
@public
This is to be able to depend on ReactClippingViewGroup from BaseViewManager. Devs using ReactClippingViewGroup may need to update their imports when updating past this commit.
Reviewed By: lexs
Differential Revision: D3835328
Summary:
We were always getting LEFT explicitly, and, due to RTL support, we
should be asking for START instead.
Reviewed By: sriramramani
Differential Revision: D3836816
Summary:
@public
Setting the line height with the help of Android-provided StaticLayout is incorrect. A
simple example app will display the following when `setLineSpacing(50.f, 0.f)`
is set: {F62987699}. You'll notice that the height of the first line is a few
pixels shorter than the other lines.
So we use a custom LineHeightSpan instead, which needs to be applied to the text
itself, and no height-related attributes need to be set on the TextView itself.
Reviewed By: lexs
Differential Revision: D3841658
Summary:
Fix TextInput padding on Nodes. We used to not call super and used
to manually do the setting of padding. This stops us from running the same
logic that non-Nodes runs (we bypassed it), so this fixes it.
Reviewed By: astreet
Differential Revision: D3825227
Summary:
Due to the RTL implementation, the ViewProps spacing array has START
and END, whereas Nodes should deal with RIGHT and LEFT directly (just like
non-Nodes does). This is the same implementation in use by non-Nodes.
Reviewed By: astreet
Differential Revision: D3809028
Summary:
Nodes historically had two image implementations -
DrawImageWithDrawee and DrawImageWithPipeline. The drawee implementation
was the default (per request of the Fresco team). At this point, there is
no point of having two (especially since updates to one need to be made to
the other), so this patch removes pipeline.
Reviewed By: sriramramani
Differential Revision: D3755523
Summary:
Nodes would typically clip its images, and then Fresco would then
re-clip as part of ScaleTypeDrawable - in addition to being unnecessary,
it's also incorrect, beacuse it causes the image to be smaller than it
should be.
Reviewed By: sriramramani
Differential Revision: D3754778
Summary:
@public
Setting the line height with the help of Android-provided StaticLayout is incorrect. A
simple example app will display the following when `setLineSpacing(50.f, 0.f)`
is set: {F62987699}. You'll notice that the height of the first line is a few
pixels shorter than the other lines.
So we use a custom LineHeightSpan instead, which needs to be applied to the text
itself, and no height-related attributes need to be set on the TextView itself.
Reviewed By: lexs
Differential Revision: D3751097
Summary:
@public
Setting the line height with the help of Android-provided StaticLayout is incorrect. A
simple example app will display the following when `setLineSpacing(50.f, 0.f)`
is set: {F62987699}. You'll notice that the height of the first line is a few
pixels shorter than the other lines.
So we use a custom LineHeightSpan instead, which needs to be applied to the text
itself, and no height-related attributes need to be set on the TextView itself.
Reviewed By: lexs
Differential Revision: D3751097
Summary:
This is just a minor cleanup, use constants for the LEFT
and RIGHT alignments, since they are hide.
Reviewed By: sriramramani
Differential Revision: D3746019
Summary: Adds a flag that can be set in FlatViewGroup to see known performance issues in Nodes objects. This is mostly useful in internal development of Nodes, and will be a dead code path when not set.
Reviewed By: ahmedre
Differential Revision: D3732675
Summary: Node region bounds are assumed to equal the underlying node bounds. In the case of hit slop, these need to be abstracted.
Reviewed By: ahmedre
Differential Revision: D3713430
Summary:
This is minor, but for our use case a SparseArray is going to be faster as long as we have less than 10,000 clipped subviews, and will also use much less memory.
Faster because of the boxing, unboxing and hash caching; less memory as it is two arrays instead of the object overhead of the HashMap.
Reviewed By: ahmedre
Differential Revision: D3704326
Summary: Previously, the first time we collected a draw view, we would make a clone, even though the draw view had never been mutated. This refactors draw view to avoid this extra allocate.
Reviewed By: ahmedre
Differential Revision: D3719056
Summary:
In Nodes, there were certain cases where text wasn't drawn due to an
optimization that skipped measuring because the size was already known.
Reviewed By: emilsjolander
Differential Revision: D3713841
Summary:
For Nodes that don't mount to views, measureLayout wasn't working
because our calls for getting the width and height would return the view delta
bounds, which won't exist for Nodes. #accept2ship
Differential Revision: D3707880
Summary:
Nodes currently doesn't ask Fresco to resize images, but this is
potentially problematic (ex having a camera photo of 6000x1500 causes a crash
due to the massive size).
Differential Revision: D3687944
Summary: Adds support for horizontal clipping, though the FlatViewGroup needs to be made aware still of which it is.
Reviewed By: ahmedre
Differential Revision: D3673501
Summary: This optimizes node region searches in clipping cases, and does position calculation for drawCommands off of the UI thread.
Reviewed By: ahmedre
Differential Revision: D3665301
Summary: Add jni bindings for csslayout. First step in many of removing LayoutEngine.java and performing all layout in native.
Reviewed By: lucasr
Differential Revision: D3648793
Summary:
Use ImageRequestBuilder directly in Nodes, just like we do for
non-Nodes.
Reviewed By: sriramramani
Differential Revision:
D3660610
Ninja: Sandcastle is broken. 25 denizens of the Facebook republic are affected by this unrelated issue today.
Summary: Add directional aware clipping to DrawCommandManager. Currently not attached to FlatViewGroup logic, with the plan to keep this unattached until we are clipping the way we want to in the final state.
Reviewed By: ahmedre
Differential Revision: D3622253
Summary: @public Make UIOperation public so that custom implementations can expose instances of it.
Reviewed By: ahmedre
Differential Revision: D3618197
Summary:
Support rounded clipping in Nodes. Before, if a view had
a radius and had overflow of hidden, its children could still draw
outside of it (specifically, in the area between the rounded rect
and square rect) - this is due to the fact that clipping is, by
default, rectangular. This patch supports this type of rounded
clipping.
Differential Revision: D3634861
Summary: Previously, we had no information about the positioning of the view until after we had attached it. We have the position information attached to the shadow node, but this attaches it to the DrawView as well. It also removes the need for AbstractClippingDrawCommand.
Reviewed By: ahmedre
Differential Revision: D3609092
Summary: @public Change the textalign setter to support RTL. In order to support text alignment according to layout style, move the textalign setter bridge function from ReactTextViewManager.java to ReactTextShadowNode.java and calculate it correctly on RCTTextUpdate.
Reviewed By: dmmiller
Differential Revision: D3597494
Summary:
This patch fixes measureInWindow for Nodes backed by Views.
Whereas the intention was to call the super implementation when we have a
Node backed by a View, we instead called the super implementation of
measure, which doesn't measure relative to window.
Differential Revision: D3607890
Ahmed El-Helw
Andy Street
Emil Sjolander
Andrew Y. Chen
Andrei Coman
Sriram Ramasubramanian
Seth Kirby
Mengjue Wang