fix(magecommand-less): at-rule selector scoping follows the less.php version - #117
Open
pingiun wants to merge 2 commits into
Open
fix(magecommand-less): at-rule selector scoping follows the less.php version#117pingiun wants to merge 2 commits into
pingiun wants to merge 2 commits into
Conversation
…version Magento's own `_rules-temp.less` nests an `@-moz-document url-prefix()` fieldset fix inside `.admin__scope-old`. Whether that nesting carries the enclosing selector into the at-rule's body is `isRooted` in the parser's directive table — and it is VERSION-DEPENDENT, which we modelled as a constant: - less.php 3.x has no `isRooted` concept at all, so nothing carries the selector, and a 3.2.1 store deploys `.rule-tree .fieldset`; - less.php 5.x sets `$isRooted = false` for `@document`/`@supports`, and a 5.5.1 store deploys `.admin__scope-old .rule-tree .fieldset`; - less.js 4.x adds `@starting-style` and `@layer` on top. We hardcoded the less.js 4.x set for every profile, so the 3.x store got a rule scoped to `.admin__scope-old` that the real deploy applies globally — a cascade difference, not formatting. The set is now data on `LessOptions` (`non_rooted_at_rules`), chosen by the profile the deploy already detects from the store's installed `wikimedia/less.php`. Matched on the vendor-STRIPPED name throughout: every version computes a `nonVendorSpecificName` before its switch, so `@-moz-document` is `@document`. (I briefly "fixed" this by matching the raw name instead, which produced the right answer on the 3.x store and broke the 5.x one — the versions differ in the TABLE, not in prefix handling.) Verified against a real `setup:static-content:deploy` on both: the 5.5.1 store's `styles-old.css` goes from 15 semantic findings to 0, and the 3.2.1 store loses the two the at-rule contributed. The vendored less.js fixture suite is unchanged — it runs the LessJs profile, whose set keeps all four entries. The conformance test pins both directions and a rooted control (`@media` still nests by its own path); it fails if the 3.x entry is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ss.php, and the extend gap it found
`wikimedia/less.php` 3.x and 5.x are different compilers, and Magento 2.4.7 and
2.4.8 ship them respectively — so a single source file legitimately deploys
different css on two stores. Reasoning about which side is "right" from a
deployed stylesheet alone does not work; both times I tried, I got it backwards
once before checking the second store.
`scripts/diff-against-less-php.sh` compiles a `.less` with a STORE's own
library and diffs it against ours. Pointed at two stores on different majors it
separates "we are wrong" from "the versions disagree", which is the distinction
that decides whether anything needs fixing at all. It found the at-rule scoping
difference and the one below.
The one below: `&:extend` inside a MIXIN called more than once. Each call site
registers its own extend, and less.php 5.x lets the second match selectors the
FIRST call site generated, emitting `.abs .sub .sub` from
.mx() { color: red; .sub { &:extend(.menu all); color: blue; } }
.menu { .mx(); }
.abs { .mx(); }
Writing that structure out literally, or calling the mixin once, produces no
such selector on either version — the duplicated mixin is the whole trigger.
We match 5.x byte for byte, which the new conformance test pins. less.php 3.x
does NOT emit them, so a 2.4.7 store deploys a smaller stylesheet than we
produce: ~17KB of extra selectors across one real store's admin theme, 106
findings in `styles.css` and 13 in `styles-old.css`. Bloat rather than a cascade
change — every selector we add is a descendant chain matching nothing the
reference matched — but it is real, and the `Magento247` profile does not model
2.5.3's extend semantics yet.
Not attempted here on purpose: the two versions' `ProcessExtends` visitors are
byte-identical modulo formatting, so the divergence originates upstream of the
extend pass, and guessing at it risks the 5.x path that currently matches
exactly. The minimal case is pinned and reproducible; that is the right place
for the next person to start.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Magento's own
_rules-temp.lessnests an@-moz-document url-prefix()fieldset fix inside.admin__scope-old. Whether that nesting carries the enclosing selector into the at-rule's body isisRootedin the parser's directive table — and it is version-dependent, which we modelled as a constant.isRootedconcept at all).rule-tree .fieldset@document,@supports.admin__scope-old .rule-tree .fieldset@starting-style,@layerWe hardcoded the less.js 4.x set for every profile, so a 3.x store got a rule scoped to
.admin__scope-oldthat the real deploy applies globally. That is a cascade difference, not formatting — the Firefox fieldset fix silently stops applying outside that scope.The set is now data on
LessOptions(non_rooted_at_rules), chosen by the profile the deploy already detects from the store's installedwikimedia/less.php.One wrong turn worth recording
I first "fixed" this by matching on the raw at-rule name instead of the vendor-stripped one — reasoning that
@-moz-documentmisses acase "@document"arm. That produced the right answer on the 3.x store and broke the 5.x one, because both versions compute anonVendorSpecificNamebefore the switch. The versions differ in the table, not in prefix handling. Caught it by checking the second store rather than shipping on one green result.Validation
Against a real
setup:static-content:deployon two stores running different less.php versions:styles-old.css15 semantic findings → 0:extendover-generation)The vendored less.js fixture suite is unchanged — it runs the
LessJsprofile, whose set keeps all four entries. Workspace tests and clippy green.The conformance test pins both directions plus a rooted control (
@mediastill nests by its own path, so the fix didn't make everything wrap), and I confirmed it fails when the 3.x entry is removed.Still open on these stores
Unrelated and untouched:
:extendover-generating selector lists (13 and 106 findings on the 3.x store), the cssmin#ffffffeb→#fff ebserialization difference (where Magento's output is the broken one), andurl()relocation — on which I owe a correction: both magecommand's and Magento's URLs 404 there, the working path being a third one, so that one is not the visible breakage I first called it.🤖 Generated with Claude Code