Skip to content

Conversation

@rectified95
Copy link
Contributor

@rectified95 rectified95 commented Apr 1, 2021

This reverts commit 7d293b3.

#6692 fixed a crash we experienced in the e2e app when clicking on the test result hyperlink (a TextBlock created outside of RNW, which didn't have a Tag, and our hit test crashed when trying to access the TagProperty which wasn't there.)

However, this broke the entire hit testing algorithm for nested Text elements - the added check for whether an element was a FrameworkElement before calling el.GetValue(TagProperty) skips the iteration over Inline elements of a Span in a TextBlock, because they're not FE.

Instead, using ReadLocalValue since it returns UnsetValue in case a dependency property is not present on an element.

Microsoft Reviewers: Open in CodeFlow

@rectified95 rectified95 requested a review from a team as a code owner April 1, 2021 19:48
@asklar
Copy link
Member

asklar commented Apr 1, 2021

#6992 fixed a crash we experienced in the e2e app when clicking on the test result hyperlink

That bug is about animations on RS5, is that the right link?

Copy link
Member

@asklar asklar left a comment

Choose a reason for hiding this comment

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

I think the fix you want instead of reverting it all is to close the "if (try as framework element)" block right after assigning tag. That way if we didn't assign it, tag will still be null

@ghost ghost added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Apr 1, 2021
@rectified95
Copy link
Contributor Author

Fixed the bug/PR link in description, and replied to comment thread.

@rectified95 rectified95 changed the title Revert "Fix touch event crash (#6692)" Restore nested press events in Text, and prevent crash. Apr 2, 2021
return fe.GetValue(xaml::FrameworkElement::TagProperty()).try_as<winrt::IPropertyValue>();
}

inline int64_t GetTag(XamlView view) {
Copy link
Member

Choose a reason for hiding this comment

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

nit, another option is to return optional<int64_t> which might be nicer than using -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a nice idea. Would I then return early from a function calling this and getting an empty optional (or a -1 for that matter)?

Copy link
Member

Choose a reason for hiding this comment

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

yes same idea, instead of checking for special -1 value you just check foo.has_value()

@rectified95 rectified95 merged commit 539f941 into microsoft:master Apr 5, 2021
@rectified95 rectified95 linked an issue Apr 5, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Press events broken for nested Text

3 participants