Skip to content

Optimize header layout performance with flexbox mixins - #5

Open
joelachance wants to merge 1 commit into
header-layout-optimization-prefrom
header-layout-optimization-post
Open

joelachance wants to merge 1 commit into
header-layout-optimization-prefrom
header-layout-optimization-post

Conversation

@joelachance

Copy link
Copy Markdown

Benchmark PR recreated from ai-code-review-evaluation/discourse-graphite for Code Review Bench. Upstream: ai-code-review-evaluation#5

.contents {
margin: 8px 0;
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Header .contents becomes a flex container but .title child lost its selector

Before this change, .title had float: left as a separate rule. The diff removes the .title rule entirely and makes .contents a flex container. However, the .title element no longer has any explicit order set, while .panel gets order(3) and .extra-info-wrapper gets order(2). Without an explicit order on .title, its visual position depends on source order only among unordered siblings. If any plugin or theme injects content into .contents, the title may appear after other flex children that also lack an explicit order.

Explicit ordering prevents fragile reliance on DOM source order for the most prominent header element, reducing regression risk from plugins or minor template reordering.

Add @include order(1); to a .title rule inside .d-header .contents in app/assets/stylesheets/common/base/header.scss to make the intended layout order explicit and robust.


@mixin order($val) {
-webkit-box-ordinal-group: $val;
-moz-box-ordinal-group: $val;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

-webkit-box-ordinal-group requires value incremented by 1

The order mixin passes $val directly to -webkit-box-ordinal-group, but the old flexbox spec (-webkit-box-ordinal-group) is 1-based — an order: 2 equivalent requires -webkit-box-ordinal-group: 3, and order: 3 requires 4. Passing the same numeric value means elements will be misordered in older WebKit browsers (Safari 5/6, old Android stock browser) that only support the 2009 flexbox spec.

Correct visual ordering on older WebKit browsers that this codebase explicitly supports (Safari 5.1+, iOS 7+, Android 4.1+), preventing layout regression for those users.

In app/assets/stylesheets/common/foundation/mixins.scss, change the order mixin to output -webkit-box-ordinal-group: $val + 1; instead of -webkit-box-ordinal-group: $val;. Verify by inspecting computed styles in an older WebKit engine or by checking the 2009 spec behavior.

display: -moz-inline-box;
display: -ms-inline-flexbox;
display: inline-flex;
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

align-items mixin emits incorrect value for -webkit-box-align and -ms-flex-align

The align-items mixin blindly passes $alignment (e.g. center, baseline) to -webkit-box-align and -ms-flex-align. The 2009 box model uses different keywords: center is middle for -webkit-box-align, and baseline maps to baseline but center does not. Similarly -ms-flex-align uses center but calls baseline as baseline — however -webkit-box-align: center is not a valid value; it should be middle. This means @include align-items(center) (used in header.scss and topic-post.scss) will have no effect on older WebKit.

Header and small-action elements will be vertically centered correctly on Safari ≤6 and Android 4.x stock browser, matching the intended design.

In the align-items mixin in app/assets/stylesheets/common/foundation/mixins.scss, map the standard values to their legacy equivalents: e.g. use a conditional or a Sass map to output -webkit-box-align: middle when $alignment == center, -webkit-box-align: start when flex-start, etc. At minimum handle center → middle since that is the value used in this PR.

display: -webkit-inline-box;
display: -webkit-inline-flex;
display: -moz-inline-box;
display: -ms-inline-flexbox;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fix invalid legacy flexbox values in align-items() mixin

The new @mixin align-items($alignment) writes $alignment directly into -webkit-box-align. When called with center or baseline (as in this patch), older -webkit-box-align expects middle or baseline, and center may be ignored, causing vertical misalignment specifically in older WebKit/Android browsers that still use the 2009 flexbox syntax.

Prevents real cross-browser layout regressions where elements stop vertically aligning (header contents and badge bullets) on older engines covered by the project’s long-tail browser support.

In app/assets/stylesheets/common/foundation/mixins.scss, update @mixin align-items to map modern values to 2009 spec values for -webkit-box-align (and optionally -moz-box-align). For example:

  • if $alignment == center use middle for *-box-align
  • if $alignment == flex-start use start
  • if $alignment == flex-end use end
    Keep the modern align-items / -webkit-align-items / -ms-flex-align as-is. Then verify in a browser matrix (at least Safari + an older Android/WebKit if you still target it).

-webkit-box-align: $alignment;
-webkit-align-items: $alignment;
-ms-flex-align: $alignment;
-ms-align-items: $alignment;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Correct order() mixin’s -webkit-box-ordinal-group value (off-by-one in old spec)

The new @mixin order($val) sets -webkit-box-ordinal-group: $val;. In the 2009 flexbox spec, box-ordinal-group is 1-based, while modern order is 0-based. Using order(3) (added in header.scss) may not match the intended ordering in older WebKit, producing inconsistent element order across browsers.

Avoids inconsistent header layout ordering on older WebKit implementations (elements can appear in an unexpected sequence).

In app/assets/stylesheets/common/foundation/mixins.scss, change -webkit-box-ordinal-group (and -moz-box-ordinal-group) to $val + 1 to match modern order semantics (0-based). Example: -webkit-box-ordinal-group: $val + 1; If SCSS arithmetic needs interpolation, use #{ $val + 1 }. Then re-check header ordering in supported browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant