Update documentation on TextPainter to note default color differences
#165048
Conversation
There was a problem hiding this comment.
I believe the default is current set by flutter not skparagraph / canvaskit. (It looks like we're not setting default text colors for the canvaskit, instead the default is from here).
the non-web default is from here.
And it looks like the canvaskit backend default is from here.
The wasm implementation here
@mdebbar does it make sense to update one of the defaults so it's consistent between backends?
Probably completely misunderstanding you here, and I cant see where the colour is actually set in the second link, so I don't know which backend you'd suggest changing - but doesn't it make sense for the colour to be black on white on web (regardless of backend) and white on black on native (regardless of backend). And from my testing - although I can't fully remember/be sure - I didn't notice a difference in the text colour whether I was using wasm or not on web - because of course, they should both be black on white to be visible. |
Yeah you're right that the second link isn't where the default is set. The default is actually from canvaskit. I'll update the link. |
| /// > The default text style color is white on non-web platforms and black on | ||
| /// > the web. | ||
| /// > | ||
| /// > The differences in text color may be visible even if the background color |
There was a problem hiding this comment.
I agree with the suggestion that the best practice is to set your own defaults in the root span in general. But
The differences in text color may be visible even if the background color of the canvas is no longer visible.
seems a bit redundant (and initially it was confusing) to me. The text color is supposed to not be affected by the canvas color (unless these's blend mode / composition) so I don't see a good reason to explicitly state that "text color and canvas color do not affect each other".
There was a problem hiding this comment.
Removed. Although
The text color is supposed to not be affected by the canvas color
It kinda is. I know its not actually linked, but they might as well be, since it's black on web because the default canvas color is white (and vice versa).
There was a problem hiding this comment.
Do you mean adding a quick explanation for why the default color is different on web / non-web? That makes sense to me, or as code comment since it's just a background story.
There was a problem hiding this comment.
Yeah, so something like:
The default text style color is white on non-web platforms and black on the web, to provide contrast to the differing default background colors.
But I won't change this now, since it's approved, unless you think that's better.
LongCatIsLooong
left a comment
There was a problem hiding this comment.
LGTM, thank you for contributing!
…es (flutter#165048) This change only changes the documentation of `TextPainter`. Previously, the documentation was incorrect in stating that the default text color was white. It changes depending on platform to be contrasted to the canvas' background color. This is in an "important" box because it affects apps working on native & web, and is relatively easy to miss. It may be worth documenting this elsewhere? Fixes flutter#165047 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…es (flutter#165048) This change only changes the documentation of `TextPainter`. Previously, the documentation was incorrect in stating that the default text color was white. It changes depending on platform to be contrasted to the canvas' background color. This is in an "important" box because it affects apps working on native & web, and is relatively easy to miss. It may be worth documenting this elsewhere? Fixes flutter#165047 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This change only changes the documentation of
TextPainter.Previously, the documentation was incorrect in stating that the default text color was white. It changes depending on platform to be contrasted to the canvas' background color.
This is in an "important" box because it affects apps working on native & web, and is relatively easy to miss.
It may be worth documenting this elsewhere?
Fixes #165047
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.