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

Fix gestures in Label Spans #12027

Closed
wants to merge 32 commits into from
Closed

Fix gestures in Label Spans #12027

wants to merge 32 commits into from

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Dec 12, 2022

Description of Change

Fix gestures in Label Spans.

Android
fix-gestures-span-droid
image

iOS/Catalyst
fix-gestures-span-ios

Windows
(Work in progress)

To test/validate the changes, open the .NET MAUI Gallery and navigate to the Label page. Tap the Span with the text "Click Me". If the Span changes the TextColor, is working as expected.

Issues Fixed

Fixes #4734
Fixes #8004

@jsuarezruiz jsuarezruiz added t/bug Something isn't working area-controls-label Label, Span area-gestures Gesture types labels Dec 12, 2022
@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Dec 12, 2022
var span = element.FormattedText.Spans[i];

var inline = control.Inlines.ElementAt(i);
var startRect = inline.ContentStart.GetCharacterRect(LogicalDirection.Forward);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always returning an empty Rect. Reviewing why is happening and possible workarounds.

@IeuanWalker
Copy link

@jsuarezruiz not sure if you have or havnt, but could you add a test for multiple clickable spans.

There was an issue in XF where if you had multiple spans with tap gestures, if you clicked on the last span, all the previous spans tap gestures would also trigger.

It was fixed, just want to make sure it doesnt crop back up in MAUI - xamarin/Xamarin.Forms#11650

@jsuarezruiz
Copy link
Contributor Author

@jsuarezruiz not sure if you have or havnt, but could you add a test for multiple clickable spans.

There was an issue in XF where if you had multiple spans with tap gestures, if you clicked on the last span, all the previous spans tap gestures would also trigger.

It was fixed, just want to make sure it doesnt crop back up in MAUI - xamarin/Xamarin.Forms#11650

Good point, I've checked it.
multiple-spans-click

@IeuanWalker
Copy link

@pictos also wondering if tap gestures will be updated in maui to support keyboard accessibility?

In XF navigating with a keyboard, tap gestured controls were skipped over.

@pictos
Copy link
Contributor

pictos commented Dec 31, 2022

@pictos also wondering if tap gestures will be updated in maui to support keyboard accessibility?

In XF navigating with a keyboard, tap gestured controls were skipped over.

@PureWeen , I belive you have more context to answer this question

@angelru angelru mentioned this pull request Jan 17, 2023
4 tasks
@SadamSkula
Copy link

Hi @jsuarezruiz thank you for hard work on implementing this one, as well as other things on MAUI.
Do we have ETA for having this in production?

Best regards,
Sadam

@hartez hartez marked this pull request as ready for review February 6, 2023 20:27
@hartez
Copy link
Contributor

hartez commented Feb 6, 2023

Pulling this out of draft, need someone to diagnose the failures from the Android API 23 tests.

# Conflicts:
#	src/Controls/samples/Controls.Sample/Pages/Controls/LabelPage.xaml.cs
#	src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt
#	src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
src/Core/tests/DeviceTests.Shared/Stubs/ContextStub.cs Outdated Show resolved Hide resolved
@samhouts samhouts dismissed stale reviews from mandel-macaque and PureWeen February 27, 2023 17:35

Changes made. Please re-review.

if (platformView == null)
return;

if (View.GestureRecognizers.Count == 0)
if (View.GestureController.CompositeGestureRecognizers.Count == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

{
if (Handler is LabelHandler labelHandler && labelHandler.PlatformView is TextBlock textBlock)
{
_textBlock = textBlock;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the native view change here? can't we look at size changes on the Label itself ? Override Measure or something ?

@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

Thank you for your pull request. We are auto-formating your source code to follow our code guidelines.

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

Some comments need to still be addressed and we have some NRE cases in the code.

src/Controls/src/Core/HandlerImpl/Label/Label.Windows.cs Outdated Show resolved Hide resolved
var heights = new List<double>();
for (var i = 0; i < formatted.Spans.Count; i++)
{
var span = formatted.Spans[i];
Copy link
Member

Choose a reason for hiding this comment

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

src/Controls/src/Core/HandlerImpl/Label/Label.Windows.cs Outdated Show resolved Hide resolved
var heights = new List<double>();
for (var i = 0; i < formatted.Spans.Count; i++)
{
var span = formatted.Spans[i];
Copy link
Member

Choose a reason for hiding this comment

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

Still stands.

Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

The targets seem a bit off on iOS.

Testing this on iOS, I can tap above and below the "Click Me" Label and it changes the colors - for example, I can tap the "Colors" Label above "Click Me" and the event fires.

Same thing with the part that says "Two clickable spans in one line" - I can tap the word "line" and it causes the color change.

@jsuarezruiz
Copy link
Contributor Author

This PR affects all platforms, and has quite a few changes here and there. To facilitate its review and have fixes in releases as soon as possible, I am going to close this PR to open smaller PRs focused on each platform.

@jsuarezruiz jsuarezruiz closed this Apr 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2023
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-label Label, Span area-gestures Gesture types t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TapGestureRecognizer does not work in label's span Gestures don't work on Label Spans
9 participants