Tighten two loose guards, and correct ADR-0142's no-JS-runner claim #202
Loading…
Reference in a new issue
No description provided.
Delete branch "tighten-guards-and-correct-0142"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Tasks 258 and 259.
258 — two source-text guards that did not discriminate.
WideCentredModalBoundTestsnow checkseach comma-separated selector member, not the selector text as a whole;
.modal-overlay-panel-wide, .modal-overlay-panel-centredpassed the old check while handing the 90vw/90vh ceiling to everyconfirmation.
ModalOverlayCloseButtonStackingTestsnow captures thez-indexvalue and requires itpositive;
z-index: -1kept it green while re-burying the close button under the image. Both watchedred against exactly those evasions, and the first watched green against its own pre-tightening form to
confirm the evasion was real.
259 — ADR-0142 claimed JS behaviour could not be verified here. Corrected in place, in the
Corrected:+ inline-blockquote form of PR #200/#201, keeping two claims apart: no JS unit-testrunner and no
package.json(true), versus no way to verify JS behaviour (false — thee2ejob runsreal Chromium on every PR). ADR-0167 had already disavowed the borrowed claim but called it "simply
false", one word too broad; it now carries a matching correction pointing back at ADR-0142.
What ADR-0142 declined on the false grounds is now covered:
SharePlaceJourneyTestsrunsshareOrCopyAsyncin real Chromium, proving the clipboard branch end to end (the URL is read back offthe system clipboard) and the
window.isSecureContext ? "failed" : "insecure"expression with bothAPIs removed. Both watched red against a mutated
share.js. The"insecure"return stays uncovered,now for the reason that actually holds — the fixture serves
http://localhost:5169, a potentiallytrustworthy origin.
No
src/changes; WebUI coverage is unaffected.Verdict: changes needed
The
var()narrowing is the right call — this button's1is a local stacking lift inside the panel's own context, not a rung of ADR-0156's ladder, so an integer literal is the correct form to require and a token really would put the sign out of this test's sight. Two consequences of it need work: the regex enforces it only when a token replaces the integer, not when one overrides it, and the message avar()trips misdescribes what happened. PlusMatchwhereMatchesis needed, and one gap left by 259's own acceptance criteria.ADR wording lands on the distinction in both records and the pointers resolve both ways; no findings there.
CI: run #836 for
8ce2185was still in flight at review time, so I have not seen it green. It needs to be green for the head that carries these fixes.@ -88,0 +103,4 @@/// move the sign into a token this test cannot read, and so would reopen the same hole./// The greedy prefix lands on the rule's last <c>z-index</c>, which is the one CSS itself applies./// </summary>[GeneratedRegex(@"\.modal-overlay-close\s*\{[^}]*position:\s*relative[^}]*z-index:\s*(?<zIndex>[+-]?\d+)")]The greedy prefix finds the last integer-valued
z-index, not the rule's last one.z-index: 1; z-index: var(--z-x);backtracks onto the1and passes while the cascade applies the token — the same hole the narrowing exists to close, entered by overriding rather than replacing. Either read the rule body and check its finalz-indexdeclaration whatever its form, or drop the claim on line 104.@ -93,3 +112,3 @@var css = Solution.ReadFile(ModalOverlayCssRelativePath);CloseButtonIsLiftedAbovePanelContent().IsMatch(css).ShouldBeTrue(var lift = CloseButtonIsLiftedAbovePanelContent().Match(css);Matchtakes only the first.modal-overlay-closerule declaringposition: relative. A later, more specific rule — this stylesheet already carries.modal-overlay-panel-wide .modal-overlay-closefor the same button — settingz-index: -1leaves this green. UseMatchesand require every match positive.@ -95,1 +114,3 @@CloseButtonIsLiftedAbovePanelContent().IsMatch(css).ShouldBeTrue(var lift = CloseButtonIsLiftedAbovePanelContent().Match(css);lift.Success.ShouldBeTrue(A
var()value now fails here with a message saying the button "is a bare float again", sending the reader to look for a declaration that is right in front of them. Keep the narrowing, but name it in the message: integer literal required, andZIndexComputedStackingJourneyTests(realgetComputedStyle, resolves the token) is the route if this value is ever tokenised.@ -0,0 +27,4 @@/// rehosting the fixture on a non-loopback address for every journey in it. What the second test/// below does reach is the other half of that same expression, with a real/// <c>window.isSecureContext</c> deciding it. <c>PlaceDetailPanelTests</c> still carries the/// <c>"insecure"</c> outcome's own message mapping, which needs no browser.PlaceDetailPanelTestsproves the mapping from"insecure", not thatshare.jsstill produces it. Collapse the ternary toreturn "failed";and this class, that class and the rest of the suite stay green while task 226's headline fix is silently gone. Task 259's own acceptance criteria ask for a source-text guard on that branch — aGeolocationSecureContextFirstTests-shaped check thatshare.jsstill readswindow.isSecureContextin code.All four actioned in
c94ed73.1+2. The stacking guard no longer uses a single regex over the whole file. It collects every rule whose selector names
.modal-overlay-closeand reads that rule's lastz-indexdeclaration whatever its form, then requires each to be a positive integer literal. Watched red against:z-index: -1added to.modal-overlay-panel-wide .modal-overlay-close;z-index: 1; z-index: var(--z-x);in the shared rule;z-index: var(--z-overlay)alone;z-index: -1 !important; the lift deleted; andposition: relativedeleted with thez-indexkept. The line-104 claim is gone with the regex it described.The narrowing message now names it — not an integer literal, prove it through
ZIndexComputedStackingJourneyTests(realgetComputedStyle, resolvesvar()) and drop the rule from the tripwire.ShareInsecureContextReportedTestsadded,GeolocationSecureContextFirstTests-shaped. Two checks:window.isSecureContextin comment- and string-stripped code,"insecure"in comment-stripped-but-string-kept code. Red againstreturn "failed";, againstwindow.isSecureContext ? "failed" : "failed", and againstreturn "insecure";with only the doc comment still naming the read.Residual evasion, confirmed by running it rather than reasoned about: moving
position: relative; z-index: 1inside an@media printblock keeps the stacking guard green — it reads the nested rule by its own selector and cannot tell the query never matches. Recorded in the class remarks alongside the other blind spots (a selector naming no class, another stylesheet raising a sibling instead), with the reason no numbered inventory is kept.Verdict: mergeable
Re-review at
c94ed73, superseding review 328 (8ce2185). All four findings actioned; nothing new blocks.The
@mediablind spot is right to leave open. The guard exists for the accidental edit — the sign flipped, the lift deleted, a more specific rule added later — and relocatingposition: relative; z-index: 1into an at-rule is a deliberate restructuring, not an accident. It is recorded as a shape rather than an inventory, with the reason the inventory is not kept, and withZIndexComputedStackingJourneyTestsnamed as where a resolved answer exists. That is the honest form. One wording point inline: it is not a@mediahole, it is an at-rule hole.The six mutations do what is claimed. The
-1on the later wide rule reddens the second test because that test reads every rule with az-indexregardless ofposition;z-index: 1; z-index: var(--z-x)reddens becauseEffectiveZIndextakes the last declaration whatever its form rather than the last parseable one; the lift deleted andposition: relativedeleted redden the first test and only the first, which is the correct split. Not over-strict: the current stylesheet's other two.modal-overlay-closerules carry noz-indexand are skipped, and a value the check cannot read fails with a message that names the narrowing and points at the computed-style route.ShareInsecureContextReportedTestsdiscriminates for the right reason in both directions — comments stripped for thewindow.isSecureContextread, comments stripped but strings kept for"insecure", which is exactly the decoyGeolocationSecureContextFirstTestswas found accepting — and the ADR now saysPlaceDetailPanelTestsproves the mapping from the outcome rather than that anything still produces it.CI: run #838 for
c94ed73is stillrunning(started 14:02 UTC, six minutes ago). Not seen green — this verdict is on the code, and the merge still waits on that run.@ -83,0 +106,4 @@/// It finds the button by the class name in the selector text, so a rule reaching the same element by/// some other route (a structural selector naming no class of its own) is invisible to it, as is/// anything in another stylesheet raising a sibling above the button rather than lowering the button/// itself, and as is the lift moved inside a media query — read here by its own selector, with no wayNon-blocking.
CssRulematches the innermost brace pair, so this is not specific to@media—@supports,@layer,@containerand@scopehide their condition in exactly the same way. Naming the mechanism (any at-rule wrapper) rather than one at-rule is the general statement, not the inventory you are right to avoid.Also worth knowing before the claim that it cannot be closed without a parser hardens: brace depth at
match.Indexin the comment-stripped text closes the whole family in about three lines — depth > 0 means the rule is nested inside something this check cannot evaluate, so fail with that as the reason. Every.modal-overlay-closerule in the file today is at depth 0, so it changes nothing until someone moves one.The reviewer is right and my previous comment was wrong on both counts — it named
@mediaalone, and claimed closing it needed a parser. Closed in3445dcd, two lines:The lift-exists test now requires an unconditional rule; the positive-
z-indextest still checks every rule at any depth, since a conditional rule can still bury the button when its condition holds. Watched red with the lift moved inside@media print,@supports (display: grid),@layer overlaysand@container (min-width: 1px), and separately withz-index: -1added inside@media (min-width: 1px)— the last confirming the depth check did not make the second test blind to conditional rules. The six earlier mutations all still redden, and the real stylesheet is green.The class remarks now record the at-rule family as checked rather than as a blind spot, and note that the first description of it was wrong in both directions.
This supersedes my "cannot be done without becoming a parser" claim; please disregard it.
Verdict: mergeable
Re-review at
3445dcd, superseding review 330 (c94ed73). Nothing to action.The split between the two tests is right, and for a reason worth keeping: the lift is an existence claim, so it has to hold unconditionally — a lift that applies only when a query matches does not establish that the way out of a modal is always clickable — while a lowered or unreadable
z-indexis a prohibition, so it has to hold everywhere, because a rule that applies only sometimes still buries the button on those occasions. That is why the positive-integer test deliberately ignoresIsUnconditional, and thez-index: -1inside@media (min-width: 1px)mutation is the assertion that the two have not collapsed into one behaviour.The depth check has not blinded the lift test to anything. Its predicate is now strictly stronger and the assertion is still "not empty", so the set of stylesheets that pass is a strict subset of the set that passed before — the change can only make it redden more readily, never less. The count itself lands where the remarks say:
CssRule's match begins after the enclosing{, so an at-rule wrapper leaves exactly one unmatched brace behind it, and stripping comments first is what keeps a brace inside a comment out of the count.The retraction is accurate in both directions it claims and does not reach for a third: it names the family rather than
@mediaalone, withdraws the parser claim, and — the part that matters — drops the at-rule item from the best-effort list now that it is closed, instead of leaving a description of a hole that no longer exists. The remaining two entries are still the honest ones, still a shape rather than an inventory.CI, as it stands at 14:16 UTC: #838 (
c94ed73) iscancelled, ended 14:12:48 exactly as this push superseded it, so that head never went green. #841 (3445dcd) isrunning, started 14:12:48 — about three minutes ago, not forty. Today's two completed runs took 9m34s and 11m27s, so it is not yet due, and #839 and #840 arerunningalongside it. Merge still waits on #841.3445dcd3f1823725ff8fVerdict: mergeable
Re-review at
823725f(rebased onto04ef274), superseding review 331 (3445dcd). Nothing to action.The rebase lost nothing. Same five files, and the four I had already fixed counts for are unchanged — ADR-0142 68/0, ADR-0167 16/0,
ShareInsecureContextReportedTests75/0,SharePlaceJourneyTests108/0.ModalOverlayStylesheetTests.csis 147/12 against the new base, and I sampled it in four places rather than trusting the total: the ceiling guard's per-selector-member loop, the at-rule paragraph in the remarks — at the same line numbers as before the rebase — the positive-z-indexfailure message, andEffectiveZIndexwithCloseButtonRules' brace count. All verbatim.Both anchors are intact, which was the thing worth checking.
ModalOverlay.razor.cssat this head is byte-identical to what I read at8ce2185:.modal-overlay-panel-centred.modal-overlay-panel-widestill carries the 90vw/90vh ceiling, and.modal-overlay-closestill declaresposition: relative; z-index: 1at the top level, with the@media (prefers-reduced-motion: reduce)block closed above it — so the brace count reads balanced there and the real stylesheet passes on the unconditional path rather than by accident. #201 did not touch this file. No name collides either: this branch declaresWideCentredModalBoundTests,ModalOverlayCloseButtonStackingTests,ShareInsecureContextReportedTestsandSharePlaceJourneyTests, none of which #201 added.CI, read from the jobs endpoint rather than the run: #848 (
/actions/runs/756) reportsbuild,e2eandcontainer-imagesallsuccess. The run-level status still saysrunning— that is the lag, not the state.