Assertion failure: !IsLazilyCascadedPseudoElement() (Lazy pseudos can't inherit lazy pseudos) with ::marker { display: list-item }
Categories
(Core :: Layout, defect, P3)
Tracking
()
People
(Reporter: emilio, Assigned: emilio)
References
()
Details
Attachments
(3 files)
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.
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 1•5 years ago
|
||
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
|
|
Assignee | |
Comment 2•5 years ago
|
||
|
||
Comment 3•5 years ago
|
||
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.)
|
|
Assignee | |
Comment 4•5 years ago
|
||
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.
|
|
Assignee | |
Comment 5•5 years ago
|
||
I have a patch to fix the selector-matching here, but it's a bit gross...
|
|
Assignee | |
Comment 6•5 years ago
|
||
Found a way which wasn't as gross, though required a couple other fixes.
|
|
Assignee | |
Comment 7•5 years ago
|
||
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.
|
|
Assignee | |
Comment 8•5 years ago
|
||
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
|
||
Updated•5 years ago
|
|
||
Comment 10•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/961d9f01940b Check iterator length in SelectorIter::is_featureless_host_selector. r=heycam
|
|
||
Comment 11•5 years ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8c89fd37b79d Fix selector-matching for nested pseudo-elements. r=heycam,mats
|
||
Comment 12•5 years ago
|
||
Backed out changeset 8c89fd37b79d (Bug 1544242) for failures in browser_rules_shadowdom_slot_rules.js CLOSED TREE
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=8c89fd37b79d353a6eb8794f3de11151f6924b26&selectedJob=243984434
Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243984434&repo=autoland&lineNumber=17850
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243984428&repo=autoland&lineNumber=841
Backout: https://hg.mozilla.org/integration/autoland/rev/c7a22f0ea7b4ce3ff379f5be1d1ad3070aa76317
|
|
||
Comment 13•5 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 14•5 years ago
|
||
Actual fix was backed out.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 15•5 years ago
|
||
Always generating the slot assignment combinator means that we check the current
host correctly.
|
||
Updated•5 years ago
|
|
|
||
Comment 16•5 years ago
|
||
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.
|
|
||
Comment 19•5 years ago
|
||
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.
|
|
||
Comment 21•5 years ago
|
||
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.
|
||
Comment 23•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/1becf7e81202
https://hg.mozilla.org/mozilla-central/rev/4c53283a32df
https://hg.mozilla.org/mozilla-central/rev/6c59557f4033
Upstream PR merged
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 25•5 years ago
|
||
I assume this can ride the trains, feel free to reset status if not.
Description
•