Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Infrastructure] Fix widget tester tapping on block component (Resolves #2197) #2207

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

angelosilvestre
Copy link
Collaborator

[Infrastructure] Fix widget tester tapping on block component. Resolves #2197

Trying to tap on the downstream position of a block component is always resulting in an upstream selection.

The issue is that the computation to determine the offset to tap relies on getRectForPosition. For block components, this always return a rect of the entire component, as is documented in the method:

/// Returns a [Rect] that bounds this entire box component.
///
/// The behavior of this method is the same, regardless of whether the given
/// [nodePosition] is `upstream` or `downstream`.
@override
Rect getRectForPosition(NodePosition nodePosition) {
   ///
}

This PR changes the tapAtDocumentPosition to handle block components differently, by computing its edge position and returning a Rect of half of the component's width.

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

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

I left a comment. Also, to ensure this works, can you adjust whichever test(s) I recently committed where I wanted to use this, but I couldn't?

);

// Translate the rect to global coordinates.
final documentLayoutElement = _findDocumentLayoutElement(superEditorFinder);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that the need to access the document element isn't great. Does the DocumentLayout interface not offer any way to accomplish this goal as-is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't find any method to allow that. The Rect returned by Component.getEdgeForPosition is in component space and I didn't find any other method to convert it to document space.

@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll I search on the current main but I didn't find any tests like the one posted in the issue ticket, where we manually place the selection using UpstreamDownstreamNodePosition.downstream().

Should I include that test?

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

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

LGTM - We can leave this as-is for now so we can move forward, but this block position calculation should probably be a part of the real layout and component system, and that real system should also make it possible to get to global coordinates.

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.

[Infrastructure] - Fix widget tester tapping on block nodes downstream
2 participants