Convert remaining components to CSS logical properties and remove bidi() (#1084) by stephaniehobson · Pull Request #1174 · mozilla/protocol

stephaniehobson · 2026-10-06T04:24:56Z

Description

Convert remaining components to CSS logical properties and remove bidi() (#1084)

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

Issue

#1084

Testing

TBA

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

@include

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.

@include

…i() (#1084) Converts the last 26 @include bidi() call sites across button, breadcrumb, card, choice, language-switcher, logo, wordmark, modal, notification-bar, picto, and sticky-promo, plus a handful of plain physical properties (clear, margin-left) in the same files. Properties with no logical equivalent (background-position, content, animation-name) keep an explicit [dir='rtl'] override instead. With every internal call site converted, deletes includes/mixins/_bidi.scss and its @forward, and fixes the Fractal docs theme's own use of the mixin.

stephaniehobson · 2026-10-06T17:06:55Z

-                (left, 0.03rem, auto),
-                (right, auto, 0.83rem),
-            ));
+


Can this be inset-inline-start?

inset-inline-start: 0 as set on line 96 without lines 121, 128-131 works when I apply those changes (I’m just inspecting the live site).

Copilot

Copilot review overview

🟢 Approval recommended

The migration is complete and behavior-preserving, with only minor duplicate notification-bar rules noted for cleanup.

Review effort: Balanced
Findings: 1

Open (1)

Remove redundant RTL positioning block · New

What changed in this PR

Completes the migration from the bidi() Sass mixin to CSS logical properties and explicit RTL overrides.

Changes:

Converts remaining component styles to logical properties.
Removes the public bidi() mixin.
Adds migration guidance and changelog entries.

File	Description
`CHANGELOG.md`	Records the breaking migration.
`docs/02-usage/02-framework.md`	Removes obsolete mixin documentation.
`docs/02-usage/migration.md`	Adds consumer migration steps.
`assets/sass/protocol/includes/mixins/_index.scss`	Stops exporting `bidi()`.
`assets/sass/protocol/includes/mixins/_bidi.scss`	Removes the mixin implementation.
`assets/sass/protocol/components/_breadcrumb.scss`	Adds an explicit RTL arrow.
`assets/sass/protocol/components/_button.scss`	Uses logical icon margins.
`assets/sass/protocol/components/_card.scss`	Converts tag spacing and RTL positioning.
`assets/sass/protocol/components/_inline-list.scss`	Uses logical list margins.
`assets/sass/protocol/components/_language-switcher.scss`	Converts link margins.
`assets/sass/protocol/components/_modal.scss`	Converts padding and positioning.
`assets/sass/protocol/components/_notification-bar.scss`	Reworks directional close-button styling.
`assets/sass/protocol/components/_picto.scss`	Converts side layout positioning.
`assets/sass/protocol/components/_sticky-promo.scss`	Converts positioning and explicit RTL exceptions.
`assets/sass/protocol/components/forms/_button-container.scss`	Converts adjacent-button spacing.
`assets/sass/protocol/components/forms/_choice.scss`	Converts form control layout properties.
`assets/sass/protocol/components/logos/_logo.scss`	Adds explicit RTL background positioning.
`assets/sass/protocol/components/logos/_wordmark.scss`	Adds explicit RTL background positioning.
`theme/assets/sass/components/_pen.scss`	Replaces the documentation-theme mixin usage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        right: 0;
        top: 0;
        width: 20px;

+        [dir='rtl'] & {
+            right: auto;
+            left: 0;
+        }


stephaniehobson · 2026-10-06T17:10:42Z

        margin: $spacing-sm;
        padding: 0;
        position: absolute;
+        right: 0;


Why switch to this away from inset-inline-end?!?!

stephaniehobson · 2026-10-06T17:11:56Z

-        (left, auto, 0),
-    ));
+
+    [dir='rtl'] & {


Inset-inline?

stephaniehobson · 2026-10-06T17:12:59Z

I think we should leave bidi() available. Removing it seems like a bigger, harder change for down-stream to absorb.

stephaniehobson added 2 commits October 5, 2026 21:19

stephaniehobson added this pull request to stack #1149 October 6, 2026 04:24

stephaniehobson requested a balanced review from Copilot October 6, 2026 17:05

Copilot started reviewing on behalf of stephaniehobson October 6, 2026 17:06 View session

stephaniehobson commented Oct 6, 2026

View reviewed changes

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread assets/sass/protocol/components/_notification-bar.scss

Comment on lines +71 to +78

right: 0;

top: 0;

width: 20px;

[dir='rtl'] & {

right: auto;

left: 0;

}

stephaniehobson commented Oct 6, 2026

View reviewed changes

Navigation Menu

Repository navigation

Convert remaining components to CSS logical properties and remove bidi() (#1084) - #1174

Convert remaining components to CSS logical properties and remove bidi() (#1084)#1174
stephaniehobson wants to merge 2 commits into
v23/logical-templatesfrom
v23/logical-components

stephaniehobson commented Oct 6, 2026 •

edited

Loading

Uh oh!

stephaniehobson Oct 6, 2026

Uh oh!

knowler Oct 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

stephaniehobson Oct 6, 2026

Uh oh!

stephaniehobson Oct 6, 2026

Uh oh!

stephaniehobson commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Description

Issue

Testing

Uh oh!

stephaniehobson Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

knowler Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

stephaniehobson Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

stephaniehobson Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

stephaniehobson commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stephaniehobson commented Oct 6, 2026 •

edited

Loading