1908003 - [Menu Redesign] Fix the line height of the text on the menu item and header.

Status: RESOLVED FIXED

(Firefox for Android :: Toolbar, defect, P3)

Description

[Aarjav [:aarjav]]

Steps to reproduce

1. Open the menu

Expected behavior

• Check the line height of text styles from design systems for menu items and the header
  Subtitle 1 - Primary Text - https://www.figma.com/design/pEyGeE4KV5ytYHeXMfLcEr/Mobile-Styles?node-id=5486-19280&t=fLvw7b6d9QkGSLFF-4
  Link
Body 2 - Secondary Text - https://www.figma.com/design/pEyGeE4KV5ytYHeXMfLcEr/Mobile-Styles?node-id=5486-19283&t=fLvw7b6d9QkGSLFF-4

Actual behavior

• The line height looks short in height compared to the designs.

Device information

• Firefox version:
• Android device model:
• Android OS version:

Any additional information?

Bug shown in figma - https://www.figma.com/design/RFz9fYtotQCQuinwcZujZt/Menu-Redesign?node-id=16634-319553&t=AkMU1d3vNoAMLjzX-4.

Comment 1

[Chris Peterson [:cpeterson]]

Aarjav says fixing this line height bug doesn't need to block our Nightly milestone.

gl says our code uses sp sizing, but design system uses dp sizing. This is a code bug related to Compose text styling and not something the design system team can fix.

Comment 2

[Chris Peterson [:cpeterson]]

Does the line height use sp or dp sizing?

Comment 3

[Iorga Gabriel]

Line Height is in sp.:
subtitle1 = TextStyle(
fontSize = 16.sp,
fontWeight = FontWeight.W400,
letterSpacing = 0.15.sp,
lineHeight = 24.sp,
),
body2 = TextStyle(
fontSize = 14.sp,
fontWeight = FontWeight.W400,
letterSpacing = 0.25.sp,
lineHeight = 20.sp,
),

Comment 4

[Iorga Gabriel]

It depends on font size from phone settings. If the font size bar is in the middle, like in the first print screen, 24.sp will be 26.285715.dp. If the font size bar is like in the second photo, 24.sp will be 24.dp.

Comment 5

[Iorga Gabriel]

Attachment: first.png

Comment 6

[Iorga Gabriel]

Attachment: second.png

Comment 7

[Iorga Gabriel]

Attachment: Screenshot_20241022-173545.png

Comment 8

[Petru-Mugurel Lingurar [:petru]]

(from to Aarjav [:aarjav] from comment #0)

Expected behavior

• Check the line height of text styles from design systems for menu items and the header
  Subtitle 1 - Primary Text - https://www.figma.com/design/pEyGeE4KV5ytYHeXMfLcEr/Mobile-Styles?node-id=5486-19280&t=fLvw7b6d9QkGSLFF-4
  Link
Body 2 - Secondary Text - https://www.figma.com/design/pEyGeE4KV5ytYHeXMfLcEr/Mobile-Styles?node-id=5486-19283&t=fLvw7b6d9QkGSLFF-4

The Figma specs for body_2 are:

• font size: 14
• line height: 20
• font weight: 400

which are mapped exactly as such in the app as body2 and then used for the menu item description

---

The Figma specs for subtitle_1 are:

• font size: 18
• line height: 24
• font weight: 500

which are mapped differently in our app as subtitle1 with the following properties:

• font size: 16
• line height: 24
• font weight: 400

and then used for the menu item label here.

@Aarjav I see though a comment in Figma saying

These are Figma only tokens and are not reflective of the current codebase

so maybe that's okay?
Note also that these same styles are used for the new compose based bookmarks (can be enabled in Secret Settings) - if we change the subtitle1 token inside the app more scenarios would be impacted, not just the menu.

There is a separate issue in that tokens from the app are not applied perfectly (we set the right value (for example for lineHeight in code but it is not respected), I need to investigate that more but in the meantime wanted to double check if we can have slightly different values for the font size and weight for the subtitle1 token.

Comment 9

[Aarjav [:aarjav]]

Hey Petru, I am not sure where I have mentioned that last comment, but the intention is to say that it hasn't been correctly reflected in the implemented feature. But eventually, the implementation needs to match the designs.

And regarding bookmarks, it would be okay if they were also implemented there as it would be correct. It just needs to follow the details mentioned in the design system file.

Bookmarks and Menu uses the same design tokens.

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Seems like there is an issue in the Compose framework where TextStyles are not applied correctly - this would affect the entire application (and of others also) where we make heavy use of TextStyles and the fonts may be smaller than expected.
Opened an issue @Compose https://issuetracker.google.com/issues/382453600 .

In the meantime I'll look into slight updates for our usecase here.

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: Menu Fonts Comparison.jpg

Added here a comparison of possible outcomes.
Using the Figma specs for the subtitle_1 token results in quite a big UI difference so I recommend just increasing the line height for now.

@Aarjav any recommendations for what should the result be in this conditions?

Comment 12

[Aarjav [:aarjav]]

The second image seems fine, but I can see that the text + icon is losing horizontal alignment. Would you mind sharing an apk and I can share the exact comparison of that implementation and designs? This will help us get an understanding of how this new slightly better implementation differs from the designs.

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Attachment: Updated Menu Fonts.jpg

Adjusted the implementation for the second approach to avoid the issue Aarjav pointed.
Here is an apk to testdrive the result.

Comment 14

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1908003 - part 1 - includeFontPadding for list items r=#android-reviewers

Workaround for compose TextStyle not applying correctly.
See https://issuetracker.google.com/issues/382453600.

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1908003 - part 2 - includeFontPadding for the menu header r=#android-reviewers

This change helps complete having the needed line height for everything
in the new menu.

XML uses font padding by default.
Compose not.
See https://medium.com/androiddevelopers/fixing-font-padding-in-compose-text-768cd232425b
for more details.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1f35d6f71f0
part 1 - includeFontPadding for list items r=android-reviewers,007,tchoh,sfamisa,harrisono

Comment 17

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/a1f35d6f71f0

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4205e9fbce67
part 2 - includeFontPadding for the menu header r=android-reviewers,sfamisa

Comment 19

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/4205e9fbce67