Convert templates/ and nav/menu/footer family to logical properties by stephaniehobson · Pull Request #1147 · mozilla/protocol

stephaniehobson · 2026-09-11T19:02:19Z

Description

Convert templates/ and nav/menu/footer family to logical properties

~~I have documented this change in the design system.~~
I have recorded this change in CHANGELOG.md.

Issue

Part of #1084

Testing

Spot check in RTL. I ran a visual regression locally and found no problems.

stephaniehobson · 2026-10-02T02:08:03Z

        &:nth-child(odd) {
            clear: inline-start;
-            @include bidi(((padding, 0 ($layout-md * 0.5) 0 0, 0 0 0 ($layout-md * 0.5)),));
+            padding-inline-end: $layout-md * 0.5;


It is going to drive me a little crazy that there is no logical property short hand for all 4 values.

Many changes in this PR no longer zero out padding that used to be explicitly removed by the previous declaration.

ya, this ends up being a footgun for a lot of people. They keep using the padding shorthand either for zeroing or even the multi-value syntax, but then they mix the individual logical properties (and then never test). I end up just using padding-block and padding-inline or the longhands, unless I’m zeroing everything.

stephaniehobson · 2026-10-06T03:54:37Z

Visual regression tests run locally find no regressions so I'm opening this for review.

@include

…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.

knowler

r+wc

Looks good after a quick spot check. Just one question below.

knowler · 2026-10-13T15:37:10Z

        @media #{$mq-md} {
-            @include bidi(((right, $layout-lg, left, auto),));
+            right: $layout-lg;
+
+            [dir='rtl'] & {
+                right: auto;
+                left: $layout-lg;
+            }
        }

        @media #{$mq-lg} {
-            @include bidi(((right, $layout-xl, left, auto),));
+            right: $layout-xl;
+
+            [dir='rtl'] & {
+                right: auto;
+                left: $layout-xl;
+            }
        }


Should these just be inset-inline-end?

stephaniehobson force-pushed the v23/logical-templates branch from c3f5837 to a1ab063 Compare September 11, 2026 20:29

stephaniehobson added this pull request to stack #1149 September 11, 2026 20:33

stephaniehobson closed this Sep 11, 2026

stephaniehobson reopened this Sep 11, 2026

stephaniehobson force-pushed the v23/logical-templates branch from a1ab063 to c357f50 Compare September 14, 2026 23:08

stephaniehobson force-pushed the v23/logical-templates branch from c357f50 to 03e6803 Compare September 29, 2026 20:45

stephaniehobson force-pushed the v23/logical-templates branch 2 times, most recently from f37ae80 to 27ba758 Compare October 2, 2026 02:05

stephaniehobson commented Oct 2, 2026

View reviewed changes

Base automatically changed from v23/logical-base to main October 5, 2026 17:44

stephaniehobson force-pushed the v23/logical-templates branch from 27ba758 to ef493a3 Compare October 5, 2026 17:44

stephaniehobson marked this pull request as ready for review October 6, 2026 03:54

stephaniehobson added the Needs:Review 👋 Ready for Developer Review label Oct 6, 2026

stephaniehobson force-pushed the v23/logical-templates branch 2 times, most recently from fbc0f31 to 1d2d070 Compare October 7, 2026 19:41

knowler approved these changes Oct 13, 2026

View reviewed changes

stephaniehobson merged commit 24b794c into main Oct 13, 2026
8 checks passed

stephaniehobson deleted the v23/logical-templates branch October 13, 2026 21:24

Navigation Menu

Repository navigation

Convert templates/ and nav/menu/footer family to logical properties - #1147

Convert templates/ and nav/menu/footer family to logical properties#1147
stephaniehobson merged 1 commit into
mainfrom
v23/logical-templates

stephaniehobson commented Sep 11, 2026 •

edited

Loading

Uh oh!

stephaniehobson Oct 2, 2026

Uh oh!

knowler Oct 5, 2026 •

edited

Loading

Uh oh!

stephaniehobson commented Oct 6, 2026

Uh oh!

knowler left a comment

Uh oh!

knowler Oct 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

stephaniehobson commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue

Testing

Uh oh!

stephaniehobson Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

knowler Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephaniehobson commented Oct 6, 2026

Uh oh!

knowler left a comment

Choose a reason for hiding this comment

Uh oh!

knowler Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stephaniehobson commented Sep 11, 2026 •

edited

Loading

knowler Oct 5, 2026 •

edited

Loading