1926164 - CSS styles under :not(:not(:x-of-type)) does not apply unless "flushed"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brendon]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0
Firefox for Android

Steps to reproduce:

1. On a fresh instance, open: https://ed7n.github.io/net-viewer
2. Toggle the menu item: View -> Controls
3. Resize the viewport to toggle between desktop and mobile modes.

Actual results:

1. The control panel does not disappear on startup.
2. Toggling said menu item does not affect the visibility of the panel.
3. Resizing the viewport fixes the issue, and it persists when resizing back.

Expected results:

1. The control panel should disappear on startup.
2. Toggling said menu item should affect the visibility of the panel.

Comment 1

[Brendon]

The CSS definition of interest:

article.no-panels
  > section:not(:first-of-type):not(:last-of-type)
  > section:not(:not(:first-of-type):not(:last-of-type)) {
  display: none;
}

The Controls menu item toggles the .no-panels class in the article element.

In addition to resizing the viewport to toggle the CSS media rules,
if one were to duplicate the above definition with the Style Editor, then the style applies as it should.

It seems that the style is applied only if some sort of style cache is rebuilt.

Comment 2

[Brendon]

For reference, the behavior is correct as of Chromium, Version 129.0.6668.100 (Official Build, ungoogled-chromium) (64-bit).

Comment 3

[Brendon]

For reference:

• The behavior is likewise incorrect on Fennec F-Droid, 129.0.2 (Built #1290220).
• The behavior is correct on Chromium, Version 129.0.6668.100 (Official Build, ungoogled-chromium) (64-bit).
• The behavior is correct on Android System WebView, 129.0.6668.100.

Comment 4

[Gregory Pappas [:gregp]]

got this range https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=272d7188fe71c94facdcd15027c7b5385863315f&tochange=2d1b642cb391ae3675fd3960fdeec82b5706a2d1

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1837351

:zrhoffman, since you are the author of the regressor, bug 1837351, could you take a look?

For more information, please visit BugBot documentation.

Comment 6

[Tess Hoffman [:zrhoffman]]

Attachment: testcase

Thanks for pointing to the problematic CSS selector, it made coming up with a reduced testcase much easier!

Confirmed that commenting out the if statement that skips positional pseudo-class matching during invalidation (https://searchfox.org/mozilla-central/rev/faa7b1b2a7b5/servo/components/selectors/matching.rs#1311-1318) makes the testcase work as expected (with a significant performance cost).

David has significantly improved positional pseudo-class invalidation and expanded on that check more recently than bug 1837351. David, do you know what's making invalidation fail in bug 1926164? :)

Comment 7

[Tess Hoffman [:zrhoffman]]

Attachment: 1926164-testcase.html

Added a testcase with more delay to keep the testcase from being intermittent.

Comment 8

[David Shin[:dshin] (PTO Til 17th)]

Attachment: Further minified test case

Hrm, that's unfortunate.
In the context of invalidation, we pretend these indexed selectors always match, so that the invalidation machinery just marks them for invalidation without doing expensive indexing math (At the cost of over-invalidation).
We account for :not by flipping the result so that it returns True in the context outside of that negation. The problem is of course there being another negation, which flips the result to False and we skip invalidating it.
For these selectors, we need :not(:not(<...>:first-of-type<..>)) to return matching regardless of the depth of nesting.
This sliped through because we only do matching with potential unknown results when we move on invalidating from an inner selector to an outer selector e.g. In .foo :is(.bar .baz) .baa:first-of-type, we invalidate on .bar class change down to .baz, then check to see if the outer selector's matching result changes.

EDIT: Whoops, I put one too many :not there to make it work. Basically, nesting odd numbers of :not will make it work, even numbers will not.

Comment 9

[Tess Hoffman [:zrhoffman]]

(In reply to David Shin[:dshin] from comment #8)

Basically, nesting odd numbers of :not will make it work, even numbers will not.

Yep you are right, hard-coding in_negation as true was breaking invalidation. https://searchfox.org/mozilla-central/rev/6ec81d7b1d2f/servo/components/selectors/context.rs#365

Patch incoming...

Comment 10

[Tess Hoffman [:zrhoffman]]

Attachment: Bug 1926164 - Negate in_negation when nesting r=dshin,#style

in_negation was true in cases where it should be false, like
:not(:not(...)).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecd7dce029ce
Negate in_negation when nesting r=dshin,jwatt

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/48823 for changes under testing/web-platform/tests

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ecd7dce029ce

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this worth an ESR128 uplift? Not sure how common this is in the wild.

Comment 17

[Tess Hoffman [:zrhoffman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Is this worth an ESR128 uplift? Not sure how common this is in the wild.

Not sure how common it is in the wild, but because the fix is so trivial, it wouldn't hurt.

Comment 18

[Tess Hoffman [:zrhoffman]]

Comment on attachment 9432720 [details]
Bug 1926164 - Negate in_negation when nesting r=dshin,#style

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: One-liner fix.
• User impact if declined: comment 0
• Fix Landed on Version: 133
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial fix.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9432720 [details]
Bug 1926164 - Negate in_negation when nesting r=dshin,#style

Approved for 128.6esr.

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/2ef9c3c61099