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 Android FormattedText and related platforms #7219

Merged
merged 9 commits into from May 17, 2022
Merged

Conversation

mattleibow
Copy link
Contributor

@mattleibow mattleibow commented May 16, 2022

Description of Change

  • Label sometimes displays hugely wrong font size and spacing #6801 - Initialize the span font size to NaN
    This initial value of NaN (or could be 0) indicates to the layout engine that the size must come from the Label.
    If we return a value in IFontElement.FontSizeDefaultValueCreator(), then there is no way of knowing that the size was not actually set since this marks the BindableProperty as "set". The reason it works after an update is because on the second time around, the span is not yet attached to the view (see Shift Handler?.UpdateValue to happen after all PropertyChanged events have fired #5587) and as a result there is no maui context and the value falls back to 0 which triggers the use of the label's font size.
    If the size is NaN, it falls back to the "default font size" which is not really "default" but actually "fallback" - the font size of the label.
  • Android FormattedText and LineHeights are not correct #7220 - Correctly apply span values to spans
    The previous implementation captured the TextView.Paint and used that as the base line height, but like the previous issue, the paint is before any Label font sizes are set.
    We never actually want to use this as if the line height of the label is different to the label of the span (span's font size is larger), then the span's line height is ignored.

Issues Fixed

Pictures!

Initial Update
Before image image
#6801 image image
#7220 image image

mattleibow added 2 commits May 16, 2022
This initial value of Nan (or could be 0) indicates to the layout engine that the size must come from the Label. If we set it here, then there is no way of knowing that the size was not actually set so uses the size. If the size is Nan, it falls back to the "default font size" which is not really default but actually the font size of the label.
@mattleibow mattleibow self-assigned this May 16, 2022
@mattleibow mattleibow added this to the 6.0.300-servicing milestone May 16, 2022
@mattleibow mattleibow added platform/android 🤖 area/controls 🎮 control-label t/bug labels May 16, 2022
All:
- set the default parameter values for line height to be -1 as that is what is the default currently
Android:
- don't capture the TextView.Paint as that is always wrong initially since none of the other properties are set
- don't fall back to the Label.LineHeight to the spans as that is always applied - regardless of span values
- pass the Label.CharacterSpacing down to be consistent
- pass the Label.TextDecorations down as well
- for text decorations, use the platform spans
- split a LetterSpacingSpan out of the FontSpan so that they can be individually applied
@mattleibow mattleibow marked this pull request as ready for review May 16, 2022
…ingExtensions.cs

Co-authored-by: campersau <buchholz.bastian@googlemail.com>
this.GetDefaultFontSize();
double.NaN;
Copy link
Contributor Author

@mattleibow mattleibow May 16, 2022

Choose a reason for hiding this comment

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

This is the only "breaking" change, but anyone using this value without it being set by the user is getting a wrong value. I could use 0?

Copy link
Contributor

@jsuarezruiz jsuarezruiz May 17, 2022

Choose a reason for hiding this comment

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

GetDefaultFontSize is not returning a good value without it being set by the user?

Copy link
Contributor Author

@mattleibow mattleibow May 17, 2022

Choose a reason for hiding this comment

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

It is, sort of. The method returns 14 if the span is attached and 0 if not. The issue with these default value functions is that it sets the value as if the user set it.

So the OG issue is that the label font was set to a big size, and then the font remains small because by the time the label is rendered the framework overrides the big with the default of 14.

So if you do:

// in ctor
var firstSpan = new Span();
var firstSize = span.FontSize; // <- 0

var secondSpan = new Span();
var label = new Label {
    FontSIze = 100,
    FormattedText = new FormattedString { firstSpan, secondSpan }
};

// after rendering
var secondSize = secondSpan.FontSize; // <- 14
var firstSizeAgain = firstSpan.FontSIze; // <- 0

In both cases, you did not set a font size on the span, and in the second you actually set a size on the label. And that size is not used.

The NaN is a way to tell the rest of the framework that no size has been set, so keep looking.

@mattleibow mattleibow requested review from Redth and jsuarezruiz May 16, 2022
@mattleibow mattleibow changed the title Fix Android FormattedText Fix Android FormattedText and related platforms May 16, 2022
@mattleibow mattleibow changed the base branch from release/6.0.3xx to main May 16, 2022
Redth
Redth approved these changes May 16, 2022
this.GetDefaultFontSize();
double.NaN;
Copy link
Contributor

@jsuarezruiz jsuarezruiz May 17, 2022

Choose a reason for hiding this comment

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

GetDefaultFontSize is not returning a good value without it being set by the user?

[InlineData(10)]
[InlineData(20)]
[InlineData(30)]
public async Task UpdatingFormattedTextResultsINTheSmaeLayout(double fontSize)
Copy link
Contributor

@jsuarezruiz jsuarezruiz May 17, 2022

Choose a reason for hiding this comment

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

❤️

@mattleibow mattleibow merged commit a51243a into main May 17, 2022
23 checks passed
@mattleibow mattleibow deleted the dev/span-font-size branch May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controls 🎮 control-label platform/android 🤖 t/bug
Projects
None yet
4 participants