Permalink
Browse files

Allow views to be collapsed when pointerEvents is set

Reviewed By: astreet

Differential Revision: D4440164

fbshipit-source-id: 88a710affea229228f9c96b82d0bcf4c81f3205d
  • Loading branch information...
javache authored and facebook-github-bot committed Jan 23, 2017
1 parent 29a996c commit 88eeea0995faa800e3b9e48461f95488c73f7fd9
@@ -446,7 +446,7 @@ private static boolean isLayoutOnlyAndCollapsable(@Nullable ReactStylesDiffMap p
ReadableMapKeySetIterator keyIterator = props.mBackingMap.keySetIterator();
while (keyIterator.hasNextKey()) {
- if (!ViewProps.isLayoutOnly(keyIterator.nextKey())) {
+ if (!ViewProps.isLayoutOnly(props.mBackingMap, keyIterator.nextKey())) {
return false;
}
}
@@ -12,6 +12,8 @@
import java.util.Arrays;
import java.util.HashSet;
+import com.facebook.react.bridge.ReadableMap;
+
/**
* Keys for props that need to be shared across multiple classes.
*/
@@ -64,6 +66,9 @@
public static final String ASPECT_RATIO = "aspectRatio";
+ // Props that sometimes may prevent us from collapsing views
+ public static final String POINTER_EVENTS = "pointerEvents";
+
// Props that affect more than just layout
public static final String ENABLED = "enabled";
public static final String BACKGROUND_COLOR = "backgroundColor";
@@ -151,7 +156,14 @@
PADDING_TOP,
PADDING_BOTTOM));
- public static boolean isLayoutOnly(String prop) {
- return LAYOUT_ONLY_PROPS.contains(prop);
+ public static boolean isLayoutOnly(ReadableMap map, String prop) {
+ if (LAYOUT_ONLY_PROPS.contains(prop)) {
+ return true;
+ } else if (POINTER_EVENTS.equals(prop)) {
+ String value = map.getString(prop);
+ return "auto".equals(value) || "box-none".equals(value);
+ } else {
+ return false;
+ }
}
}
@@ -93,7 +93,7 @@ public void setHitSlop(final ReactViewGroup view, @Nullable ReadableMap hitSlop)
}
}
- @ReactProp(name = "pointerEvents")
+ @ReactProp(name = ViewProps.POINTER_EVENTS)
public void setPointerEvents(ReactViewGroup view, @Nullable String pointerEventsStr) {
if (pointerEventsStr != null) {
PointerEvents pointerEvents =

11 comments on commit 88eeea0

@rigdern

This comment has been minimized.

Show comment
Hide comment
@rigdern

rigdern Jan 27, 2017

Contributor

@javache @astreet Cool, this commit fixes a bug my team hit. I have a couple of questions after looking at this change:

  1. It looks like the default value of pointerEvents is auto. Such views should not be layout-only but it looks like the code still treats them as layout-only. I imagine fixing this bug would regress perf in many apps because a lot of views would no longer be optimized away. To avoid that perf regression while fixing the bug, the default value of pointerEvents could be changed to box-none. Unfortunately, that would be a breaking change. Do you have any thoughts on this?
  2. It looks like setPointerEvents doesn't properly handle the null/undefined case. Rather than reverting the view to its default pointerEvents state of auto, the function looks like it's a no-op.
Contributor

rigdern replied Jan 27, 2017

@javache @astreet Cool, this commit fixes a bug my team hit. I have a couple of questions after looking at this change:

  1. It looks like the default value of pointerEvents is auto. Such views should not be layout-only but it looks like the code still treats them as layout-only. I imagine fixing this bug would regress perf in many apps because a lot of views would no longer be optimized away. To avoid that perf regression while fixing the bug, the default value of pointerEvents could be changed to box-none. Unfortunately, that would be a breaking change. Do you have any thoughts on this?
  2. It looks like setPointerEvents doesn't properly handle the null/undefined case. Rather than reverting the view to its default pointerEvents state of auto, the function looks like it's a no-op.
@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 27, 2017

Contributor

Such views should not be layout-only but it looks like the code still treats them as layout-only.

I don't follow, can you point to what code you're talking about?

Also, with this change, we should only be collapsing more Views, since previously all values of pointerEvents made the view non-collapsable.

It looks like setPointerEvents doesn't properly handle the null/undefined case. Rather than reverting the view to its default pointerEvents state of auto, the function looks like it's a no-op.

Good catch! Want to send a PR? ;)

Contributor

astreet replied Jan 27, 2017

Such views should not be layout-only but it looks like the code still treats them as layout-only.

I don't follow, can you point to what code you're talking about?

Also, with this change, we should only be collapsing more Views, since previously all values of pointerEvents made the view non-collapsable.

It looks like setPointerEvents doesn't properly handle the null/undefined case. Rather than reverting the view to its default pointerEvents state of auto, the function looks like it's a no-op.

Good catch! Want to send a PR? ;)

