Skip to content

Commit

Permalink
Simplify Flyout and Popup coordinate spaces (#11822)
Browse files Browse the repository at this point in the history
* Simplify Flyout and Popup coordinate spaces

We are seeing some issues with pointer event handling in XAML Islands
apps. It appears that part of the issue has to do with the coordinate
spaces not exactly lining up. It is too difficult to reason about the
coordinate spaces when there are minor differences in measurement
behavior. The coordinate spaces for TouchEventHandler and
UIManager.measure need to line up exactly for Pressable to work as
expected (as Pressable calls UIManager.measure to ensure the pointer is
still in the hit box after a pointerMove event occurs).

TouchEventHandler transforms the RoutedPointerEventArgs
pointer position to the view passed to the TouchEventHandler:

UIManager.measure measures against either the ReactRootView or the view
corresponding to the last ancestor before a ShadowNode with IsWindowed
set to true.

This latter logic is the part in UIManager.measure is the part that
complicates the reasoning about coordinate spaces. Even though it's
designed to measure against the same native component, it's an extra
(and arguably unnecessary) step to figuring out if the coordinate spaces
match. It's much simpler to assume that all coordinate spaces are
relative to the ReactRootView.

This change should probably be marked as breaking, even though it could
conceivably be considered a bug fix. Breaking in part because the X/Y
positions returned from UIManager.measure will no longer be relative to
the Flyout or Popup root. Also breaking because `pointerEvents` values
for nodes above the Flyout will now impact the Flyout.

For example:
```
<View pointerEvents="none">
  <Flyout isOpen={true}>
    <Button title="Click Me" onPress={() => alert("Pressed")} />
  </Flyout>
</View>
```

Previously, the Flyout content would not respect the ancestors
`pointerEvents` value. With this change, the pointer event will
propagate through the entire tree, and respect any
`"box-none"` or `"none"` props for ancestors in the main window.

* Change files
  • Loading branch information
rozele committed Oct 25, 2023
1 parent 53456b7 commit d603706
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 51 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Simplify Flyout and Popup coordinate spaces",
"packageName": "react-native-windows",
"email": "email not defined",
"dependentChangeType": "patch"
}
31 changes: 4 additions & 27 deletions vnext/Microsoft.ReactNative/Modules/NativeUIManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -999,34 +999,11 @@ void NativeUIManager::measure(
return;
}

// Traverse up the react node tree to find any windowed popups.
// If there are none, then we use the top-level root provided by our caller.
xaml::FrameworkElement feRootView = nullptr;
int64_t rootTag = shadowNode.m_tag;
int64_t childTag = rootTag;
while (true) {
auto &currNode = m_host->GetShadowNodeForTag(rootTag);
if (currNode.m_parent == InvalidTag)
break;
ShadowNodeBase &rootNode = static_cast<ShadowNodeBase &>(currNode);
if (rootNode.IsWindowed()) {
ShadowNodeBase &childNode = static_cast<ShadowNodeBase &>(m_host->GetShadowNodeForTag(childTag));
feRootView = childNode.GetView().try_as<xaml::FrameworkElement>();
break;
}
childTag = currNode.m_tag;
rootTag = currNode.m_parent;
}

// Retrieve the XAML element for the root view containing this view
auto feRootView = static_cast<ShadowNodeBase &>(shadowRoot).GetView().try_as<xaml::FrameworkElement>();
if (feRootView == nullptr) {
// Retrieve the XAML element for the root view containing this view
if (auto xamlRootView = static_cast<ShadowNodeBase &>(shadowRoot).GetView()) {
feRootView = xamlRootView.as<xaml::FrameworkElement>();
}
if (feRootView == nullptr) {
m_context.JSDispatcher().Post([callback = std::move(callback)]() { callback(0, 0, 0, 0, 0, 0); });
return;
}
m_context.JSDispatcher().Post([callback = std::move(callback)]() { callback(0, 0, 0, 0, 0, 0); });
return;
}

