fix(native): answer an undecidable media condition with unknown rather than false - #426
Open
YevheniiKotyrlo wants to merge 11 commits into
Open
fix(native): answer an undecidable media condition with unknown rather than false#426YevheniiKotyrlo wants to merge 11 commits into
YevheniiKotyrlo wants to merge 11 commits into
Conversation
`extractMedia` pushed every query of a comma-separated `@media` prelude into `StyleRule.m` as a separate entry. That array is a conjunction — the runtime tests it with `.every()` — so `@media (a), (b)` required both `(a)` and `(b)` to match. CSS specifies a comma list as a union: the block applies when any one query matches. `parseMediaQuery` now returns its condition instead of writing it into the builder, and `extractMedia` combines the list. Two or more queries are joined with the `"|"` condition the IR already carries for `or`, so a comma list and `(a) or (b)` compile to the same thing. A single query is added unwrapped, so existing output does not change and it still intersects with the conditions of any enclosing rule. A query that cannot compile contributes nothing, which is how CSS treats an unmatchable query in a list. Covers both planes: the compiler emits the union, and the native runtime applies a rule whose list has one matching branch.
A media feature value the compiler cannot resolve compiles to `undefined`, and a container feature the runtime cannot measure reads back `undefined`. Both evaluators then produced a match instead of refusing: - `testComparison` dispatched on the feature name, and two features reach a verdict without reading the value. `hover` always answers true, and `orientation` treats anything other than `"landscape"` as portrait, so `@media ((orientation: env(safe-area-inset-top)) and (min-width: 0px))` applied its block. - `testContainerMediaCondition` compared `undefined === undefined` for `=`, so `@container ((block-size: env(safe-area-inset-top)) and (width > 0px))` applied its block on a container it never measured. Both now refuse an unresolved operand before the feature decides anything, so the comparison is false and `and` / `or` / `not` compose it as CSS requires. Forcing the operand false at the comparison is what keeps the composition right. Dropping the whole block would discard the sibling branch of an `or`, and dropping the operand alone would let the remaining branch of an `and` apply a block that asked for more than the runtime can answer - so the operand stays in the compiled condition, pinned by a compiler test.
`@media (height)` and `@container (width)` ask whether a feature has a value that is not zero. Both evaluators answered `false` for every one of them, so a block behind a boolean feature never applied. `isTruthyFeatureValue` holds that question for both planes, and each plane supplies the value: `getMediaFeatureValue` reads the viewport, the color scheme, the direction and the pixel ratio, and `getContainerFeatureValue` already reads the container. A feature neither can answer has no value and stays false, and a zero or non-finite measurement is false as well - a container that has not laid out yet does not satisfy `(width)`. `getMediaFeatureValue` also replaces the numeric lookup inside `testComparison`, so the viewport is read from one place rather than two.
…hold A media or container feature value with no compile-time answer - `env()`, a ratio, an unsupported `calc()` - compiles to a marker the runtime refuses. That marker was `undefined`, which is not a value the transport can carry: a stylesheet reaches a native bundle as JSON source text, and `JSON.stringify` writes `undefined` inside an array as `null`. An operand is an array slot, so a device never saw the marker the runtime tested for, and every block behind an unresolvable operand applied. The compiler now writes `null`, the one spelling of "no value" that survives the transport, and `MediaFeatureOperand` excludes `undefined` from the slot so a regression is a compile error rather than a device-only bug. Two things follow from the marker being the same on both planes: - The runtime refuses `null`. The container evaluator needs no refusal of its own - a feature it cannot measure has no value, `null` neither equals that nor is a number, so every comparison already answers false. - A query is no longer dropped for carrying an unresolvable operand. Dropping it removed the condition entirely, and a rule with no condition applies unconditionally, which is the opposite of refusing it. Measured on a device's shape, `@media (orientation: env(safe-area-inset-top))` applied its block everywhere. `registerCSS` injects through the same serializer Metro writes into the bundle, so a test asserts against the shape a device holds. Nothing drove that path before, which is how the marker could be wrong on every device while the suite stayed green. The container comparison operators and `containerHeightFamily` carry separate defects that I fix on fix/container-query-defects. This commit is ordered behind that branch and leaves them alone.
…urce Three media features contradicted themselves or the spec. `hover` reaches its verdict without reading the operand, so `(hover: hover)`, `(hover: none)` and `(hover)` all match at once - a combination no UA can produce. The comparison now reads the value `getMediaFeatureValue` reports, so one of the two values matches and the other does not. Which one stays a deliberate deviation from MQ5 5.1, where `none` covers a touchscreen: React Native raises `onHoverIn` / `onHoverOut` wherever a pointer exists, and a utility framework's `hover:` variant compiles to this feature, so the runtime answers `hover` on every platform. The comment says so rather than leaving it to be rediscovered. `prefers-color-scheme` had two answers that disagreed for a user who has expressed no preference: `getMediaFeatureValue` reported no value, while the comparison compared the operand against `null`. MQ5 12.5 makes `light` the answer in that case, so `getMediaFeatureValue` returns it and the comparison reads from there. `(prefers-color-scheme)` and `(prefers-color-scheme: light)` now agree. `(color)` answered false. MQ5 6.1 uses that query as its own example of one matching every color device, and React Native renders to one, so the runtime reports the conventional eight bits per component. The test that wanted a feature with no answer at all asks for `environment-blending` instead.
`containerHeightFamily` read `.width` from the layout rectangle, so every container answered its own width for both axes. That makes every container square: `width > height` is never true, so `orientation` is `portrait` for every container however it is laid out, and `aspect-ratio` is always 1. A 500x200 container now answers `landscape`, and `(height > 300px)` refuses it instead of comparing 500 against 300.
`>=`, `<` and `<=` all evaluated `left > right`, so only `>` and `=` were correct. The arms differ from the right ones by a single character each, which is why the switch reads as correct. The damage is invisible on most inputs - `500 > 100` and `500 >= 100` agree - and shows up at the boundary and in the reversed direction: `(min-width: 500px)` refused a 500px container, and `(width < 400px)` matched one. The media evaluator already had these right; only the container copy drifted.
…alse A term neither the compiler nor the runtime can decide was answered `false`. Two-valued logic then makes `not <that term>` true, so every negated term this implementation does not support applied to everything: `@media not (monochrome: 1)`, `not (fictional-thing)`, and `not (400px < width < 500px)` all matched unconditionally. MQ5 3.1 defines the three-valued logic that exists for exactly this reason - "the only reasonable value is false, but this means that `not unknown(function)` is true, which can be confusing and unwanted". `kleene.ts` implements it once and both evaluators use it, so the two cannot drift the way the range operators did. The combinators take the terms and an evaluator rather than already-evaluated values, so a decided conjunction never evaluates the rest. That is not only arithmetic here: evaluating a term reads reactive observables and reading one subscribes to it, so staying lazy keeps the subscription set to the operands that actually decided the answer. It is sound because the operand that decided the answer is itself subscribed, so the change that could revive a skipped operand is the change that re-runs the whole condition - measured in `short-circuit-subscription.test.tsx`, which drives a condition through a short-circuit and out the other side. On the compiler side, a `<container-condition>` with no representation here - `style(--foo: bar)` - now compiles to the term `["?"]` instead of being dropped. Dropping it is a different answer in three places: alone it left no condition at all, which applies inside every container; under `not` it vanished with the same result; and inside a conjunction the operand disappeared, turning `true and unknown` into `true`. Refusing to emit the block covers the first two but not the third, because the condition around the dropped operand still parses. The marker is `["?"]` rather than `undefined` for the reason the operand marker is `null`: Metro writes the stylesheet into the bundle as JSON, and `JSON.stringify` cannot carry `undefined` in either position.
… prelude
Three arms of the three-valued evaluators were load-bearing and unobserved.
Removing any of them left the whole suite green, so each was one refactor away
from being deleted as dead code.
`(width > 10em)` compiles to the length descriptor `[{}, "em", 10, 1]`, because
`em` is relative to the element's own font size and no compile-time pass can
fold it. That descriptor reaches the comparison as an operand it cannot order,
and ordering it anyway gives `NaN` - false for every operator, and false is the
one answer a negation turns into a match. The guard that answers unknown
instead was reachable from ordinary CSS the whole time; what hid it is that no
test in the suite used a relative length. `not (width > 10em)` pins the
container arm and `not all and (width > 10em)` pins the media one - the query's
`not` qualifier rather than `not (...)`, because lightningcss folds that
spelling into `(width <= 10em)` and leaves no negation to observe.
`(inline-size)` is the boolean form of a feature this runtime cannot measure,
so it is unknown rather than false, and `not (inline-size)` is the only shape
that tells the two apart.
`extractMedia` now refuses a prelude whose every query was refused. It emitted
the block with no condition, which applies it everywhere - the opposite of what
a refused prelude means, and the trade `extractContainer` already guards. The
guard cannot be an empty-`conditions` check, because an empty list is equally
what `all`, `screen` and `not print` produce, so `parseMediaQuery` returns
`ParsedMediaQuery` and says which of the three happened. No stylesheet reaches
the refused branch today; the separation is what makes the backstop safe to
hold.
The `["?"]` arms that are genuinely unreachable now say so, and say what would
make them live rather than reading as dead: the native media `case "?"` is
unreachable because lightningcss parses `@media (fictional-thing)` as a boolean
feature rather than as MQ5's `<general-enclosed>`, which is a property of the
installed parser and not of the grammar.
`fix/container-query-defects` already draws this distinction, in `7be5cf4`: `CompiledCondition` separates `always` / `never` / `condition` so `extractMedia` can tell "there is no condition" from "the condition did not compile", and it carries a compiler-plane table of four uncompilable preludes across both at-rules, six controls, runtime tables on both planes, and a mutation proof for each arm - including the two this shape has to get right, `@media not print` and `@media all`. The version here was a second name for that type with none of those tests, and it covered only the media half. Two spellings of one distinction across two branches that merge together is drift, so the concept stays where it is already proven and this branch keeps the arms it can pin on its own.
Contributor
Author
|
Cross-referencing #425 and #426, which conflict — flagging it now rather than at merge time. They are independent siblings ( The hazard is in the
Taking #426's arm wholesale therefore un-fixes #425's range container queries. Taking #425's wholesale loses #426's three-valued handling, where The resolution that is correct in both directions evaluates the interval and returns One more line worth keeping whichever way it lands: #426's boolean-context arm uses A two-case test pins both halves, and neither branch passes it alone:
|
Contributor
Author
|
Correction to my note above — I undercounted the conflict set, and the omission was the load-bearing file.
A second resolution needs the same care, in Taking #426's arm onto #425's types does not compile — ["!", query.type === "condition" ? query.condition : (["?"] as MediaCondition)]And one more, in |
…unanswerable value `parseMediaFeatureOperand`'s doc comment lists what has no compile-time answer as `env()`, a ratio, an unsupported `calc()`. On this branch that is true, and it stops being true the moment nativewind#425 lands: there a reducible ratio compiles to its quotient and only a degenerate one stays unanswerable. Naming the ratio more precisely does not fix it. A restrictive qualifier — "a ratio the compiler cannot reduce to a finite quotient" — implies a non-empty complement, and on this branch the complement is empty: every ratio, `16/9` and bare `1` included, answers `undefined`, because `case "ratio"` falls through to `case "env"`. So the qualified form is truth-conditionally fine and tells a reader of THIS branch something false about it. There is no wording that names ratios here and is right both before and after nativewind#425. Dropping the ratio is not a narrower list, it is an honest one, because the enumeration was never the case table it reads as. `parseLength` refuses 43 units by name — every physical unit, every font-relative unit but `em`/`rem`, every viewport and container variant but bare `vw`/`vh` — so `(min-width: 1lh)` already answers `null` here with no `calc()` anywhere in it, unlisted. The two members kept are the two verified stable: `env()` and an unsupported `calc()` answer `null` on this branch and after nativewind#425 alike. `such as` is what stops the next reader trusting the list as exhaustive, which is the failure this comment already had. The ratio's own explanation belongs at the hop that owns the fact, and nativewind#425 puts it there, in `case "ratio"`. Comments only.
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.
The media- and container-condition evaluators answer several questions wrongly, and one of them wrongly in a way that cannot be distinguished from a legitimate "no".
The core problem: a condition the runtime cannot decide answered
falseAn operand that failed to resolve was treated as a negative rather than as unknown.
falseand "I could not work this out" are different facts, and collapsing them means a rule silently does not apply and nothing anywhere reports why. This branch introduces the unknown state, carries the unresolved-operand marker in a shape JSON can actually hold, and refuses an operand that could not be resolved rather than guessing.The rest
@media (a), (b)is a disjunction; one refused branch dropped the whole list.(hover)/(pointer)form, as distinct from(hover: hover).hover,colorandprefers-color-schemewere answered from different places. They now come from one source, which is what stops them disagreeing.>=,<and<=previously all evaluated asleft > right.Evidence
A reviewer pair audited this branch and converged on SHIP-WITH-CHANGES; the one point they disagreed on was settled by measurement rather than by argument, and the changes they asked for are in.
The last two commits are worth reading as a pair: one pins the two condition arms no test observed, and the next deliberately hands the media-prelude backstop to the branch that actually tests it, rather than keeping a second copy here.
Relationship to the container-query-defects branch
I am submitting both, and they overlap — better said here than discovered at merge. They share 10 files and
git merge-treereports 9 conflict sites.No commit is duplicated between them: patch-id overlap is zero. But two defects — the container range operators, and the height axis — are fixed on both branches by different implementations. That is my doing, from working the two problems in parallel.
Tell me which you would rather take first and I will rebase the other onto it, or collapse the overlapping commits into one branch. I would rather restructure than hand you nine conflicts.