content: Put code-block styles in ContentTheme; use weightVariableTextStyle for bold spans
#727
Conversation
chrisbobbe
added
a-content
chrisbobbe
force-pushed
the
pr-code-block-styles
branch
from
228dd7e
to
61fe10a
Compare
lib/widgets/content.dart
Outdated
| InlineSpan _buildNodes(List<CodeBlockSpanNode> nodes) { | ||
| InlineSpan _buildNodes(List<CodeBlockSpanNode> nodes, {required CodeBlockTextStyles styles}) { |
There was a problem hiding this comment.
Hmm, this helper and the _buildNode below it are a bit silly — they each have just one call site, so they're not doing much. That's largely harmless as is, but makes it a pain to pass down additional data through them, as here.
So a good prep commit would be to inline both these helpers into build. Then this can just be a local styles.
| required TextStyle vi, | ||
| required TextStyle il, | ||
| }) : | ||
| _hll = hll, | ||
| _c = c, |
There was a problem hiding this comment.
Oof, lotta boilerplate.
But I think this is the structure we need in order to support dark mode (and particularly to support nicely lerping in and out of it), so that's just how it'll be. Definitely glad we have this off in its own file.
| final _kCodeBlockStyleGh = TextStyle(color: const HSLColor.fromAHSL(1, 240, 1, 0.25).toColor(), fontWeight: FontWeight.bold); | ||
| // .gh { color: hsl(240deg 100% 25%); font-weight: bold; } | ||
| gh: TextStyle(color: const HSLColor.fromAHSL(1, 240, 1, 0.25).toColor()).merge(bold), |
There was a problem hiding this comment.
Hmm I see. The FontWeight.bold was getting ignored because of the wght variation, which leaked through from the ambient TextStyle (which in turn came from the ContentTheme). Is that right?
It'd be good to add a few words to the commit message about the visible effect of this. I wasn't sure what to expect before I opened up the screenshots at full size and compared them.
There was a problem hiding this comment.
In particular:
And while we're add it, add some tests that spot-check the weight of
a non-bold span and a bold span.
Those tests aren't just a by-the-way — they're regression tests for the bug this fixes 🙂
There was a problem hiding this comment.
Hmm I see. The
FontWeight.boldwas getting ignored because of thewghtvariation, which leaked through from the ambient TextStyle (which in turn came from the ContentTheme). Is that right?
No, I don't think that's quite right. The "Before" screenshot already shows the text being bolder than surrounding text, just not in the way we want it to be. I think this was caused by the FontWeight.bold here and:
For glyphs that render in a variable-weight font, we want to specify a wght value, so that they appear as the font designer intended. Just setting TextStyle.fontWeight by itself doesn't do that—not until flutter/flutter#148026 anyway—and our way of setting wght is to use weightVariableTextStyle. These glyphs did have a wght value before this change, but it was wrong. It should be 700, but it was 400, from the ambient TextStyle from ContentTheme.
There was a problem hiding this comment.
Yeah, I guess "ignored" isn't right. But as you say, it (the FontWeight.bold) wasn't having the desired effect.
lib/widgets/code_block.dart
Outdated
| /// Used to render syntax highlighting. | ||
| // Span styles adapted from: | ||
| // https://github.com/zulip/zulip/blob/213387249e7ba7772084411b22d8cef64b135dd0/web/styles/pygments.css | ||
| TextStyle? span(CodeBlockSpanType type) { |
There was a problem hiding this comment.
How about:
| TextStyle? span(CodeBlockSpanType type) { | |
| TextStyle? forSpan(CodeBlockSpanType type) { |
The name just "span" reads to me like it's meant to return a span.
lib/widgets/code_block.dart
Outdated
|
|
||
| final TextStyle plain; | ||
|
|
||
| final TextStyle _hll; |
There was a problem hiding this comment.
plain here has a different role from all the other styles, in that it's meant to be a baseline that each of them gets applied on top of.
So that asymmetry would be good to make explicit, perhaps in dartdoc on plain. In particular that helps explain why in the last commit, an explicit weight gets added to plain but there's no need for it on the rest.
chrisbobbe
force-pushed
the
pr-code-block-styles
branch
from
61fe10a
to
2651cc1
Compare
|
Thanks for the review! Revision pushed. |
|
Thanks for the revision! This all looks good, but it seems to have a conflict with #728, which I just merged. So please go ahead and rebase past that, and then merge. |
With the code-block span styles all bundled together in a new CodeBlockTextStyles class, it'll be convenient to make separate `light` and `dark` constructors toward zulip#95 dark theme. Next, we'll pass a BuildContext to the CodeBlockTextStyles constructor, in order to use weightVariableTextStyle for the spans that are supposed to be bold. (The code-block font, Source Code Pro, is variable-weight.) This would have been possible before the current refactor, of course, but this way the spans don't have to repeat the weightVariableTextStyle computations on-demand. They can instead just look up the result from this central source. Related: zulip#95
Bold spans still don't set `wght` to 700; we'll fix that and test for it next.
Now, as the added test confirms, bold spans are getting a `wght` value of 700, instead of 400 from the ambient style. The FontWeight.bold seems to have already been causing the spans to render bolder than surrounding text, but in a way that doesn't match what we get for Source Code Pro with `wght` 700. I think this is because of flutter/flutter#136779 .
I *think* it's not possible for a code block to appear in a bold context. In particular I haven't found a way to put one inside a spoiler header. Anyway, just in case it does end up being possible somehow, might as well clobber the bold with normal weight, to make sure the code block's bold spans remain distinct from non-bold spans. It's convenient (and cheap) to do that here in the CodeBlockTextStyles constructor.
chrisbobbe
force-pushed
the
pr-code-block-styles
branch
from
2651cc1
to
4a2e4b5
Compare
|
Done; thanks for the review! |
Related: #95