@javache

This comment has been minimized.

Show comment
Hide comment
@javache

javache Jan 27, 2017

Member

the default value of pointerEvents is auto. Such views should not be layout-only

Can you give a scenario for this? I assumed that if a view was completely layout-only and then also set pointerEvents to auto that shouldn't change any behaviour (since it's the default).

Member

javache replied Jan 27, 2017

the default value of pointerEvents is auto. Such views should not be layout-only

Can you give a scenario for this? I assumed that if a view was completely layout-only and then also set pointerEvents to auto that shouldn't change any behaviour (since it's the default).

@rigdern

This comment has been minimized.

Show comment
Hide comment
@rigdern

rigdern Jan 27, 2017

Contributor

Let me elaborate on point (1). Rereading this commit, I see that my thoughts on a heuristic for layout-only pointerEvents event differ from yours. I think that if a View can be the target of a touch event then it cannot be layout-only. This means these pointerEvents values are not layout-only:

  • auto (default)
  • box-only

And these pointerEvents values are layout-only:

  • none
  • box-none

Why? If a View can be the target of touch events (pointerEvents auto or box-only) then somebody might be listening to Gesture Responder Events on that View. However, if that View is optimized away, those events will never fire.

If I'm right, then we should:

  • Remove auto from the layout-only heuristic and add none to the layout-only heuristic.
  • As I mentioned in (1), think about what to do with the default value which is currently auto (a non layout-only value). Currently, the absence of a pointerEvents prop (aka the default value) is incorrectly treated as layout-only. To fix this bug, we might want to do one of these things:
    • Keep the default auto but ensure these views don't get optimized away. This will probably lead to a perf regression because many Views will no longer be able to be optimized away.
    • Change the default to box-none so we fix the bug and views can be optimized away by default. This would be a breaking change although I don't have a sense for how bad the breaking impact would be.

I know I didn't directly answer the questions you asked. If this elaboration still leaves you with questions, let me know and I'll answer them.

Regarding point (2):

It looks like setPointerEvents doesn't properly handle the null/undefined case. Rather than reverting the view to its default pointerEvents state of auto, the function looks like it's a no-op.

Good catch! Want to send a PR? ;)

I'm not sure when I'll get around to it. If this bug still isn't fixed when I have some free time, I'll send a PR.

Contributor

rigdern replied Jan 27, 2017

Let me elaborate on point (1). Rereading this commit, I see that my thoughts on a heuristic for layout-only pointerEvents event differ from yours. I think that if a View can be the target of a touch event then it cannot be layout-only. This means these pointerEvents values are not layout-only:

  • auto (default)
  • box-only

And these pointerEvents values are layout-only:

  • none
  • box-none

Why? If a View can be the target of touch events (pointerEvents auto or box-only) then somebody might be listening to Gesture Responder Events on that View. However, if that View is optimized away, those events will never fire.

If I'm right, then we should:

  • Remove auto from the layout-only heuristic and add none to the layout-only heuristic.
  • As I mentioned in (1), think about what to do with the default value which is currently auto (a non layout-only value). Currently, the absence of a pointerEvents prop (aka the default value) is incorrectly treated as layout-only. To fix this bug, we might want to do one of these things:
    • Keep the default auto but ensure these views don't get optimized away. This will probably lead to a perf regression because many Views will no longer be able to be optimized away.
    • Change the default to box-none so we fix the bug and views can be optimized away by default. This would be a breaking change although I don't have a sense for how bad the breaking impact would be.

I know I didn't directly answer the questions you asked. If this elaboration still leaves you with questions, let me know and I'll answer them.

Regarding point (2):

It looks like setPointerEvents doesn't properly handle the null/undefined case. Rather than reverting the view to its default pointerEvents state of auto, the function looks like it's a no-op.

Good catch! Want to send a PR? ;)

I'm not sure when I'll get around to it. If this bug still isn't fixed when I have some free time, I'll send a PR.

@rigdern

This comment has been minimized.

Show comment
Hide comment
@rigdern

rigdern Jan 30, 2017

Contributor

@astreet @javache -- at mentioning you in case you didn't get a notification about the comment I added last week.

Contributor

rigdern replied Jan 30, 2017

@astreet @javache -- at mentioning you in case you didn't get a notification about the comment I added last week.

@javache

This comment has been minimized.

Show comment
Hide comment
@javache

javache Jan 31, 2017

Member

'none' cannot be a layout-only property, since it has implications on its subviews. If we were to remove the a View with pointerEvents = 'only', the subviews would receive pointerEvents where they previously didn't.

I still don't completely understand how 'auto' can be an issue here. If there was something listening to the events, we should also be getting an onClick property here, which would invalidate the isLayoutOnly logic. If that wasn't a case, we would have already been dropping views that didn't specify anything for pointerEvents. Or are you saying this was a bug even before this change?

Member

javache replied Jan 31, 2017

'none' cannot be a layout-only property, since it has implications on its subviews. If we were to remove the a View with pointerEvents = 'only', the subviews would receive pointerEvents where they previously didn't.

I still don't completely understand how 'auto' can be an issue here. If there was something listening to the events, we should also be getting an onClick property here, which would invalidate the isLayoutOnly logic. If that wasn't a case, we would have already been dropping views that didn't specify anything for pointerEvents. Or are you saying this was a bug even before this change?

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Feb 1, 2017

Contributor

Well, 'auto' is the default, so making views that have auto explicitly set collapsable isn't changing existing behavior really. That doesn't mean there isn't a possible bug here. Specifically, I think you might hit a bug if you have a collapsed view with no children that is supposed to receive a touch event: in that case, the view wouldn't be enumerated here:

private static View findTouchTargetView(float[] eventCoords, ViewGroup viewGroup) {
. That being said, this would mean that the touch target wouldn't be drawing anything anyway (or else it wouldn't be collapsable) which I don't think is very common. It's still worth fixing, but we'll soon be moving to the 'nodes' implementation of UI on Android which I don't think has this problem (or at the least would require a different fix).

The reason other scenarios aren't affected is because of the way we do touch handling in RN: we deliver the touch event to JS with the deepest view that was touched on the native side. We then bubble that event up thought the JS view hierarchy, meaning collapsing isn't a problem unless it's the deepest views that are collapsed.

The reason box-only and none need to not be collapsed is because they stop touch event traversal to the View's children in native code, which means they need to be enumerated in the TouchTargetHelper code linked above. box-none is ok to leave layout only since we do want to visit the View's children.

I hope this makes sense, it's not the simplest thing in the world :P Also let me know if you think I'm wrong about something here, this is just based on me reading over the code just now.

I'm not sure when I'll get around to it. If this bug still isn't fixed when I have some free time, I'll send a PR.

That's fine, you've done enough on this issue, I'll fix it up today :)

Contributor

astreet replied Feb 1, 2017

Well, 'auto' is the default, so making views that have auto explicitly set collapsable isn't changing existing behavior really. That doesn't mean there isn't a possible bug here. Specifically, I think you might hit a bug if you have a collapsed view with no children that is supposed to receive a touch event: in that case, the view wouldn't be enumerated here:

private static View findTouchTargetView(float[] eventCoords, ViewGroup viewGroup) {
. That being said, this would mean that the touch target wouldn't be drawing anything anyway (or else it wouldn't be collapsable) which I don't think is very common. It's still worth fixing, but we'll soon be moving to the 'nodes' implementation of UI on Android which I don't think has this problem (or at the least would require a different fix).

The reason other scenarios aren't affected is because of the way we do touch handling in RN: we deliver the touch event to JS with the deepest view that was touched on the native side. We then bubble that event up thought the JS view hierarchy, meaning collapsing isn't a problem unless it's the deepest views that are collapsed.

The reason box-only and none need to not be collapsed is because they stop touch event traversal to the View's children in native code, which means they need to be enumerated in the TouchTargetHelper code linked above. box-none is ok to leave layout only since we do want to visit the View's children.

I hope this makes sense, it's not the simplest thing in the world :P Also let me know if you think I'm wrong about something here, this is just based on me reading over the code just now.

I'm not sure when I'll get around to it. If this bug still isn't fixed when I have some free time, I'll send a PR.

That's fine, you've done enough on this issue, I'll fix it up today :)

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Feb 1, 2017

Contributor

If there was something listening to the events, we should also be getting an onClick property here, which would invalidate the isLayoutOnly logic.

We don't send an onClick property to native on either platform afaik, so yes I think this could be a bug, but only in the scenario I mentioned above

Contributor

astreet replied Feb 1, 2017

If there was something listening to the events, we should also be getting an onClick property here, which would invalidate the isLayoutOnly logic.

We don't send an onClick property to native on either platform afaik, so yes I think this could be a bug, but only in the scenario I mentioned above

@rigdern

This comment has been minimized.

Show comment
Hide comment
@rigdern

rigdern Feb 2, 2017

Contributor

@astreet @javache Thanks for the explanation. I now understand why a view with pointerEvents set to none cannot be collapsed in the current implementation.

I think Andy understands the bug I was trying to describe when a view with pointerEvents set to auto gets collapsed. This bug existed both before and after this commit. I want to elaborate on this scenario anyway. Imagine you have 2 views which are siblings. One is layout-only with pointerEvents set to auto and the other is a TouchableHighlight. Suppose the layout-only view is positioned such that it entirely overlaps the TouchableHighlight and it sits on top of the TouchableHighlight. You'll experience different behaviors on iOS and Android due to Android's collapsing view optimization:

  • On iOS, you will not be able to tap on the TouchableHighlight because the layout-only view will eat all of the input.
  • On Android, you will be able to tap on the TouchableHighlight because the layout-only view has been optimized away.

In practice, my team has run into this inconsistency between iOS and Android on several occasions and we workaround it by throwing in a collapsable={false}. :( I'd like to get this inconsistency fixed.

To fix this, I was thinking that views with pointerEvents set to auto cannot be optimized away. Because that's the default, such a change could cause a perf regression. This is why I mentioned we might want to consider changing the default to box-none which can be optimized away.

Andy, you brought up Nodes and how this issue my be fixed in Nodes or require a different fix. I'm not familiar with Nodes so I don't have any comments on that in particular.

I think you understand the edge case I'm worried about with auto. How should we proceed?

By the way, the Navigator was broken by this commit and a workaround of collapsable={false} was added in a83af44. It seems like that might be an indication of a bug introduced by this commit. Do you have an understanding of the root cause?

I'm not sure when I'll get around to it. If this bug still isn't fixed when I have some free time, I'll send a PR.

That's fine, you've done enough on this issue, I'll fix it up today :)

Thanks, I've seen you've already gone and fixed it (866ac17) 😄 .

Contributor

rigdern replied Feb 2, 2017

@astreet @javache Thanks for the explanation. I now understand why a view with pointerEvents set to none cannot be collapsed in the current implementation.

I think Andy understands the bug I was trying to describe when a view with pointerEvents set to auto gets collapsed. This bug existed both before and after this commit. I want to elaborate on this scenario anyway. Imagine you have 2 views which are siblings. One is layout-only with pointerEvents set to auto and the other is a TouchableHighlight. Suppose the layout-only view is positioned such that it entirely overlaps the TouchableHighlight and it sits on top of the TouchableHighlight. You'll experience different behaviors on iOS and Android due to Android's collapsing view optimization:

  • On iOS, you will not be able to tap on the TouchableHighlight because the layout-only view will eat all of the input.
  • On Android, you will be able to tap on the TouchableHighlight because the layout-only view has been optimized away.

In practice, my team has run into this inconsistency between iOS and Android on several occasions and we workaround it by throwing in a collapsable={false}. :( I'd like to get this inconsistency fixed.

To fix this, I was thinking that views with pointerEvents set to auto cannot be optimized away. Because that's the default, such a change could cause a perf regression. This is why I mentioned we might want to consider changing the default to box-none which can be optimized away.

Andy, you brought up Nodes and how this issue my be fixed in Nodes or require a different fix. I'm not familiar with Nodes so I don't have any comments on that in particular.

I think you understand the edge case I'm worried about with auto. How should we proceed?

By the way, the Navigator was broken by this commit and a workaround of collapsable={false} was added in a83af44. It seems like that might be an indication of a bug introduced by this commit. Do you have an understanding of the root cause?

I'm not sure when I'll get around to it. If this bug still isn't fixed when I have some free time, I'll send a PR.

That's fine, you've done enough on this issue, I'll fix it up today :)

Thanks, I've seen you've already gone and fixed it (866ac17) 😄 .

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Feb 2, 2017

Contributor

I agree with wanting to get this inconsistency fixed and trying to get to the bottom of the navigator issue. I'm checking with the nodes team on when they think we'll switch over to nodes in OSS. @javache has a task on him around investigating the navigator issue.

Contributor

astreet replied Feb 2, 2017

I agree with wanting to get this inconsistency fixed and trying to get to the bottom of the navigator issue. I'm checking with the nodes team on when they think we'll switch over to nodes in OSS. @javache has a task on him around investigating the navigator issue.

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Feb 2, 2017

Contributor

A possible fix: if a view has any onTouch listeners set on it, automatically set collapsable=false.

Contributor

astreet replied Feb 2, 2017

A possible fix: if a view has any onTouch listeners set on it, automatically set collapsable=false.

Please sign in to comment.