Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
287 stars 76 forks source link

Action-bar - Arabic text is is cut off in Firefox #3955

Closed zaramatheson closed 2 years ago

zaramatheson commented 2 years ago

Actual Behavior

Arabic text is cut off in Map Viewer in Firefox only. image

Expected Behavior

Text should not be cut off: image

Reproduction Sample

https://www.arcgis.com/apps/mapviewer/index.html?locale=ar

Codepen sample: https://codepen.io/mtb-alan/pen/LYzvMgg

  1. If you don't see the descenders clipping, inspect the Action and look for the .button element.
  2. Set font-family to revert

Reproduction Steps

  1. Open https://www.arcgis.com/apps/mapviewer/index.html?locale=ar in Firefox and expand the toolbars to see the text.

Reproduction Version

@esri/calcite-components@1.0.0-VERSION

Relevant Info

Regression? Last working version: @esri/calcite-components@1.0.0-VERSION

Source

cc @asangma @driskull

github-actions[bot] commented 2 years ago

More information is required to proceed with this issue:

This issue will be automatically closed in five days if the information is not provided. Thanks for your understanding.

asangma commented 2 years ago

@benelan I can take this on. I added a Codepen and some repro steps.

asangma commented 2 years ago

@benelan I made a PR but then realized this issue doesn't have a milestone.

benelan commented 2 years ago

No worries, it happens.

jcfranco commented 2 years ago

@asangma Is this still valid? I saw the associated PR got installed, but didn't see a difference in FF when comparing the codepen between its version (beta.65) and the latest.

zaramatheson commented 2 years ago

@jcfranco @asangma This is still an issue in MV using beta.77

AdelheidF commented 2 years ago

beta.77 was released Feb 14, 2022. This PR got merged Jan 28. So I assume this is still an issue with the latest beta.80.

AdelheidF commented 2 years ago

still an issue with beta.81, Zara checked

jcfranco commented 2 years ago

@asangma Apologies if this is redundant info, but it looks like the clipping is caused by the text-truncation styles applied to the action's text container: https://codepen.io/jcfranco/pen/OJzeYWa. Looking into this, I stumbled upon Capsize, which uses font metadata to generate styles for better text layout. Their FAQ also has some info on truncation and diacritic/accent marks for non-latin alphabets. Something worth looking into.

paulcpederson commented 2 years ago

Per another issue I was added to I think the map viewer is moving to the v2 calcite fonts implementation. This uses Avenir Next World for Arabic, greatly improving the rendering. This should help with this specific case where it's cutting off the text.

I will say that this line-height is pretty tight, though. It may make sense to bump it regardless (could lower y-padding to compensate).

Using something like capsize is going to be hard. We have lots of languages using system defaults (all different per language), then some faces still using non-avenir web fonts, plus older calcite-fonts and calcite web implementations which also use different font files with different metrics. There is no way to know for the system fonts which font a user has on their machine. This is a problem capsize has acknowledged https://github.com/seek-oss/capsize/issues/23#issuecomment-673743474

jcfranco commented 2 years ago

Awesome. It looks like v2 fonts will work w/o us doing anything. Is this correct?

I understand the complexity of covering all cases w/ Capsize. I brought it up to see if it would at least help cover our bases with the calcite font-families.

paulcpederson commented 2 years ago

Awesome. It looks like v2 fonts will work w/o us doing anything. Is this correct?

Correct, basically avenir next world was redrawn with new metrics and characters for more languages, so the arabic descenders/joiners fit inside the glyph better.

jcfranco commented 2 years ago

I will say that this line-height is pretty tight, though. It may make sense to bump it regardless (could lower y-padding to compensate).

@asangma Can we tweak the line-height as suggested above and rely on apps using Avenir Next for the rest?

github-actions[bot] commented 2 years ago

Installed and assigned for verification.

benelan commented 2 years ago

verified an increased line-height on next