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

[Enhancement] Allow underline and strikethrough text decorations on labels and spans #2221

Merged
merged 19 commits into from Jul 20, 2018

Conversation

@alanag13
Copy link
Contributor

alanag13 commented Apr 1, 2018

Description of Change

Fixes #1632.

Adds the ability to apply underline and strikethrough text decorations to Labels and Spans via a new BP, TextDecorations. Includes support for the CSS text-decoration property.

Note that this implementation is a bit different than what is laid out in #1632, I spoke with @jassmith on this a little bit ago and we agreed on this new approach but I'm happy to answer any questions you might have.

Platform support:

  • Android
  • iOS
  • macOS
  • UWP
  • WPF
  • Tizen
  • GTK

API Changes

enum TextDecorations None, Underline, Strikethrough
interface IDecorableText
TextDecorations IDecorableText.TextDecorations //Bindable Property

Label and Span now implement IDecorableText.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
else
Control.TextDecorations |= Windows.UI.Text.TextDecorations.Strikethrough;

Control.Text = Control.Text; //TextDecorations are not updated in the UI until the text changes

This comment has been minimized.

@alanag13

alanag13 Apr 1, 2018

Author Contributor

This was an oddity I observed on UWP -- it seems that changes to TextDecorations do not propagate to the UI until the next time the text changes, as if TextDecorations only applies to each new string assigned to Text.

This comment has been minimized.

@PureWeen

PureWeen Jun 1, 2018

Contributor

So I think this is the code that is causing the exception on UWP. If the Control TEXT is set via "spans" then this line of code wipes out the inline runs on the control which then causes the exception to happen in RecalculateSpanPositions so this code here is going to need to check if text is set via spans and if so then iterate over those instead of setting the Text to itself

