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.

Sign up for GitHub

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

Jump to bottom

Ensures that text is scaled properly using accessibility fonts. #333

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Ensures that text is scaled properly using accessibility fonts. #333

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion lib/html_parser.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,15 @@ class HtmlParser extends StatelessWidget {
cleanedTree,
);

return StyledText(textSpan: parsedTree, style: cleanedTree.style);
// This is the final scaling that assumes any other StyledText instances are
// using textScaleFactor = 1.0 (which is the default). This ensures the correct
// scaling is used, but relies on https://github.com/flutter/flutter/pull/59711
// to wrap everything when larger accessibility fonts are used.
return StyledText(
textSpan: parsedTree,
style: cleanedTree.style,
textScaleFactor: MediaQuery.of(context).textScaleFactor,
Copy link

@lukeurban lukeurban Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This should be added as a parameter, It's still breaking accessibility functions. When textScaleFactor != 1.0. But I've tried to do it, still was breaking view.
Simulator Screen Shot - iPhone SE (1st generation) - 2020-06-24 at 16 07 39
Simulator Screen Shot - iPhone SE (1st generation) - 2020-06-24 at 16 13 07

Copy link

Choose a reason for hiding this comment

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

I have a branch with a working solution. This does everything you did plus, what's not working for me. Don't have permission to push it tho xD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that being able to pass in textScaleFactor would be a useful enhancement. I'll push up a change containing that today.

When you say that it is "still breaking accessibility functions", I'm not sure what you mean. What is broken?

Copy link

Choose a reason for hiding this comment

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

As screenshots are showing when I am allowing the user to define textScaleFactor via accessibility functions on his phone (I've made font smaller, not bigger). Everything was shifted to the left and smaller (mainly width). (this will be later fixed in flutter/flutter#59711) I've modified my local flutter code to verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I see now... so, I imagine the width problem will be resolved with the Flutter PR you mentioned... I'm just trying to work out if you're saying there needs to be additional changes beyond a) the changes in this PR and b) the Flutter PR.

Copy link

Choose a reason for hiding this comment

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

Any updates on this? I just tested this branch and the content no longer overflows the screen for me, but still overflows its container to the right. (On both flutter channels beta and master).

);
}

/// [parseHTML] converts a string of HTML to a DOM document using the dart `html` library.
Expand Down Expand Up @@ -703,10 +711,12 @@ class ContainerSpan extends StatelessWidget {
class StyledText extends StatelessWidget {
final InlineSpan textSpan;
final Style style;
final double textScaleFactor;

const StyledText({
this.textSpan,
this.style,
this.textScaleFactor = 1.0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to default this and make it a property of StyledText? Are we exposing it somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underlying problem we have here is that RichText (which is what Rich.text() ultimately creates) will take the textScaleFactor that is passed to it and will rescale based on that. Due to the way that we're constructing the widget tree, we have nested RichText objects so if textScaleFactor is anything other than 1.0, then we end up with the scaling getting applied multiple times.

By defaulting to 1.0, we allow StyledText objects to be created all over the place with the correct internal scaling value. And then, on line 74, just before we return the final widget tree in the build() function we create one more outermost StyledText but this time we pass in the real scaling factor so that everything underneath gets scaled correctly. Hope that makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense. When I asked I was just doing general code review since I haven't had time to take a look at the specific issue or even run the PR.

After running it, reading what you had to say and actually reading the code, everything looks okay, I don't see any problems with this, I will merge it.

});

@override
Expand All @@ -721,6 +731,7 @@ class StyledText extends StatelessWidget {
style: style.generateTextStyle(),
textAlign: style.textAlign,
textDirection: style.direction,
textScaleFactor: textScaleFactor,
),
);
}
Expand Down