Convert templates/ and nav/menu/footer family to logical properties - #1148
Closed
stephaniehobson wants to merge 2 commits into
Closed
stephaniehobson wants to merge 2 commits into
stephaniehobson wants to merge 2 commits into
Conversation
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.
…1084) Second pass of the logical-properties migration -- the templates/ and navigation/menu/footer component family, all built against current main (post mixed-decls merge), not the older WS-4 branch state. 59 of the 110 remaining @include bidi() calls removed: templates/_card-layout.scss (15 -> 0) _navigation.scss (11 -> 0) _footer.scss (10 -> 0) _menu-item.scss (6 -> 0) _menu.scss (5 -> 0) _menu-list.scss (4 -> 0) _sidebar-menu.scss (4 -> 0) templates/_main-with-sidebar.scss (4 -> 0) All eight files are now fully bidi()-free. Notable non-mechanical cases: - _navigation.scss / _menu-list.scss: several bidi() calls used the 3-value "same property, different value per direction" form in pairs (e.g. two separate (padding-left, X, 0) / (padding-right, 0, X) tuples) rather than one 4-value tuple. Same underlying swap pattern, just spelled differently -- traced each pair through by hand to confirm which logical property they resolve to. - _sidebar-menu.scss: one bidi() tuple paired a margin swap with a `transform: none / translateY(3px) rotate(180deg)` pair, flipping a ▸ triangle glyph to point the other way in RTL. transform has no logical/direction-aware equivalent (it's always in the element's own coordinate space), so that one stays an explicit [dir='rtl'] override -- only the margin half converted to margin-inline-start. - _navigation.scss: two `background-position` bidi() calls stay physical with [dir='rtl'] overrides (same policy as the select rule in E1 -- no safe logical keyword syntax for background-position across this browser matrix). One of the two also had a bidi() tuple that was identical in both directions (dead weight, a pure no-op) -- collapsed to a single plain declaration. - templates/_card-layout.scss, _menu.scss, _menu-item.scss: also converted several bare (non-bidi-wrapped) margin-left/margin-right:0 pairs to margin-inline: 0 -- these were plain symmetric physical declarations sitting right next to the bidi() calls, in scope for the same inline-axis cleanup even though they weren't wrapped in the mixin. Two real bugs found via a reported visual regression (menu-list component rendering padding: 0 24px 0 4px locally vs. production's padding: 0 24px 0 0) and fixed here, both the same class as the label.mzp-u-inline bug found in the E1 commit -- a full 4-value padding/margin shorthand implicitly zeroes every side it doesn't otherwise set, and replacing it with a single logical longhand drops that protection for the sides not touched: - _menu-list.scss's `.is-details .mzp-c-menu-list-title button`: the original bidi() call's LTR value was the full shorthand "0 (16px + $spacing-sm) 0 0", explicitly zeroing padding-top/bottom/ left. My conversion had only set padding-inline-end, so the button's padding-block and padding-inline-start fell through to the browser's UA default <button> padding (non-zero on every browser) instead of the intended 0. Fixed with padding-block: 0; padding-inline: 0 X. - _footer.scss's `.mzp-c-footer-section:first-child`/`:last-child` at $mq-lg: the parent rule sets an explicit padding: 0 (X); shorthand (both inline sides non-zero), and :first-child/:last-child each need to zero out *one specific side* against that parent value -- not restate the other side, which is what I'd written (a copy-paste-shaped mistake: I pattern-matched this diff's shape against a different, unrelated footer case that had no competing parent padding, and used the same-looking conversion without re-deriving it against this rule's actual cascade). Fixed to padding-inline-start: 0 / padding-inline-end: 0 respectively. Given the severity, went back and systematically re-audited every other bare "padding"/"margin"-shorthand-origin bidi() conversion across both this commit and the E1 commit against the *actual* parent-cascaded baseline (not just diff-shape pattern matching) -- checked ~20 selectors individually against true-original compiled output. Everything else checked out: card-layout's, navigation's, and menu-item's single-side overrides all rely on an explicit sibling declaration (an earlier-cascading media query, or a base rule in the same file) that already supplies the correct value for the side not touched -- confirmed byte-for-byte via the same before/after compile diff technique used throughout, this time checked per-selector rather than by diff shape. Also confirmed the already-merged mixed-decls PR doesn't have this bug class: it only did float/text-align keyword swaps (no "sides" to drop), never converted a padding/margin shorthand. Branched directly off current main rather than the stale WS-4 chain (D1/D2/E1 sit on pre-mixed-decls-merge main via the WS-3 tip) -- avoided converting bidi() calls against a structure that would need re-reconciling again once that chain rebases. This PR has no file overlap with D1/D2/E1's own scope. 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 exhaustive before/after compile diff described above all pass.
stephaniehobson
force-pushed
the
v23/logical-templates
branch
from
September 11, 2026 20:29
c3f5837 to
a1ab063
Compare
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
Second pass of the logical-properties migration.
templates/_card-layout.scss (15 -> 0)
_navigation.scss (11 -> 0)
_footer.scss (10 -> 0)
_menu-item.scss (6 -> 0)
_menu.scss (5 -> 0)
_menu-list.scss (4 -> 0)
_sidebar-menu.scss (4 -> 0)
templates/_main-with-sidebar.scss (4 -> 0)
I have documented this change in the design system.CHANGELOG.md.Issue
Part of #1084.
Testing
Enter helpful notes for whoever code reviews this change.