Closed Bug 1544242 Opened 6 years ago Closed 6 years ago

Assertion failure: !IsLazilyCascadedPseudoElement() (Lazy pseudos can't inherit lazy pseudos) with ::marker { display: list-item }

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(
URL
)

Details

Attachments

(3 files)

Bug 1544242 - Check iterator length in SelectorIter::is_featureless_host_selector. r=heycam,mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1544242 - Cleanup selector-matching for nested pseudo-elements, match ::slotted correctly when there's no selector before it, and add tests. r=heycam,mats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1544242 - Add a test for a bug that this inadvertently fixes. r=heycam
47 bytes, text/x-phabricator-request
Details | Review

Follow-up to bug 1543758, the following test-case asserts:

<!doctype html>
<style>
  li::marker {
    display: list-item;
  }
  li + li::marker {
    content: counters(list-item, ".");
  }
</style>
<ol>
  <li>Foo
  <li>Bar

This asserts because we don't expect pseudo-elements to have other pseudo-elements. We were already doing that for ::before and ::after (as in, probing the ::marker pseudo-style on a ::before or ::after).

We weren't asserting for that case though. Probing such an style will always return null (since nested pseudo-elements cannot be targeted by selectors), so we should probably just not assert and return null on probe, and assert on non-probe.

Flags: needinfo?(emilio)

So the good thing is that the assertion is there because of something that no longer exists (which is that cached inheriting pseudos used to be in a linked list).

But our behavior here is quite a bit bizarre... Take the following:

<!doctype html>
<style>
  ul { margin-left: 100px; }

  ::marker {
    display: list-item;
    content: "Foo"
  }
</style>
<ul>
  <li>Bar

Right now we render a bullet, and two Foo strings. That's insane. If only, it only works by chance (if we decide to match the ::marker selector even for the anonymous marker then we'd just recursively create infinite markers). Mats, my expectation is that ::marker shouldn't be allowed to create other markers. Does that match yours?

::before and ::after can create markers, but I'm still undecided about whether they should be targeted by the global selector... WebKit does show two red markers here as well:

<!doctype html>
<style>
  ul { margin-left: 100px; }

  li::after {
    content: "After";
    display: list-item;
  }
  ::marker {
    color: red;
  }
</style>
<ul>
  <li>Bar
Flags: needinfo?(mats)

Mats, my expectation is that ::marker shouldn't be allowed to create other markers. Does that match yours?

I thought ::marker was identical to *::marker and since * doesn't
match pseudos it shouldn't match the ::after's marker. You'd need to
write ::after::marker for that, but that's not valid syntax AFAIK.

So, you can create a marker for the ::after by setting display:list-item
on it, but you can't address that marker with a selector. So you can create
a ::marker on all pseudos (including ::marker), but only one level since
there's no way to set display on any deeper markers.

(I haven't read the Selectors spec in a long time though, so I could be wrong.)

Flags: needinfo?(mats)

Yes, that's right, but that's not our current behavior. We end up matching the global ::marker rules. I agree that's wrong.

However if we don't match rules like that, then ProbePseudoElementStyle will return null and thus we'll generate no marker at all.

Even if it didn't, the UA rule for marker would not work: https://searchfox.org/mozilla-central/rev/0376cbf447efa16922c550da3bfd783b916e35d3/layout/style/res/ua.css#130

So I think the best behavior is making pseudo-elements don't create markers to begin with.

I have a patch to fix the selector-matching here, but it's a bit gross...

Found a way which wasn't as gross, though required a couple other fixes.

Assignee: nobody → emilio
Flags: needinfo?(emilio)

I'm going to unconditionally generate the PseudoElement combinator, and this
causes issues since we'll put the raw ::pseudo selectors in the host bucket,
which is obviously wrong.

We always include the combinator for pseudo-elements now (not including it was
just an optimization) in order to not match when nested pseudo-elements are
involved.

We could add a more generic check in matches_simple_selector like:

if element.is_pseudo_element() {
    match *selector {
        Component::PseudoElement(..) |
        Component::NonTSPseudoClass(..) => {},
        _ => return false,
    }
}

But even that wouldn't be enough to make selectors like :hover::marker not
match on the ::before::marker pseudo-element, plus that code is really hot.

So for now do the check on the next_element_for_combinator function. It's a
bit hacky but it's the best I could came up with...

While at it, simplify some checks to use is_pseudo_element() instead of
implemented_pseudo_element() directly.

Only the Rust patch as-is would make markers for ::before and ::after on list
items not show up, so we also need to switch ::marker to use ProbeMarkerStyle()
rather than ProbePseudoElementStyle(), since the marker should exist even if it
matches no rules.

Depends on D27528

Priority: -- → P3
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/961d9f01940b Check iterator length in SelectorIter::is_featureless_host_selector. r=heycam
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c89fd37b79d Fix selector-matching for nested pseudo-elements. r=heycam,mats

https://hg.mozilla.org/mozilla-central/rev/961d9f01940b

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Actual fix was backed out.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See Also: → 1554009

Always generating the slot assignment combinator means that we check the current
host correctly.

Attachment #9058301 - Attachment description: Bug 1544242 - Fix selector-matching for nested pseudo-elements. r=heycam,mats → Bug 1544242 - Cleanup selector-matching for nested pseudo-elements, match ::slotted correctly when there's no selector before it, and add tests. r=heycam,mats
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1becf7e81202 Cleanup selector-matching for nested pseudo-elements, match ::slotted correctly when there's no selector before it, and add tests. r=heycam,mats
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16986 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c53283a32df Add a test for a bug that this inadvertently fixes. r=heycam
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/6c59557f4033 Give up on a test that's too fuzzy on win and OSX for now. CLOSED TREE
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

https://hg.mozilla.org/mozilla-central/rev/1becf7e81202
https://hg.mozilla.org/mozilla-central/rev/4c53283a32df
https://hg.mozilla.org/mozilla-central/rev/6c59557f4033

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla68 → mozilla69
Flags: needinfo?(emilio)

I assume this can ride the trains, feel free to reset status if not.

status-firefox68: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: