fix: keep Font Awesome icons centered in a menu with a title

[bbrala]

Closes #738

Problem

The documented recipe for giving a menu a title (see the menu title demo) is a :before pseudo element on the menu plus a top margin on its first child, to make room for that title:

.css-title :first-child {
    margin-top: 20px;
}

A Font Awesome icon is rendered as an <i> element inside the menu item, so it is the first child of its own <li>. That descendant :first-child selector matched every icon as well and pushed them all down by the title's height, leaving the icons hanging below their label. The <label> of an input item hit the same thing.

Measured on master, with a title the icon's center sat 20.6px below its item's center. Without a title it was 0.6px.

Fix

The <i>/<svg> element is created and positioned by this plugin, so its box is now pinned down completely instead of relying on whatever the icon library or the surrounding page leaves it at:

• margin: -0.5em 0 0 resets the margin, so page level rules such as the title recipe can no longer displace the icon. The plugin's selector is more specific than the recipe's, so pages already using the old descendant selector are fixed without touching their CSS.
• height: 1em + line-height: 1 + top: 50% center the icon in its item for any item padding or font size, replacing the fixed top: 0.3em that only happened to be near center at the default sizes. This matches how the built-in icon font is already centered.
• Centering via margin rather than transform leaves Font Awesome's own fa-rotate-* and fa-flip-* helpers working.

Documentation wise, the recipe in the menu title demo now uses a child combinator (> :first-child), since that top margin is only ever meant for the first menu item, with a note explaining why.

Tests

New Playwright spec test/specs/menu-title-icon-alignment.js plus a new demo fixture documentation/demo/menu-title-fontawesome.md combining a menu title with Font Awesome icons. It asserts the icon's vertical center is within 1.5px of its item's vertical center for:

• Font Awesome icons, with a title
• Font Awesome icons, without a title
• Font Awesome icons with the pre-font-awesome 5 Icons with Title #738 descendant :first-child rule injected, so the plugin CSS itself stays robust for pages in the wild
• the built-in icon font, with and without a title

The three Font Awesome cases fail on master and pass with this change. The built-in icon cases pass either way and are there so the Font Awesome fix cannot be made at their expense.

dist/ is intentionally not committed.

[bbrala]

Heads up on the CI failure on the first push: it was not the fix, it was the fixture pipeline.

The acceptance job copies the committed dist/ into the fixture pages, and dist/ is only rebuilt at release time, so it is normally a few commits behind src/. On master today it is already missing the RTL rules from #797, for example. That means anything changed in src/sass (or src/jquery.contextMenu.js) is invisible to the Playwright suite on CI, and a regression test written against it fails there while passing locally.

Added ci: build dist before generating acceptance test fixtures (de850fc) which runs npm run build in the acceptance job before npm run test:fixtures. dist/ itself is still not committed.

Happy to split that into its own PR if you would rather keep this one to the fix.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f43e935c7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Compile the changed Sass before exercising this fixture

In every acceptance job, npm run test:fixtures generates pages whose stylesheet link points to the checked-in dist/jquery.contextMenu.css, and the workflow proceeds directly to Playwright without running npm run build. Because this commit changes only the Sass source, the loaded CSS still has top: 0.3em and no margin reset; therefore this injected descendant rule gives each icon a 20px top margin and the subsequent centering assertion fails across the acceptance matrix. Update the tracked distribution CSS or make the fixture/workflow compile and load the changed stylesheet.

Useful? React with 👍 / 👎.

[bbrala]

Thanks, that P1 is correct and it is the same thing the first CI run tripped on. It was already addressed in de850fc, pushed before the review landed (Codex reviewed 4f43e93).

The acceptance job now runs npm run build before npm run test:fixtures, so the fixture pages load CSS compiled from the changed Sass rather than the checked-in bundle. I went with building in CI rather than committing dist/, since dist/ in this repo is only regenerated at release time and recent PRs deliberately leave it alone. As a side effect the acceptance suite now also covers src/jquery.contextMenu.js changes, which it silently did not before.

Nothing else from the review to address.

[bbrala]

👍