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

Make Label display HTML from a string #4527

Merged
merged 16 commits into from Aug 28, 2019

Conversation

@jfversluis
Copy link
Member

commented Nov 23, 2018

Description of Change

With the new TextType property you can now set HTML formatted content on the Label. In the future other types might be added that use pure text formatting like Markdown. Functionality around FormattedText is unchanged.

In the discussion below we have been exploring the possibility to solve this with different spans. This is possible on iOS, but from what I can tell not on Android (and thus did not explore other platforms). While talking to @PureWeen we decided this was the best alternative.

Although we do allow HTML (through the native handlers for that on iOS and Android) not all tags are supported. We are limited by the support on the different platforms. I feel that might be something that is going to come up in the future.

Issues Resolved

API Changes

Added:

  • public enum TextType
  • TextType Label.TextType { get; set; } //Bindable Property

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP
  • Mac OS

Behavioral/Visual Changes

None by default, but the user is now able to show HTML formatted content by setting the Labels TextType property to TextType.Html. Default is TextType.Text which changes nothing in the default behavior as it is today

Before/After Screenshots

HTML Content on iOS

HTML Content on Android

HTML Content on UWP

Testing Procedure

Test cases were added in the gallery app

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@samhouts samhouts added this to In Review in v3.6.0 Nov 23, 2018

@StephaneDelcroix
Copy link
Member

left a comment

I almost closed this right away, but I went through it to the end, and I might actually like it :)

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I tagged this "do not merge" so we have the time to discuss this internally

@StephaneDelcroix StephaneDelcroix changed the title [Enhancement][Core, iOS, Android, UWP, Mac OS, Tizen] Make Label display HTML from a string Make Label display HTML from a string Nov 24, 2018

@samhouts samhouts moved this from In Review to In Progress in v3.6.0 Nov 26, 2018

@samhouts samhouts added the e/6 🕕 label Nov 27, 2018

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

Implemented UWP with the code from @matteobortolazzo. Left out the <a> tag since I don't think it works on other platforms as well. If anything more is needed let me know :)

@davidortinau davidortinau requested a review from PureWeen Dec 1, 2018

@samhouts samhouts moved this from In Progress to In Review in v3.6.0 Jan 3, 2019

@PureWeen PureWeen self-assigned this Jan 10, 2019

@PureWeen
Copy link
Contributor

left a comment

Switching between HTML and Text seems to not work. It'll switch back to Text fine but then not back to HTML

var label = new Label
			{
				Html = "<h1>Hello world!</h1><p>Lorem <strong>ipsum</strong> bla di bla <i>blabla</i> blablabl ablabla blablablablabl ablabl ablablabl <br/>ablablabla <br/> blablablablablablab<br/> lablablabla blablab lablablabla blablabl ablablablab lablabla blab lablablabla blablab lablabla blablablablab lablabla <br/> blablab lablablabl ablablabla blablablablablabla <div>blablabla</div></p>",
				MaxLines = 5
			};

			return new ContentPage()
			{
				Content = new StackLayout()
				{
					Children =
					{
						label,
						new Button()
						{
							Text = "switching",
							Command = new Command(() =>
							{
								if(label.Html != null)
								{
									label.Text = "i am text";
								}
								else
								{
									label.Html = "<h1>Hello world!</h1><p>Lorem <strong>ipsum</strong> bla di bla <i>blabla</i> blablabl ablabla blablablablabl ablabl ablablabl <br/>ablablabla <br/> blablablablablablab<br/> lablablabla blablab lablablabla blablabl ablablablab lablabla blab lablablabla blablab lablabla blablablablab lablabla <br/> blablab lablablabl ablablabla blablablablablabla <div>blablabla</div></p>";
							}
							})
						}
					}
				}
			};

What are your thoughts on adding a ContentType or TextType property where you set it as HTML opposed to adding an HTML property?

I feel like having two properties that are basically setting the "Text" of the label is going to lead to duplication like I referenced in one of my comments.

Whatever way we go can you add a UI Tests that mimics what I test above?


