Convert includes/ and base/ to CSS logical properties - #1143
Draft
stephaniehobson wants to merge 1 commit into
Draft
stephaniehobson wants to merge 1 commit into
stephaniehobson wants to merge 1 commit into
Conversation
stephaniehobson
force-pushed
the
v23/logical-base
branch
from
September 11, 2026 19:01
ef95dc1 to
93a1cc2
Compare
First pass of the logical-properties migration -- establishes the conversion pattern the rest of the workstream follows, and removes 23 of the 127 remaining @include bidi() calls. includes/mixins/_utils.scss (0 bidi, 2 physical: text-align kept physical -- see note below; inset:0 shorthand for a symmetric all-sides absolute-position reset) includes/mixins/_details.scss (2 bidi -> padding-inline-end, inset-inline-end) includes/forms/index.scss (1 bidi -> inset-inline-start; this was the form-msg-pointer mixed-decls warning noted as deferred back in the mixed-decls PR -- confirmed fixed) base/elements/_lists.scss (8 bidi -> margin-inline-start, all identical margin-left/right swap pattern; also collapsed two margin-left+margin-right !important pairs to margin-inline !important) base/elements/_forms.scss (2 bidi -> padding-inline-end; the select rule's background-position kept physical with an explicit [dir='rtl'] override -- background- position has no standard logical keyword syntax safe for this matrix -- but its accompanying padding tuple did convert cleanly to padding-block + padding-inline) base/elements/_links.scss (2 bidi -> margin-inline shorthand) base/elements/_tables.scss (2 bidi -> text-align: start; also removed a redundant plain text-align: left that duplicated the LTR half of the old bidi call) base/elements/_quotes.scss (1 bidi -> border-block-width + border-inline-*-width, since there's no single logical shorthand for all four border-width sides at once) base/elements/_details.scss (1 bidi -> padding-inline, symmetric zero on both sides) base/utilities/_rich-text.scss (4 bidi -> margin-inline-start, same swap pattern as _lists.scss) Left _utils.scss's image-replaced mixin's text-align: left alone -- it also hardcodes direction: ltr, so it's intentionally fixed regardless of page direction (an old image-replacement technique for hiding text completely), not a case of missing RTL support. Using text-align: start there would be misleading, implying adaptiveness that was deliberately designed out. Caught a real bug before it shipped: moving @include forms.form-input to the end of the select rule would have fully silenced its last 2 mixed-decls warnings, but form-input() also sets a plain padding: $field-padding shorthand -- moving it after my new padding-inline/padding-block would let that shorthand win the cascade and silently remove the space reserved for the dropdown caret icon. Reverted to form-input's original early position; the 2 warnings stay deferred (same conclusion the mixed-decls PR reached), but the actual rendered padding is unaffected. Found a second, related bug via a real visual regression report (label.mzp-u-inline in _forms.scss): the original bidi() call used a full 4-value padding shorthand (0 $spacing-sm 0 0), which explicitly zeroed padding-bottom -- overriding the $label-v-spacing bottom padding that forms.field-label() sets unconditionally on every <label>. My first-pass conversion to padding-inline-end alone dropped that override, so the label picked the mixin's padding-bottom back up. Fixed by adding padding-block-end: 0 alongside padding-inline-end. Systematically re-audited every other bare "padding"/"margin"-shorthand bidi() conversion in this file for the same class of bug (a dropped side silently falls back to some *other* rule's non-zero value rather than the CSS-initial 0) -- the border-width conversion in _quotes.scss already covered all four sides explicitly and wasn't affected. Every conversion verified against a before/after sass compile diff of both protocol.scss and protocol-components.scss -- confirmed each logical property produces byte-identical LTR and RTL output to the physical-plus-[dir=rtl]-override pair it replaced. Stacked on v23/focus-visible-forms. Part of #1084. Verified: npm run lint, npm test (47 specs, Firefox + Chrome), npm run build-docs (523 items, no errors), npm run build-package, and the before/after compile diff described above all pass.
stephaniehobson
force-pushed
the
v23/logical-base
branch
from
September 11, 2026 20:29
93a1cc2 to
c3ea9c4
Compare
stephaniehobson
changed the base branch from
v23/focus-visible-forms
to
main
September 11, 2026 20:29
stephaniehobson
added this pull request to stack #1149
September 11, 2026 20:33
Contributor
Author
|
🙅♀️ Not ready for review - I found some RTL regressions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
First pass of the logical-properties migration (goal 3/4 of #1084) — establishes the conversion pattern the rest of the workstream follows, and removes 23 of the 127 remaining
@include bidi()calls.Converts
includes/mixins/_utils.scss,includes/mixins/_details.scss,includes/forms/index.scss, andbase/elements/_forms.scss,_lists.scss,_links.scss,_tables.scss,_quotes.scss,_details.scss, andbase/utilities/_rich-text.scss.Every conversion verified against a before/after
sasscompile diff — confirmed each logical property produces byte-identical LTR and RTL output to the physical-plus-[dir=rtl]-override pair it replaced.A couple of judgment calls worth a look:
select'sbackground-positionstayed physical with an explicit[dir='rtl']override — there's no standard logical keyword syntax forbackground-positionsafe across this matrix. Its accompanying padding tuple did convert cleanly topadding-block/padding-inline.Caught a real near-bug: moving
@include forms.form-inputto the end of theselectrule would have silenced its last 2mixed-declswarnings, butform-input()also sets a plainpaddingshorthand that would then win the cascade over my new logical padding, silently breaking the caret icon's clearance space. Reverted — those 2 warnings stay deferred, same as the mixed-decls PR concluded.I have documented this change in the design system.
I have recorded this change in
CHANGELOG.md.Issue
Part of #1084
Testing
npm run lint,npm test(47 specs, Firefox + Chrome),npm run build-docs,npm run build-package, and a before/after compile diff all pass.