winrt::Rect rectInParentCoords = GetRectOfElementInParentCoords(feView, feRootView);
Expand Down
6 changes: 1 addition & 5 deletions vnext/Microsoft.ReactNative/Views/FlyoutViewManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,6 @@ class FlyoutShadowNode : public ShadowNodeBase {
winrt::Flyout GetFlyout();
void AdjustDefaultFlyoutStyle(float maxWidth, float maxHeight);

bool IsWindowed() override {
return true;
}

private:
void SetTargetFrameworkElement();
winrt::Popup GetFlyoutParentPopup() const;
Expand Down Expand Up @@ -152,7 +148,7 @@ FlyoutShadowNode::~FlyoutShadowNode() {

void FlyoutShadowNode::AddView(ShadowNode &child, int64_t /*index*/) {
auto childView = static_cast<ShadowNodeBase &>(child).GetView();
m_touchEventHandler->AddTouchHandlers(childView);
m_touchEventHandler->AddTouchHandlers(childView, GetRootView());
m_previewKeyboardEventHandlerOnRoot->hook(childView);

if (m_flyout != nullptr) {
Expand Down
6 changes: 1 addition & 5 deletions vnext/Microsoft.ReactNative/Views/PopupViewManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ class PopupShadowNode : public ShadowNodeBase {
void UpdateTabStops();
void UpdateLayout();

bool IsWindowed() override {
return true;
}

private:
bool m_autoFocus{true};
double m_verticalOffset{0};
Expand Down Expand Up @@ -132,7 +128,7 @@ void PopupShadowNode::AddView(ShadowNode &child, int64_t index) {

control.Content(childView);

m_touchEventHandler->AddTouchHandlers(childView);
m_touchEventHandler->AddTouchHandlers(childView, GetRootView());
m_previewKeyboardEventHandlerOnRoot->hook(childView);
}

Expand Down
11 changes: 11 additions & 0 deletions vnext/Microsoft.ReactNative/Views/ShadowNodeBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,17 @@ void ShadowNodeBase::UpdateTransformPS() {
}
}

XamlView ShadowNodeBase::GetRootView() {
if (auto uiManager = GetNativeUIManager(GetViewManager()->GetReactContext()).lock()) {
auto shadowNode = uiManager->getHost()->FindShadowNodeForTag(m_rootTag);
if (!shadowNode)
return nullptr;

return static_cast<::Microsoft::ReactNative::ShadowNodeBase *>(shadowNode)->GetView();
}
return nullptr;
}

void ShadowNodeBase::UpdateHandledKeyboardEvents(
std::string const &propertyName,
winrt::Microsoft::ReactNative::JSValue const &value) {
Expand Down
5 changes: 2 additions & 3 deletions vnext/Microsoft.ReactNative/Views/ShadowNodeBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,6 @@ struct REACTWINDOWS_EXPORT ShadowNodeBase : public ShadowNode {
int64_t GetParent() const {
return m_parent;
}
virtual bool IsWindowed() {
return false;
}

void ReplaceView(XamlView view);

Expand Down Expand Up @@ -136,6 +133,8 @@ struct REACTWINDOWS_EXPORT ShadowNodeBase : public ShadowNode {
bool m_isDisabled = false;
comp::CompositionPropertySet m_transformPS{nullptr};

XamlView GetRootView();

public:
double m_padding[(int)ShadowEdges::CountEdges] = INIT_UNDEFINED_EDGES;
double m_border[(int)ShadowEdges::CountEdges] = INIT_UNDEFINED_EDGES;
Expand Down
11 changes: 0 additions & 11 deletions vnext/Microsoft.ReactNative/Views/TextViewManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -180,17 +180,6 @@ class TextShadowNode final : public ShadowNodeBase {
}
}

XamlView GetRootView() {
if (auto uiManager = GetNativeUIManager(GetViewManager()->GetReactContext()).lock()) {
auto shadowNode = uiManager->getHost()->FindShadowNodeForTag(m_rootTag);
if (!shadowNode)
return nullptr;

return static_cast<::Microsoft::ReactNative::ShadowNodeBase *>(shadowNode)->GetView();
}
return nullptr;
}

TextTransform textTransform{TextTransform::Undefined};
std::shared_ptr<bool> selectionChanged = std::make_shared<bool>(false);
};
Expand Down

0 comments on commit d603706

Please sign in to comment.