Skip to content

fix(actionbutton): prevent diacritic clipping in thai #3256

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

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented Oct 15, 2024

Description

Recently, another bug was captured in relation to the diacritics and tone marks getting cut off in the action button when the text is Thai: https://jira.corp.adobe.com/browse/CCEX-131368.

This PR attempts to fix that bug by adding padding to the action button's label, allowing for the diacritics to overflow into the padding space, and no longer get cut off. A similar solution was implemented recently for picker (#2914)

If we decide that adding padding is causing VRT diffs or other unexpected issues we want to avoid, we could explore using line height as well (a PR for the same picker issue mentioned above was closed in favor of the padding fix instead: #2913)

The 2 commits related to adding and then removing the Thai text were meant to be temporary and serve as validation purposes. These could be removed/dropped before merging (or we could just clean up the squash commit message)

BEFORE 🚫

action button
Screenshot 2024-10-15 at 12 15 58 PM

action group
Screenshot 2024-10-18 at 1 41 39 PM

AFTER ✅

action button
Screenshot 2024-10-15 at 12 16 46 PM

action group
Screenshot 2024-10-18 at 1 43 31 PM

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

I tested the deploy preview in AssistivLabs, for WHCM in Chrome, Edge, Firefox.

Validation steps

  • Pull down the branch to run locally or visit the deploy preview
  • Visit the action button story page
  • Change the action button text in the controls to Thai: "ล้างทั้งหมด"
    • Alternatively, if you're viewing the branch locally, you can checkout the second commit (git checkout 35c9809de3fd760ff8f3848f2c031a4164d849a0) to see the Thai text
  • Verify that the tone marks and diacritics are no longer getting cut off visually (especially over the fourth character)
  • When viewing the action button testing preview, a new Internationalization test case appears, with the Thai characters (with no clipping of tone marks)
  • Visit the action group testing preview
  • Verify the action buttons in the vertical-justified action group are now the same height (32px) as all other buttons (this is an expected VRT)

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • ✨ This pull request is ready to merge. ✨

@marissahuysentruyt marissahuysentruyt added size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. s1 labels Oct 15, 2024
Copy link

changeset-bot bot commented Oct 15, 2024

🦋 Changeset detected

Latest commit: 68da4c2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@spectrum-css/actiongroup Patch
@spectrum-css/actionbutton Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Oct 15, 2024

🚀 Deployed on https://pr-3256--spectrum-css.netlify.app

Copy link
Contributor

github-actions bot commented Oct 15, 2024

File metrics

Summary

Total size: 4.30 MB*
Total change (Δ): 🔴 ⬆ 0.17 KB (0.00%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
actionbutton 40.79 KB 🔴 ⬆ 0.05 KB
actiongroup 8.36 KB 🔴 ⬆ 0.01 KB

Details

actionbutton

Filename Head Compared to base
index-base.css 29.72 KB 🔴 ⬆ 0.05 KB (0.17%)
index-theme.css 11.68 KB -
index-vars.css 40.79 KB 🔴 ⬆ 0.05 KB (0.12%)
index.css 40.79 KB 🔴 ⬆ 0.05 KB (0.12%)
themes/express.css 8.96 KB -
themes/spectrum.css 9.15 KB -

actiongroup

Filename Head Compared to base
index-base.css 7.88 KB 🔴 ⬆ 0.01 KB (0.06%)
index-theme.css 1.09 KB -
index-vars.css 8.36 KB 🔴 ⬆ 0.01 KB (0.06%)
index.css 8.36 KB 🔴 ⬆ 0.01 KB (0.06%)
themes/express.css 0.93 KB -
themes/spectrum.css 0.82 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@pfulton pfulton added the skip_vrt Add to a PR to skip running VRT (but still pass the action) label Oct 15, 2024
@5t3ph
Copy link
Contributor

5t3ph commented Oct 15, 2024

This addresses the core issue of preventing the clipping, although it's very tight for XS per current design (non-blocking issue).

We may want to file a suggestion for design to slightly increase the padding-block space (min button height) for XS by a couple pixels so that the diacritic marks have a bit more breathing room.

Current comparison across sizes:
image

@marissahuysentruyt
Copy link
Collaborator Author

This addresses the core issue of preventing the clipping, although it's very tight for XS per current design (non-blocking issue).

Oh yes, great catch. In comparison, what if we went with a line-height fix instead? It looks like maybe it's a little better. A solution with line-height was another option in some recent fixes for picker (but I can't remember why we didn't end up going with it). Maybe that's a better solution here- what do you think? It also looks like maybe that span no longer overflows with line-height.

Padding:
Screenshot 2024-10-15 at 12 44 17 PM
Screenshot 2024-10-15 at 12 48 13 PM

Line-height:
Screenshot 2024-10-15 at 12 43 51 PM
Screenshot 2024-10-15 at 12 48 02 PM

@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review October 15, 2024 16:54
@5t3ph
Copy link
Contributor

5t3ph commented Oct 15, 2024

@marissahuysentruyt since we do not allow line-wrapping, line-height does alleviate the issue in a slightly more "correct" way since it changes the way the text-box is evaluated by increasing the room for the characters.

However, if we allowed line-wrapping, then the larger line-height, especially pixel-based, would not be ideal, so padding may be a preferred solution in other cases.

For future reference back on this PR, it's probably also worth noting that the clipping is only happening due to overflow: hidden which is a requirement for text-overflow: ellipsis

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from 08c6aed to a03d304 Compare October 15, 2024 19:11
@castastrophe
Copy link
Collaborator

Can we add a test case to the actionbutton with an example of this issue so we can prevent regressions in future?

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from a03d304 to 4b052ab Compare October 17, 2024 16:15
@castastrophe castastrophe added run_vrt For use on PRs looking to kick off VRT and removed skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Oct 17, 2024
@castastrophe castastrophe force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from 4b052ab to 5eacbd0 Compare October 17, 2024 16:49
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from 5eacbd0 to 81ebf2f Compare October 18, 2024 19:00
Comment on lines 61 to 65
&.spectrum-ActionGroup--justified .spectrum-ActionGroup-item {
/* flex: unset removes the flex: 1 set on `.spectrum-ActionGroup--justified .spectrum-ActionGroup-item`,
in order to keep the action buttons from shrinking */
flex: unset;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@5t3ph I know you suggested that maybe we should use flex-grow: 1 as opposed to the flex: 1 (which would also set flex-shrink which was probably causing our squish-ness), however, I was struggling to not undo the horizontal justified behavior.

I tried flex-grow: 1 in a couple of places:

.spectrum-ActionGroup--justified .spectrum-ActionGroup-item {
	flex-grow: 1;
}

// OR
.spectrum-ActionGroup--vertical {
	gap: var(--mod-actiongroup-vertical-spacing-regular, var(--spectrum-actiongroup-vertical-spacing-regular));
	display: inline-flex;
	flex-direction: column;

	&.spectrum-ActionGroup--justified .spectrum-ActionGroup-item {
		flex-grow: 1;
	}
}

The first selector fixed the vertical justified button heights (from 36px to the correct 32px), but then unjustified the horizontal justified action group:
Screenshot 2024-10-18 at 3 11 53 PM

The second selector....didn't do anything? The action buttons were still the incorrect 36px, (and the icon-only aren't tall enough).
Screenshot 2024-10-18 at 3 28 58 PM

With the new selector here with flex: unset, the horizontal justified remains the same, AND the vertical justified now is not squished and the proper 32px.

Screenshot 2024-10-18 at 3 29 57 PM

Am I missing anything with flex-grow? I know you mentioned min-block-size might be redundant, but would you rather that than the flex: unset? I haven't tried anything with that yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After our pairing, we realized I just didn't understand what should be happening in a justified actiongroup! flex-grow: 1 is now up: 85656a0

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from 81ebf2f to 85656a0 Compare October 18, 2024 20:51
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from 85656a0 to b06ebe0 Compare October 21, 2024 13:34
@castastrophe
Copy link
Collaborator

Passed VRT: https://www.chromatic.com/build?appId=64762974a45b8bc5ca1705a2&number=3495

@castastrophe castastrophe added skip_vrt Add to a PR to skip running VRT (but still pass the action) and removed run_vrt For use on PRs looking to kick off VRT labels Oct 21, 2024
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from b06ebe0 to e67ad79 Compare October 22, 2024 17:38
@marissahuysentruyt marissahuysentruyt self-assigned this Oct 22, 2024
@castastrophe castastrophe force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from e67ad79 to 57a2afa Compare October 22, 2024 21:06
@castastrophe castastrophe enabled auto-merge (squash) October 22, 2024 21:07
- add line-height to .spectrum-ActionButton-label
- add changeset
- add test case for Thai language
- for justified action groups, specify that the desired effect is for
them to grow to fill the available space (as opposed to allow allowing
flex-shrink)
- add changeset
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-button-clipping-in-thai branch from 57a2afa to 68da4c2 Compare October 23, 2024 17:12
@castastrophe castastrophe merged commit b84b93e into main Oct 23, 2024
12 checks passed
@castastrophe castastrophe deleted the marissahuysentruyt/fix-button-clipping-in-thai branch October 23, 2024 17:22
This was referenced Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants