-
-
Notifications
You must be signed in to change notification settings - Fork 889
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
Ensures that text is scaled properly using accessibility fonts. #333
Ensures that text is scaled properly using accessibility fonts. #333
Conversation
return StyledText( | ||
textSpan: parsedTree, | ||
style: cleanedTree.style, | ||
textScaleFactor: MediaQuery.of(context).textScaleFactor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small gripe, but I'm sure you have a good answer for me. Once this is cleared up we can merge it
|
||
const StyledText({ | ||
this.textSpan, | ||
this.style, | ||
this.textScaleFactor = 1.0, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Hopefully that text-wrapping PR lands soon so that this library can become fully operational.
So which flutter version and flutter_html version should I use to have this work properly with font enlargement or should I still wait? I'm still on flutter 1.12.13 and flutter_html-pre due to this bug, since on that setup everytnig works as it should in my app. |
There are no stable releases yet of Flutter that will not have this issue, if you use flutter_html pre-1.0 or later. (You could use the Flutter master channel if you want, but you will be on bleeding edge for every change. If so, you also need to use master channel of flutter_html.) |
I recommend use beta and change this one line every time until they change it xD works |
Any update on this or ETA? |
FWIW, the fix in this PR addresses the scaling problem that this library suffered from. However, there is a problem in the Futter SDK (that is being addressed in flutter/flutter#59711 and flutter/flutter#60021) that means the wrapping isn't working properly. Right now, it is still only available in the master channel, but I expect it to be in the next dev channel in a week or so. If you want that fix, you'll have to use a later (presumably less stable) channel. |
On the Master Branch and it is not fixed. Checking lib/html_parser.dart the line does not appear to be updated. |
Ah, I think the problem is even though this PR has been merged, there hasn't been a new release (eg
|
Thanks, that gives a hostkey error but that git only seems to have v1.0.0 still anyway |
The correct way is:
|
Thanks,
Which is not there yet :( |
This resolves #308, but as noted in the comment does rely on flutter/flutter#59711. That fix is currently in the
master
channel, but I'm not sure when it will find its way out into thestable
release.