static void OnHtmlPropertyChanged(BindableObject bindable, object oldvalue, object newvalue)
{
var label = (Label)bindable;

This comment has been minimized.

Copy link
@PureWeen

PureWeen Jan 18, 2019

Contributor

can this part that matches up with OnTextPropertyChanged just be moved to a method

@samhouts samhouts moved this from In Review to In Progress in v3.6.0 Jan 18, 2019

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

Regarding the separate Html property; I totally agree. If we could come up with a way to have it in the existing Text property, that would be fine by me. I mostly implemented it this way because of the spec and that it makes sense to not mess with the existing functionality too much.

You would prefer to simply put the value in Text, lose the Html and have an additional content-type related property then?

Users would even be able to have some kind of HTML/Source view when you toggle the content-type property but not the text, that might be nice.

For the switching text/HTML, I can see this happening on iOS, the height of the Label becomes 0 when switching back to HTML the second time. On Android, the height looks good, but the HTML still doesn't show. Can't really put my finger on why this is happening, but it seems to be something in the Core rather than platform-specific.

Anyway, might be good to agree on the actual spec before moving forward on this.

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

What are your thoughts on adding a ContentType or TextType property where you set it as HTML opposed to adding an HTML property?

if it supports FormattedText too, I'm in. otherwise, not sure if it's worth the pain

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

Then if we would do that, it would look like this.

public enum ContentType
{
    PlainText,
    Html,
    FormattedText
}

I think (Plain)Text and Html speak for themselves, but how would you see FormattedText working? This needs a physically different object than the former two options. So, FormattedText would still need a separate property to take in its value, while the other two are plain strings.

@PureWeen

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

I don't think we can rope FormattedText in for the reasons you've outlined. If we had something like an object Content where it could parade as multiple things maybe. I think of it like settings Text vs Run elements on UWP at this point

The other downside with ContentType is that you have to set more than one thing to achieve HTML but it just doesn't feel right to add additionally mutually exclusive content setters. What about when people really want a markdown view?

I'm thinking TextType for the name as well since that aligns better with the Label APIs

At this point I'd still hold off on making changes just because it'll be annoying to do so before solidifying. I'll get some more buy in and then we'll get changes applied.

@samhouts samhouts added this to In Progress in v4.0.0 Feb 2, 2019

@samhouts samhouts removed this from In Progress in v3.6.0 Feb 2, 2019

@CZEMacLeod

This comment has been minimized.

Copy link

commented Feb 2, 2019

@PureWeen If you are looking at Content Types to display, would this not make sense to expand to allow MarkDown in your ContentType enum? Especially as that is a pure text format like HTML.

@PureWeen

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

@CZEMacLeod it absolutely would!! Which is why I like adding a TextType enum we can grow opposed to just adding additional "content type" apis. A mark down TextType would be pretty awesome!

@mattleibow

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

I saw the words markdown and label and thought of this:
https://docs.microsoft.com/en-us/windows/communitytoolkit/controls/markdowntextblock

UWP is done. 😸

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

@PureWeen ALL TESTS GREEN!!! Quickly merge it! :D

@jsuarezruiz you're on here as reviewer as well, might not hurt to have another pair of eyes have a look at this if you have the time

@jsuarezruiz
Copy link
Collaborator

left a comment

I've been doing tests. On Android it's fine (tests, Core Gallery sample). However, we need to review iOS a little bit more. For example, in Core Gallery when using this sample

, where TextType Html is used, it is rendered by default like this:
Captura de pantalla 2019-08-23 a las 14 54 28

If I tap the Label (there is a GestureRecognizer) it changes to Text mode correctly, and pressing again, change to Html correctly too.
htmllabel-issue

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@jsuarezruiz fixed the issue! Because of the refactorings the font was now applied after the HTML which apparently just removes all HTML styling. Added a TextType == Text check to a couple of methods to prevent weird behavior.

@PureWeen just need one more bit of green in the form of an approval check? 😬

@jsuarezruiz

This comment has been minimized.

Copy link
Collaborator

commented Aug 26, 2019

@jfversluis Now, it works fine on Android. :)

@PureWeen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@jfversluis Bugzilla26993, Bugzilla28001, Bugzilla28575 failures on API 19 Fast Renderers. Make sure to enable Fast Renderers when testing in the gallery :-)

I kicked off the UI tests again just to see if it was a fluke

@PureWeen

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@jfversluis once the tests are all green again merge it

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

Failing tests do seem to be unrelated. Can't seem to trigger a new run. I've retested them locally (with FR on) and they seem to succeed. I think it's good enough!

@jfversluis jfversluis merged commit 938a840 into xamarin:master Aug 28, 2019

18 of 20 checks passed

DROID-API19-FR-ISSUES uitests
Details
DROID-API19-LR uitests
Details
DROID-API19-FR uitests
Details
DROID-API19-LR uitests
Details
DROID-API19-LR-ISSUES uitests
Details
DROID-API19-LR-MANUAL uitests
Details
DROID-API27-FR uitests
Details
DROID-API27-FR-ISSUES uitests
Details
DROID-API27-FR-MANUAL uitests
Details
DROID-API27-LR uitests
Details
DROID-API27-LR-ISSUES uitests
Details
DROID-API27-LR-MANUAL uitests
Details
iOS11 uitests
Details
iOS11-ISSUES uitests
Details
iOS11-MANUAL uitests
Details
iOS12 uitests
Details
iOS12-ISSUES uitests
Details
iOS12-MANUAL uitests
Details
license/cla All CLA requirements met.
Details
xamarin-forms-ci #4.3.0.1862+159-pr.4527-sha.de08bf3c-azdo.5853 succeeded
Details

vNext (4.3.0) automation moved this from In Progress to Done Aug 28, 2019

@jfversluis jfversluis deleted the jfversluis:feature/4504-html-label branch Aug 28, 2019

@davidortinau

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Can we not do HTML in XAML?

                <Label>
                    <Label.Text>
                        <![CDATA[
                        First Line<br/>SecondLine
                        ]]>
                    </Label.Text>
                </Label>
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Hm I missed the CDATA variant. For some reason when you use that the Text property is null. I will see if that is something we can fix. Maybe @StephaneDelcroix has any ideas about that?

It does work when you encode the string like this:

<Label TextType="Html">
    <Label.Text>
        &lt;h1&gt;Hello World!&lt;/h1&gt;&lt;br/&gt;SecondLine
    </Label.Text>
</Label>

Edit: So apparently, when you want to use CDATA you'll have to do this:

                <Label>
                    <Label.Text>
                        <x:String>
                            <![CDATA[
                            First Line<br/>SecondLine
                            ]]>
                        </x:String>
                    </Label.Text>
                </Label>

@samhouts samhouts added this to Done in vNext+1 (master) Aug 30, 2019

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

@jfversluis @davidortinau the <Label.Text> is redundant, as Text is the ContentProperty for Label

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

@jfversluis @davidortinau

this works:

        <Label x:Name="label2"
                TextType="Html">
            <![CDATA[
                <h1>Hello World!</h1><br/>SecondLine
            ]]>
        </Label>

but, as you reported, this assign a null value to Text

        <Label x:Name="label4"
                TextType="Html">
            <Label.Text>
                <![CDATA[
                    <h1>Hello World!</h1><br/>SecondLine
                ]]>
            </Label.Text>
        </Label>
StephaneDelcroix added a commit that referenced this pull request Sep 1, 2019
@StephaneDelcroix StephaneDelcroix referenced this pull request Sep 1, 2019
3 of 3 tasks complete
StephaneDelcroix added a commit that referenced this pull request Sep 1, 2019
StephaneDelcroix added a commit that referenced this pull request Sep 2, 2019

@samhouts samhouts removed this from Done in vNext (4.3.0) Sep 3, 2019

jsuarezruiz added a commit that referenced this pull request Sep 4, 2019
jsuarezruiz added a commit that referenced this pull request Sep 4, 2019
adrianknight89 added a commit to adrianknight89/Xamarin.Forms that referenced this pull request Sep 10, 2019

@samhouts samhouts added this to the 4.3.0 milestone Sep 11, 2019

@samhouts samhouts removed this from Done in vNext+1 (master) Sep 11, 2019

@samhouts samhouts added this to Done in vNext (4.3.0) Sep 11, 2019

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.