-
-
Notifications
You must be signed in to change notification settings - Fork 891
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
); | ||
} | ||
|
||
/// [parseHTML] converts a string of HTML to a DOM document using the dart `html` library. | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The underlying problem we have here is that By defaulting to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -721,6 +731,7 @@ class StyledText extends StatelessWidget { | |
style: style.generateTextStyle(), | ||
textAlign: style.textAlign, | ||
textDirection: style.direction, | ||
textScaleFactor: 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.
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](https://user-images.githubusercontent.com/4116552/85572907-b1c1a000-b635-11ea-9a1d-2e7ef9bc3b60.png)
![Simulator Screen Shot - iPhone SE (1st generation) - 2020-06-24 at 16 13 07](https://user-images.githubusercontent.com/4116552/85572915-b38b6380-b635-11ea-8b7c-5814d39e2abc.png)
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).