Repository navigation
fix: navbar font mismatch and overlap before Bootstrap loads - #1039
Conversation
Reviewer's GuideVersion 3.8.2 fixes inconsistent Chinese glyph fallback in the monochrome navbar by applying an explicit shared font stack, and prevents content overlap when Bootstrap is delayed or unavailable by setting and restoring the navbar’s top offset. New offline Chromium regressions cover rendered fonts, responsive pre-Bootstrap positioning, teardown, and inline-style preservation; human verification in both new and classic UIs remains required. Sequence diagram for pre-Bootstrap navbar positioningsequenceDiagram
participant Page
participant NavbarStyler
participant Navbar
participant Bootstrap
Page->>NavbarStyler: apply()
NavbarStyler->>Navbar: preserveStyles(top)
NavbarStyler->>Navbar: classList.add(fixed-top)
NavbarStyler->>Navbar: style.top = 0
Note over Navbar,Bootstrap: Navbar stays anchored even when Bootstrap CSS is delayed or unavailable
Bootstrap-->>Navbar: load fixed-top CSS
Page->>NavbarStyler: remove()
NavbarStyler->>Navbar: restore original top style
Flow diagram for consistent monochrome navbar fontsflowchart LR
A[Document language: en or zh-CN] --> B[Monochrome navbar]
B --> C[Explicit shared navbar font stack]
C --> D[Chinese glyphs use consistent CJK fallback]
C --> E[Latin glyphs retain Source Serif 4]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Deploying xmoj-script-dev-channel with
|
| Latest commit: |
c730563
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://baf39141.xmoj-script-dev-channel.pages.dev |
| Branch Preview URL: | https://codex-fix-navbar-cjk-font.xmoj-script-dev-channel.pages.dev |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/navbar-styler.test.cjs" line_range="192" />
<code_context>
+
+test('Chinese navbar glyphs use the same fonts under legacy and web document languages', {timeout: 60000}, async () => {
+ const skin = source.match(/const MonochromeSkinCSS = `([\s\S]*?)`;/)[1];
+ const font = fs.readFileSync(path.join(__dirname, "fixtures/fonts/source-serif-4-latin.woff2")).toString("base64");
+ const browser = await chromium.launch({executablePath: process.env.XMOJ_CHROMIUM || undefined});
+ try {
</code_context>
<issue_to_address>
**issue (testing):** The navbar regression test unconditionally reads `tests/fixtures/fonts/source-serif-4-latin.woff2`, but that fixture is absent from the diff and the fixtures directory contains only `OFL.txt` and `README.md`; running the test therefore raises `ENOENT` before launching Chromium.
**Triggers:** When `npm test` or `tests/navbar-styler.test.cjs` runs.
**Suggested fix:** Add the referenced WOFF2 fixture at the expected path, or update the test to reference a file that is actually included.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: tests/navbar-styler.test.cjs:192
1011a13 to
c730563
Compare
Sourcery withdrew this approval because the latest commits introduced blocking findings.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/navbar-styler.test.cjs">
<violation number="1" location="tests/navbar-styler.test.cjs:250">
P3: This late-CSS check is vacuous: inline `position: fixed` and `top: 0` already match the injected rule, while the fixed height leaves the asserted vertical geometry unchanged. Assert a Bootstrap-only effect, such as the navbar's horizontal bounds, so this check detects a broken late stylesheet transition.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| assert.equal(before.navbarTop, mono ? 0 : 16, 'the navbar must anchor to the viewport without Bootstrap'); | ||
| assert.ok(before.contentTop >= before.navbarBottom, 'the navbar must not cover content while CSS is unavailable'); | ||
| // When Bootstrap finally arrives, the reserved space still works. | ||
| await page.addStyleTag({content: '.fixed-top { position: fixed; top: 0; right: 0; left: 0; }'}); |
There was a problem hiding this comment.
P3: This late-CSS check is vacuous: inline position: fixed and top: 0 already match the injected rule, while the fixed height leaves the asserted vertical geometry unchanged. Assert a Bootstrap-only effect, such as the navbar's horizontal bounds, so this check detects a broken late stylesheet transition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/navbar-styler.test.cjs, line 250:
<comment>This late-CSS check is vacuous: inline `position: fixed` and `top: 0` already match the injected rule, while the fixed height leaves the asserted vertical geometry unchanged. Assert a Bootstrap-only effect, such as the navbar's horizontal bounds, so this check detects a broken late stylesheet transition.</comment>
<file context>
@@ -223,3 +223,44 @@ test('Chinese navbar glyphs use the same fonts under legacy and web document lan
+ assert.equal(before.navbarTop, mono ? 0 : 16, 'the navbar must anchor to the viewport without Bootstrap');
+ assert.ok(before.contentTop >= before.navbarBottom, 'the navbar must not cover content while CSS is unavailable');
+ // When Bootstrap finally arrives, the reserved space still works.
+ await page.addStyleTag({content: '.fixed-top { position: fixed; top: 0; right: 0; left: 0; }'});
+ const after = await Measure();
+ assert.ok(after.contentTop >= after.navbarBottom);
</file context>
|
这一版的 topbar 字体都变了 |
|
奇怪 |
? Intended. |
|
但是这个PR 修的是别的问题 |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="XMOJ.user.js" line_range="2891" />
<code_context>
n.classList.add('fixed-top', 'container', 'ml-auto');
+ // The CDN fallback may still be loading when the top bar starts. Without
+ // Bootstrap's .fixed-top rule, top:auto moves it down with the spacer.
+ n.style.top = '0';
if (UtilityEnabled("MonochromeUI")) {
Object.assign(n.style, {
</code_context>
<issue_to_address>
**issue (bug_risk):** An existing inline `top: ... !important` declaration prevents `n.style.top = '0'` from taking effect, so the navbar keeps its page-provided offset instead of being pinned to the viewport top. With a sufficiently large important offset, the navbar still overlaps the page content despite the positioning fix.
**Triggers:** When the target navbar already has an inline `top` declaration with `!important`.
**Suggested fix:** Set the temporary offset with `n.style.setProperty('top', '0', 'important')`, while retaining the captured value and priority for teardown.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: XMOJ.user.js:2891
| n.classList.add('fixed-top', 'container', 'ml-auto'); | ||
| // The CDN fallback may still be loading when the top bar starts. Without | ||
| // Bootstrap's .fixed-top rule, top:auto moves it down with the spacer. | ||
| n.style.top = '0'; |
There was a problem hiding this comment.
issue (bug_risk): An existing inline top: ... !important declaration prevents n.style.top = '0' from taking effect, so the navbar keeps its page-provided offset instead of being pinned to the viewport top. With a sufficiently large important offset, the navbar still overlaps the page content despite the positioning fix.
Triggers: When the target navbar already has an inline top declaration with !important.
Suggested fix: Set the temporary offset with n.style.setProperty('top', '0', 'important'), while retaining the captured value and priority for teardown.
What does this PR aim to accomplish?:
After #1036, the monochrome navbar still renders Chinese text in different fonts on /web and classic pages. Both pages load Source Serif 4 and have identical computed font-family, but that font contains no Chinese glyphs. Classic declares lang=en while /web declares lang=zh-CN. On macOS Chromium, the browser picks PingFang SC for classic and Songti SC for /web. Switching the live /web document language to en also changes its rendered Chinese navbar font to PingFang SC.
A second reproducible failure affects generic page layouts when the Bootstrap resource cache is empty and its CDN fallback is delayed or fails. NavbarStyler sets position:fixed but relies on Bootstrap's .fixed-top for top:0. With top:auto, inserting the body spacer pushes the navbar down with the page, where it covers the first content instead of anchoring to the viewport. An offline layout probe reproduces overlap in all 24 width/skin/theme/menu combinations per version on both stable and pre-fix dev.
Related to issue #1038. Its reported all-page overlap remains unconfirmed in the reporter's browser because it contains no reproduction steps. This PR fixes the demonstrated missing-CSS condition and does not claim that it is the confirmed cause of that report.
How does this PR accomplish the above?:
Give the monochrome navbar and its links one explicit font stack, preserving the existing Latin serif fonts and adding PingFang SC, Microsoft YaHei and Noto Sans CJK SC before the generic fallback. The brand, user dropdown and dropdown items inherit that navbar stack. Body typography remains unchanged.
Pin the navbar top offset directly in NavbarStyler, so viewport anchoring works before Bootstrap is available. Preserve and restore its original inline top value and priority during teardown.
Validation:
Please verify this with the installed userscript in both UIs before merging. The final checklist is unchecked because template item 11 requires human verification; the earlier PR was merged without that check.
By submitting this pull request, I confirm the following:
git rebase)Summary by Sourcery
Fix monochrome navbar font consistency and prevent content overlap while Bootstrap styles are unavailable.
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: