Skip to content

content: Use 17 for base font size, and fix code/mentions/times in headings #540

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

Closed
wants to merge 5 commits into from

Conversation

chrisbobbe
Copy link
Collaborator

Before After
image image
image image
image image
image image

Fixes: #512
Fixes: #538

Inline code, user mentions, and global times are failing to match
the font size of an outer span when the outer span's font size is
different. This is zulip#538.

Pass that style to the code responsible for rendering those
elements, without actually using it. We'll do that next.
In the previous commit, we brought the surrounding span's TextStyle
conveniently near to the code that renders these elements.

Here, we use that TextStyle, fixing zulip#538.

Fixes: zulip#538
…citly

kBaseFontSize is currently 14.

Before this, the text was sized by
Theme.of(context).textTheme.bodyMedium.fontSize, thanks to an
AnimatedDefaultTextStyle in the Material widget. That value is 14
when the app's localization state is set to render text as
"English-like" (see [Typography]), which at this point I think it
always is.

We want to increase the content font size soon, for zulip#512, and for
that it'll be helpful if everything uses kBaseFontSize or some
multiple of it. From scrolling through a bunch of content with
kBaseFontSize increased dramatically, I think these were the only
pieces of content that weren't doing so.
@chrisbobbe chrisbobbe requested a review from gnprice February 29, 2024 02:05
@gnprice
Copy link
Member

gnprice commented Feb 29, 2024

Thanks for cleaning these up! The code changes all look good to me. A couple of tests are failing, though.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Feb 29, 2024

Those tests are helpful indeed! Hmm. So the problem starts here:

content: Show code, mentions, and times properly even in headings, etc.

The problem is that we're trying to apply the 0.825 fontSize multiplier for inline code in places that don't actually have a fontSize value to apply the multiplier to, e.g. (by the end of the branch), 17 for paragraphs and 23.8 for h1 headings. That will happen when the paragraph/h1/etc. isn't the thing directly surrounding the code span, such as when there's a strong (bold) or emphasis (italic) span around the code span.

The test failure is on this content:

<p><a href="https://a/">two <strong><em><code>words</code></em></strong></a></p>

Here's that content replicated in a CZO message with this Markdown:

[two ***`words`***](https://a/)

The error is:

The following assertion was thrown building InlineContent(dirty, dependencies: [MediaQuery]):
'package:flutter/src/painting/text_style.dart': Failed assertion: line 990 pos 12: 'fontSize != null || (fontSizeFactor == 1.0 && fontSizeDelta == 0.0)': is not true.

The code span is expecting to find a font size on the TextStyle of the surrounding span. That span is an emphasis (italic) span with just const TextStyle(fontStyle: FontStyle.italic)), so no fontSize.

@chrisbobbe
Copy link
Collaborator Author

To fix, we could maybe:

  • For every span that might sit between a code span and a paragraph/h1/etc., make sure the span merges into its own style the font size of the span that surrounds it. This seems error-prone.
  • In paragraphs/h1s/etc., use a DefaultTextStyle that provides the appropriate font size, and then the code-span code can consume it using DefaultTextStyle.of.

@gnprice
Copy link
Member

gnprice commented Feb 29, 2024

I like the second direction, and I think we can simplify it: DefaultTextStyle is a way of specifying a style as an inherited widget, to be found from throughout that widget's subtree. But paragraphs and headings etc. are leaves in the widget tree — the further recursion that happens in the ContentNode tree is all producing spans for the one widget, not additional descendant widgets.

So instead of DefaultTextStyle, I think we can pass a style to _InlineContentBuilder as the default style, and then the code-span code can consume that and do the same thing it would do with the result of DefaultTextStyle.of.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Feb 29, 2024

Interesting; thank you! I'll go for that.

@chrisbobbe
Copy link
Collaborator Author

So instead of DefaultTextStyle, I think we can pass a style to _InlineContentBuilder as the default style

Doesn't _InlineContentBuilder already have this, kind of, and call it widget.style? (Except that InlineContent.style is optional, but could easily be made required?)

@gnprice
Copy link
Member

gnprice commented Mar 1, 2024

Yeah, that seems right. So then that approach comes down to taking widget.style and drawing from it in more places, in the same way you might have done from DefaultTextStyle.of.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Mar 1, 2024

Since #541, we're now using 17 for the base font size.

I plan to close this and open a new PR (-> #544) aimed at #538, with fresh screenshots (that will show the font-size 17 in the "before" column).

@chrisbobbe chrisbobbe closed this Mar 1, 2024
@chrisbobbe chrisbobbe deleted the pr-font-size-17 branch March 1, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

content: Inline code uses paragraph font size, even in headings (h1, etc.) Increase content font size
2 participants