Fix 4143: improved Span region calculation

[Braini01]

Description of Change

There are a lot of problems that are connected to the incorrect calculation of the region of a Span, especially on Android. This causes diffferent effects like calling two commands with one tap due to overlapping regions or the reacting area for the tap is not where it is supposed to be.

This PR is an attempt to fix those problems currently for Android only. To completely fix #4143 some problems on iOS need to be fixed to and I am willing to fix them too, but I would like to discuss my Android approach here first with this PR.

Issues Resolved

• fixes Span GestureRecognizer inaccuracies #4143 (on Android)
• fixes [Android] Span gesture recognizer is not triggered when Label has padding #6992
• fixes [Bug][Android] Two spans on multiline label with TapGestureRecognizer events have an incorrect behavior #11650
• fixes [Enhancement] span TapGestureRecognizer to work with arabic font! #7655 (on Android)
• fixes [Bug] Two spans on label have an incorrect behavior #11657
• fixes [Bug] TapGestureRecognizer not working with Spans on iOS when text in the span is multiline #10520 (on Android) (not tested)
• fixes Span Gesture recognizer doesn't work on right to left text (RTL)  #4829 (on Android)

API Changes

Added:

• static Region Region.FromRectangles(IEnumerable rectangles)

Platforms Affected

• Android
• iOS (would add it later, depending on the discussion)

Behavioral/Visual Changes

Some regions now might be shorter, because certain lines were tappable as a whole. Now, it should only go as far as there is some visible character.

Before/After Screenshots

There are no visual changes but in order to verify my code I added code to the LabelRenderer that would draw rectangles around the span regions. Here are the drawn regions before and after my code changes and with and without region inflation:

Current State:

Fixed State:

Current State without inflation:

Fixed State without inflation:

I am not sure if the exisiting region inflation is working as it should, so that's why I put uninflated examples here for better comparison.

Testing Procedure

I created a Test Case in the Android ControlGallery (Issue4143). There are three Labels with text in it and some texts are colored blue to determine a tappable Span. If such a Span is tapped, its background color should change. Make sure that only the tapped Span changes its background color. Also make sure that tapping outside of blue text areas does not trigger any color changes.

PR Checklist

• Targets the correct branch
• Tests are passing (or failures are unrelated)

[Braini01]

Hi there, just would like to know what the state of this PR is. Is it unusable, is it complex and discussed internally or is the review scheduled for a certain point in the future? Just so I know if it is still in the pipe and I will just wait or if I could improve anything or if I should forget about it :)

[adamped]

Having originally wrote the span gesture recognizer code long ago (though to be clear, I don't work for MS, just a contributor), having a look at this PR, it looks great to me. It was never designed with RTL in mind originally either.

Regarding the inflation, the reason it was added, is because it we wanted to give a wider area to tap. While we can see an issue if they are all crammed together, it was designed with just a single link in a large body of text.

But creating the regions only adds them to a list. When the label is tapped, it checks the regions, if it finds 2, it deflates them and only selects the one that it touches when deflated.

You can check out the original PR here: https://github.com/xamarin/Xamarin.Forms/pull/2173/files#diff-567ae32d5aada76aa0f492e11c0f4188f293e2186636f2fd5835db95a3ee2725R305
And look at GetChildElements, you'll see the Deflate.

Also don't be discouraged if they don't respond to you for months. Some of my PRs sit there for months on end. I have one fixing a minor issue for spans in iOS - #11321

[Braini01]

@adamped Thank you very much for the insights. Very much appreciated and helpful :)

I was not discouraged but a little lost. It's my first PR here, so I did not know how things are rollin. So thanks for your information.

[cabal95]

For anybody who needs this ASAP, I was able to manually integrate this into my custom Label Renderer without too much trouble. Had to use a few Reflection lookups to access private fields and constructors, but other than that it worked great to just take the main method that was fixed and drop it straight into my custom renderer.

Thanks @Braini01 for tracking this down and fixing it!

[jsuarezruiz]

[rachelkang]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[rachelkang]

LGTM - thank you for your contribution!

Are you planning on adding the iOS implementation to this PR, or to a separate PR?

[Braini01]

If I come around to do it, I would make a new PR for iOS.

[IeuanWalker]

For anybody who needs this ASAP, I was able to manually integrate this into my custom Label Renderer without too much trouble. Had to use a few Reflection lookups to access private fields and constructors, but other than that it worked great to just take the main method that was fixed and drop it straight into my custom renderer.

Thanks @Braini01 for tracking this down and fixing it!

@cabal95 could you share your code

[cabal95]

@IeuanWalker I had to rip out a few unrelated things as we already had some other custom stuff in our renderer, but this should be the related bits. Note that this is built against Xamarin 5.0.0.1931. If you try to target any other version you might need to make sure the reflected items still exist.

Expand for code

    /// <summary>
    /// Custom <see cref="Label"/> renderer to standardize the rendering of labels.
    /// </summary>
    /// <seealso cref="Xamarin.Forms.Platform.Android.FastRenderers.LabelRenderer" />
    public class LabelRenderer : Xamarin.Forms.Platform.Android.FastRenderers.LabelRenderer
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="LabelRenderer"/> class.
        /// </summary>
        /// <param name="context">The context.</param>
        public LabelRenderer( Context context )
            : base( context )
        {
        }

        #region Temporary Fix for issue #11657

        // This code can be removed when https://github.com/xamarin/Xamarin.Forms/pull/13348 is merged.

        FieldInfo _spannableStringField = typeof( Xamarin.Forms.Platform.Android.FastRenderers.LabelRenderer )
            .GetField( "_spannableString", BindingFlags.NonPublic | BindingFlags.Instance );

        private static readonly ConstructorInfo _regionCtor = typeof( Region )
            .GetConstructor( BindingFlags.NonPublic | BindingFlags.Instance, null, new Type[1] { typeof( IList<Rectangle> ) }, null );

        protected override void OnLayout( bool changed, int left, int top, int right, int bottom )
        {
            base.OnLayout( changed, left, top, right, bottom );
            RecalculateSpanPositionsFixed( this, Element, ( SpannableString ) _spannableStringField.GetValue( this ), new SizeRequest( new Size( right - left, bottom - top ) ) );
        }

        private static void RecalculateSpanPositionsFixed( TextView textView, Label element, SpannableString spannableString, SizeRequest finalSize )
        {
            if ( element?.FormattedText?.Spans == null || element.FormattedText.Spans.Count == 0 )
                return;

            var labelWidth = finalSize.Request.Width;
            if ( labelWidth <= 0 || finalSize.Request.Height <= 0 )
                return;

            if ( spannableString == null || spannableString.Handle == IntPtr.Zero/*spannableString.IsDisposed()*/ )
                return;

            var layout = textView.Layout;
            if ( layout == null )
                return;

            int next = 0;
            int count = 0;

            var padding = element.Padding;
            var padLeft = ( int ) textView.Context.ToPixels( padding.Left );
            var padTop = ( int ) textView.Context.ToPixels( padding.Top );

            for ( int i = 0; i < spannableString.Length(); i = next )
            {
                var type = Java.Lang.Class.FromType( typeof( Java.Lang.Object ) );

                var span = element.FormattedText.Spans[count];

                count++;

                if ( string.IsNullOrEmpty( span.Text ) )
                    continue;

                // find the next span
                next = spannableString.NextSpanTransition( i, spannableString.Length(), type );

                // get all spans in the range - Android can have overlapping spans				
                var spans = spannableString.GetSpans( i, next, type );

                var startSpan = spans[0];
                var endSpan = spans[spans.Length - 1];

                var spanStartOffset = spannableString.GetSpanStart( startSpan );
                var spanEndOffset = spannableString.GetSpanEnd( endSpan );

                var spanStartLine = layout.GetLineForOffset( spanStartOffset );
                var spanEndLine = layout.GetLineForOffset( spanEndOffset );

                //go through all lines that are affected by the span and calculate an rectangle for each
                var spanRectangles = new List<Rectangle>();
                for ( var curLine = spanStartLine; curLine <= spanEndLine; curLine++ )
                {
                    global::Android.Graphics.Rect bounds = new global::Android.Graphics.Rect();
                    layout.GetLineBounds( curLine, bounds );
                    var lineHeight = bounds.Height();
                    var lineStartOffset = layout.GetLineStart( curLine );
                    var lineVisibleEndOffset = layout.GetLineVisibleEnd( curLine );

                    var startOffset = ( curLine == spanStartLine ) ? spanStartOffset : lineStartOffset;
                    var spanStartX = ( int ) layout.GetPrimaryHorizontal( startOffset );

                    var endOffset = ( curLine == spanEndLine ) ? spanEndOffset : lineVisibleEndOffset; ;
                    var spanEndX = ( int ) layout.GetSecondaryHorizontal( endOffset );

                    var spanWidth = spanEndX - spanStartX;
                    var spanLeftX = spanStartX;
                    //if rtl is used, startX would be bigger than endX
                    if ( spanStartX > spanEndX )
                    {
                        spanWidth = spanStartX - spanEndX;
                        spanLeftX = spanEndX;
                    }

                    if ( spanWidth > 1 )
                    {
                        var rectangle = new Rectangle( spanLeftX + padLeft, bounds.Top + padTop, spanWidth, lineHeight );
                        spanRectangles.Add( rectangle );
                    }
                }

                ( ( ISpatialElement ) span ).Region = ( ( Region ) _regionCtor.Invoke( new object[] { spanRectangles.ToList() } ) ).Inflate( 10 );
            }
        }

        #endregion
    }

[dhanaraj94]

@IeuanWalker I had to rip out a few unrelated things as we already had some other custom stuff in our renderer, but this should be the related bits. Note that this is built against Xamarin 5.0.0.1931. If you try to target any other version you might need to make sure the reflected items still exist.

Expand for code

    /// <summary>
    /// Custom <see cref="Label"/> renderer to standardize the rendering of labels.
    /// </summary>
    /// <seealso cref="Xamarin.Forms.Platform.Android.FastRenderers.LabelRenderer" />
    public class LabelRenderer : Xamarin.Forms.Platform.Android.FastRenderers.LabelRenderer
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="LabelRenderer"/> class.
        /// </summary>
        /// <param name="context">The context.</param>
        public LabelRenderer( Context context )
            : base( context )
        {
        }

        #region Temporary Fix for issue #11657

        // This code can be removed when https://github.com/xamarin/Xamarin.Forms/pull/13348 is merged.

        FieldInfo _spannableStringField = typeof( Xamarin.Forms.Platform.Android.FastRenderers.LabelRenderer )
            .GetField( "_spannableString", BindingFlags.NonPublic | BindingFlags.Instance );

        private static readonly ConstructorInfo _regionCtor = typeof( Region )
            .GetConstructor( BindingFlags.NonPublic | BindingFlags.Instance, null, new Type[1] { typeof( IList<Rectangle> ) }, null );

        protected override void OnLayout( bool changed, int left, int top, int right, int bottom )
        {
            base.OnLayout( changed, left, top, right, bottom );
            RecalculateSpanPositionsFixed( this, Element, ( SpannableString ) _spannableStringField.GetValue( this ), new SizeRequest( new Size( right - left, bottom - top ) ) );
        }

        private static void RecalculateSpanPositionsFixed( TextView textView, Label element, SpannableString spannableString, SizeRequest finalSize )
        {
            if ( element?.FormattedText?.Spans == null || element.FormattedText.Spans.Count == 0 )
                return;

            var labelWidth = finalSize.Request.Width;
            if ( labelWidth <= 0 || finalSize.Request.Height <= 0 )
                return;

            if ( spannableString == null || spannableString.Handle == IntPtr.Zero/*spannableString.IsDisposed()*/ )
                return;

            var layout = textView.Layout;
            if ( layout == null )
                return;

            int next = 0;
            int count = 0;

            var padding = element.Padding;
            var padLeft = ( int ) textView.Context.ToPixels( padding.Left );
            var padTop = ( int ) textView.Context.ToPixels( padding.Top );

            for ( int i = 0; i < spannableString.Length(); i = next )
            {
                var type = Java.Lang.Class.FromType( typeof( Java.Lang.Object ) );

                var span = element.FormattedText.Spans[count];

                count++;

                if ( string.IsNullOrEmpty( span.Text ) )
                    continue;

                // find the next span
                next = spannableString.NextSpanTransition( i, spannableString.Length(), type );

                // get all spans in the range - Android can have overlapping spans				
                var spans = spannableString.GetSpans( i, next, type );

                var startSpan = spans[0];
                var endSpan = spans[spans.Length - 1];

                var spanStartOffset = spannableString.GetSpanStart( startSpan );
                var spanEndOffset = spannableString.GetSpanEnd( endSpan );

                var spanStartLine = layout.GetLineForOffset( spanStartOffset );
                var spanEndLine = layout.GetLineForOffset( spanEndOffset );

                //go through all lines that are affected by the span and calculate an rectangle for each
                var spanRectangles = new List<Rectangle>();
                for ( var curLine = spanStartLine; curLine <= spanEndLine; curLine++ )
                {
                    global::Android.Graphics.Rect bounds = new global::Android.Graphics.Rect();
                    layout.GetLineBounds( curLine, bounds );
                    var lineHeight = bounds.Height();
                    var lineStartOffset = layout.GetLineStart( curLine );
                    var lineVisibleEndOffset = layout.GetLineVisibleEnd( curLine );

                    var startOffset = ( curLine == spanStartLine ) ? spanStartOffset : lineStartOffset;
                    var spanStartX = ( int ) layout.GetPrimaryHorizontal( startOffset );

                    var endOffset = ( curLine == spanEndLine ) ? spanEndOffset : lineVisibleEndOffset; ;
                    var spanEndX = ( int ) layout.GetSecondaryHorizontal( endOffset );

                    var spanWidth = spanEndX - spanStartX;
                    var spanLeftX = spanStartX;
                    //if rtl is used, startX would be bigger than endX
                    if ( spanStartX > spanEndX )
                    {
                        spanWidth = spanStartX - spanEndX;
                        spanLeftX = spanEndX;
                    }

                    if ( spanWidth > 1 )
                    {
                        var rectangle = new Rectangle( spanLeftX + padLeft, bounds.Top + padTop, spanWidth, lineHeight );
                        spanRectangles.Add( rectangle );
                    }
                }

                ( ( ISpatialElement ) span ).Region = ( ( Region ) _regionCtor.Invoke( new object[] { spanRectangles.ToList() } ) ).Inflate( 10 );
            }
        }

        #endregion
    }

@cabal95 I have tried to add the custom renderer in my android project not sure why it is not working. I have added the above renderer file and add assembly as below

[assembly: ExportRenderer(typeof(Label), typeof(LabelRenderer))]

[cabal95]

@cabal95 I have tried to add the custom renderer in my android project not sure why it is not working. I have added the above renderer file and add assembly as below

[assembly: ExportRenderer(typeof(Label), typeof(LabelRenderer))]

Depending on your using statements, it might be targetting the original Xamarin.Forms LabelRenderer. Try fully qualifying the ExportRenderer such as (as a habit I always do it this way because I've had this bite me a few times):

[assembly: ExportRenderer(typeof(Label), typeof(MyProject.Droid.LabelRenderer))]

and see if that makes a difference. If not, try setting a breakpoint in the constructor to make sure it is being called, then also in the other methods to make sure they get called when expected.

[dhanaraj94]

@cabal95 I have tried to add the custom renderer in my android project not sure why it is not working. I have added the above renderer file and add assembly as below
[assembly: ExportRenderer(typeof(Label), typeof(LabelRenderer))]

Depending on your using statements, it might be targetting the original Xamarin.Forms LabelRenderer. Try fully qualifying the ExportRenderer such as (as a habit I always do it this way because I've had this bite me a few times):

[assembly: ExportRenderer(typeof(Label), typeof(MyProject.Droid.LabelRenderer))]

and see if that makes a difference. If not, try setting a breakpoint in the constructor to make sure it is being called, then also in the other methods to make sure they get called when expected.

Thanks @cabal95 it referred the default renderer. I have changed it.

[rmarinho]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).