{
None = 0,
Strikethrough = 1 << 1,
Underline = 1 << 0

This comment has been minimized.

@rookiejava

rookiejava Apr 2, 2018

Collaborator

is this intended order?

This comment has been minimized.

@alanag13

alanag13 Apr 2, 2018

Author Contributor

I usually alphabetize them, and the values are to match their values on UWP, but I can shift them if desired.

This comment has been minimized.

@rookiejava

rookiejava Apr 4, 2018

Collaborator

I don't think alphabetical order is required for enum. There is no such requirement and it is difficult to find such an example. Would be better to shift Strikethrough and Underline.

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

I'd say shift them. Mainly because all the other [FLAGS] are in numerical order

This comment has been minimized.

@myroot

myroot May 17, 2018

Contributor

If you want alphabetical order, how about exchange values?

 None = 0,
 Strikethrough = 1 << 0,
 Underline = 1 << 1
Show resolved Hide resolved Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs
@@ -38,6 +39,8 @@ protected override void OnElementPropertyChanged(object sender, PropertyChangedE

if (e.PropertyName == Label.TextProperty.PropertyName || e.PropertyName == Label.FormattedTextProperty.PropertyName)
UpdateText();
if (e.PropertyName == Label.TextDecorationsProperty.PropertyName)

This comment has been minimized.

@rookiejava

rookiejava Apr 2, 2018

Collaborator

else if

This comment has been minimized.

@alanag13

alanag13 Apr 2, 2018

Author Contributor

good catch, I'll change this.

@samhouts samhouts added this to Ready in v3.1.0 via automation Apr 2, 2018

@samhouts samhouts moved this from Ready to In Review in v3.1.0 Apr 2, 2018

@@ -61,35 +61,54 @@ internal static NSAttributedString ToAttributed(this Span span, Element owner, C
if (text == null)
return null;

var range = new NSRange(0, text.Length);

This comment has been minimized.

@jerone

This comment has been minimized.

@alanag13

alanag13 Apr 2, 2018

Author Contributor

Guilty as charged for copying from my iOS LabelRenderer work, will fix this too, thanks.

@alanag13

This comment has been minimized.

Copy link
Contributor Author

alanag13 commented Apr 3, 2018

@hartez it looks like most of the docs got deleted from master, not sure what the reason for that is but let me know if there's anything I should do here

@hartez

This comment has been minimized.

Copy link
Member

hartez commented Apr 3, 2018

@alanag13 We've moved the docs stuff out of the repo entirely. So you can just resolve those conflicts by deleting the conflicting docs folder (docs/Xamarin.Forms.Core) in your PR.

@alanag13

This comment has been minimized.

Copy link
Contributor Author

alanag13 commented Apr 3, 2018

@hartez thanks for the clarification, done

@alanag13 alanag13 force-pushed the XamarinFormsCommunity:feature/1632-label-underline-strike branch from cd08178 to a355ec1 Apr 4, 2018

@samhouts samhouts added this to In Progress in Enhancements Apr 13, 2018

@PureWeen
Copy link
Contributor

PureWeen left a comment

Looks and works great. Just a few cosmetic changes

{
None = 0,
Strikethrough = 1 << 1,
Underline = 1 << 0

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

I'd say shift them. Mainly because all the other [FLAGS] are in numerical order

var toggleUnderline = new Label { Text = "Tap to toggle Underline", TextDecorations = TextDecorations.Underline };
toggleUnderline.GestureRecognizers.Add(new TapGestureRecognizer { Command = new Command(()=> { toggleUnderline.TextDecorations ^= TextDecorations.Underline; }) });
var toggleStrike = new Label { Text = "Tap to toggle StrikeThrough", TextDecorations = TextDecorations.Strikethrough };
toggleStrike.GestureRecognizers.Add(new TapGestureRecognizer { Command = new Command(() => { toggleStrike.TextDecorations ^= TextDecorations.Strikethrough; }) });

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

Can you add one more here that has both of them set?

@@ -57,6 +57,9 @@ public static SpannableString ToAttributed(this FormattedString formattedString,
#pragma warning restore 618
else if (defaultFont != Font.Default)
spannable.SetSpan(new FontSpan(defaultFont, view), start, end, SpanTypes.InclusiveInclusive);

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

Delete extra line


namespace Xamarin.Forms
{
public interface IDecorableText

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

Spaces => tabs

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

use IDecorableTextElement for consistency

@@ -26,7 +26,17 @@ public static Run ToRun(this Span span)
#pragma warning disable 618
run.ApplyFont(span.Font);
#pragma warning restore 618
if (! span.IsSet(Span.TextDecorationsProperty))

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

remove space

}

[Xaml.TypeConversion(typeof(TextDecorations))]
public class TextDecorationConverter : TypeConverter

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

Can we extend this to support multiple properties like
https://github.com/xamarin/Xamarin.Forms/blob/016bd546cbe1262766be84d88bbe0cc582a285a2/Xamarin.Forms.Core/FontTypeConverter.cs

Two notes about this though

  • FontTypeConverter uses a comma to split out values which I don't think is correct css (need to verify intent with some folks)
  • the code could probably be centralized that performs the splitting and reused

This comment has been minimized.

@alanag13

alanag13 May 3, 2018

Author Contributor

Font is a css property that takes multiple arguments such as a list of fall back typefaces, size, etc and comma separated values are actually part of its proper syntax. So this all makes sense for Font. You cannot specify multiple text decorations using css, though there are some hacks using border you can use to achieve this: https://stackoverflow.com/questions/45932420/css-underline-strikethrough-and-overline-with-styles-and-colors-in-one-elem. Devs familiar with css would not expect this property to accept a comma separated list.

This comment has been minimized.

@alanag13

alanag13 May 3, 2018

Author Contributor

This does mean that it would be impossible to set both underline and strikethrough using only css, but I think thats a pretty niche use case, and can still always be implemented with an effect,

This comment has been minimized.

@PureWeen

PureWeen May 3, 2018

Contributor

It seems like CSS just works when doing it

https://jsfiddle.net/L79dsh30/
https://stackoverflow.com/questions/9882443/css-multiple-text-decoration/9882469

I do see now that I was wrong about the commas and fonts

This comment has been minimized.

@alanag13

alanag13 May 3, 2018

Author Contributor

Interesting, I was unaware of that -- goes to show how "helpful" SO can be sometimes.
I'll get this added along with your other suggestions, I should have some time tonight.

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

most of our TypeConverters accept both CSS and Xaml syntax, and we're not making a big deal if you use one for the other. In both cases, using space or comma as separator, we should be able to set multiple ones at once

@StephaneDelcroix StephaneDelcroix self-requested a review May 3, 2018

@rmarinho

This comment has been minimized.

Copy link
Member

rmarinho commented May 7, 2018

@alanag13 this also needs rebase to pick new certs to run on our CI. thanks

@alanag13

This comment has been minimized.

Copy link
Contributor Author

alanag13 commented May 7, 2018

@rmarinho @PureWeen sorry for the delay on this, I'm out of the country until 5/15 but shouldn't take much for me to get this all corrected once I get back.

@StephaneDelcroix
Copy link
Member

StephaneDelcroix left a comment

please rename the interface.

}

[Xaml.TypeConversion(typeof(TextDecorations))]
public class TextDecorationConverter : TypeConverter

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

most of our TypeConverters accept both CSS and Xaml syntax, and we're not making a big deal if you use one for the other. In both cases, using space or comma as separator, we should be able to set multiple ones at once


namespace Xamarin.Forms
{
public interface IDecorableText

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix May 7, 2018

Member

use IDecorableTextElement for consistency

Show resolved Hide resolved Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs

@samhouts samhouts added this to In Review in vCurrent (3.6.0) May 7, 2018

@samhouts samhouts removed this from In Review in v3.1.0 May 7, 2018

void UpdateTextDecorations()
{
var textDecorations = Element.TextDecorations;
Control.Strikethrough = (textDecorations & TextDecorations.Strikethrough) != 0;

This comment has been minimized.

@myroot

myroot May 17, 2018

Contributor

Please, wrap it with Control.BatchBegin() and Control.BatchCommit() for performance like the UpdateFontProperties()
and UpdateFormattedText() no need to call

@davidortinau davidortinau added this to the 3.1.0 milestone May 17, 2018

@samhouts samhouts removed this from In Review in vCurrent (3.6.0) May 18, 2018

@samhouts samhouts removed this from the 3.1.0 milestone May 18, 2018

@samhouts samhouts added this to In Review in v3.1.0 May 18, 2018

@samhouts samhouts added this to In Review in vCurrent (3.6.0) May 18, 2018

@samhouts samhouts removed this from In Review in v3.1.0 May 18, 2018

@alanag13 alanag13 force-pushed the XamarinFormsCommunity:feature/1632-label-underline-strike branch from c47872e to 6697f15 Jul 13, 2018

@alanag13

This comment has been minimized.

Copy link
Contributor Author

alanag13 commented Jul 13, 2018

@PureWeen it looks like your assumption re: the UWP bug was correct. This is resolved now and I added the converter test / other small cleanup points, so this should hopefully be finally good to go 😄

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Jul 17, 2018

Yay!! Once we get through the uitests we'll merge

@alanag13

This comment has been minimized.

Copy link
Contributor Author

alanag13 commented Jul 17, 2018

@PureWeen i can't see the failures but let me know if there's anything else you need from me.

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Jul 17, 2018

Yea those don't look like they are related to this

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Jul 17, 2018

build --uitests

@PureWeen PureWeen merged commit 97d2f30 into xamarin:master Jul 20, 2018

12 of 13 checks passed

VSTS: Xamarin Forms (PR Builds) PR-2221 - (1868069) failed
Details
VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: Xamarin Forms OSX PR-2221 - (1868153) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-2221 - (1868076) succeeded with issues
Details
VSTS: iOS10 Validation UITests Finished
Details
VSTS: iOS11 Validation UITests Finished
Details
VSTS: iOS9 Validation UITests Finished
Details
license/cla All CLA requirements met.
Details

vCurrent (3.6.0) automation moved this from In Review to Done Jul 20, 2018

@PureWeen

This comment has been minimized.

Copy link
Contributor

PureWeen commented Jul 20, 2018

@alanag13 YAY!!!!

Have a good weekend

@Goldstrike

This comment has been minimized.

Copy link

Goldstrike commented Jul 25, 2018

So this feature will be available in the next update?

PureWeen added a commit that referenced this pull request Aug 28, 2018

[Enhancement] Allow underline and strikethrough text decorations on l…
…abels and spans (#2221)

* Fixes #1632

* Allow underline and strikethrought text decorations on labels and spans

* revert some files

* pr feedback adjustments

* remove docs

* rename interface

* reorder enum

* clean up whitespace

* adjust tizen renderer

* add gallery demo for setting both underline and strike

* allow multiple values of enum to be set in xaml/css

* use normal null check

* use nameof

* include paragraph style

* tab alignment

* rebase from upstream

* pass control to update method on UWP

* correct text decorations type converter

* reset run text instead of label text on UWP when spans are used

* add tests for text decoration converter

@samhouts samhouts added this to the 3.3.0 milestone Sep 20, 2018

@samhouts samhouts removed this from Done in vCurrent (3.6.0) Sep 21, 2018

@samhouts samhouts added this to Done in v3.4.0 Sep 21, 2018

@samhouts samhouts removed this from Done in v3.4.0 Nov 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.