1954142 - StyleResolver resolve_style should account for element-backed pseudo elements

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task)

Description

[Luke Warlow]

This is needed for ::details-content support.

Comment 1

[Luke Warlow]

I've tried to do this on top of my parser change (which does seem to mostly work as CSS.supports('selector(details::details-content::before)') now returns true. I changed the check to if !self.element.implemented_pseudo_element().is_some_and(|p| !p.is_element_backed()) { it doesn't fix the resolution of the styles for ::before etc. I'm assuming it needs more changes to the logic for resolution, would you be able to point me in the right direction?

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Yeah so most likely you're not finding the right rules for the ::before pseudo.

We bucket selectors by pseudo-element, so if you have something like ::details-content::before, see PerPseudoElementMap and so on in stylist.rs

For ::part() we do something a bit subtler (see PartElementAndPseudoRules). I suspect we need to either treat element-backed pseudos more like part, or generalize our lookups to support nested pseudo-elements more generally.

The later implies making rule_collector.rs deal with these maps recursively, probably. I haven't dug into this, but that probably means having rule_collector.rs keep the rule_hash_target (which should be the top-level originating element, so <details> in this case), but multiple pseudo-elements [::details-content, ::before], rather than just one). I think per spec we're supposed to support stuff like ::before::marker and so on, so that seems like a better approach.

So TLDR the way I'd go about this would be:

• Change GenericElementAndPseudoRules to support nested pseudo maps (so probably something like Box<Self> rather than Box<Map>).
• Teach stylist to walk the multiple pseudo-elements that you parse to store the Rule nto the right map. So for details::details-content::before, it should be in the ::details-content map of the ::before pseudo.
• Teach RuleCollector to carry multiple pseudo-elements (so probably make this a SmallVec<[&'a PseudoElement; 1]> or so).
• Teach RuleCollector to look up using the (rule_hash, pseudo_element) into the right map.

Or something like that. Maybe we can, as a simpler version of this, keep the data structures as they are, but put the rules into the "global" rules, but that'd mean that ::details-content::before would be matched for every ::before pseudo-element, which is a bit unfortunate. Maybe fine tho.

Hope that helps?

Comment 4

[一丝]

I found that Firefox does not currently support ::details-content::before::marker

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to 一丝 from comment #4)

I found that Firefox does not currently support ::details-content::before::marker

Yup -- unless I'm misunderstanding, I think that's precisely the sort of thing that this bug is addressing. (At least :details-content::before, and I assume ::marker as well, though maybe emilio or Luke can confirm if that's got some distinct/additional specialness outside the scope of this bug.)

Interop test relevancy note: I think this bug is what's needed in order for us to pass this WPT (which uses details::details-content::before & ::after):
https://wpt.fyi/results/html/rendering/the-details-element/details-pseudo-elements-004.html
test: https://wpt.live/html/rendering/the-details-element/details-pseudo-elements-004.html
ref: https://wpt.live/html/rendering/the-details-element/details-pseudo-elements-004-ref.html

And probably also this one (which uses details::details-content::first-letter & ::first-line):

https://wpt.fyi/results/html/rendering/the-details-element/details-pseudo-elements-005.html
test: https://wpt.live/html/rendering/the-details-element/details-pseudo-elements-005.html
ref: https://wpt.live/html/rendering/the-details-element/details-pseudo-elements-005-ref.html

(Right now, with layout.css.details-content.enabled toggled to true, we fail both of those^ tests.)

Comment 6

[Daniel Holbert [:dholbert]]

Hi Luke! Just checking in, are you planning on taking this one? (No worries if not, but if so let's mark you as the assignee to reflect reality.)

Thanks for all your recent help in this details/details-content area!

Comment 7

[Luke Warlow]

Attachment: Bug 1954142 - Fix StyleResolver to account for element-backed pseudo elements. r=emilio

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ea191bab6e21
https://hg.mozilla.org/integration/autoland/rev/229f3f836fa1
Fix StyleResolve to account for element-backed pseudo elements; r=emilio

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a1526a2c274c
https://hg.mozilla.org/integration/autoland/rev/c61d7c5764c8
Revert "Bug 1954142 - Fix StyleResolve to account for element-backed pseudo elements; r=emilio" for causing mass failures

Comment 10

[pstanciu]

Backed out for causing mass failures

• https://hg.mozilla.org/integration/autoland/rev/c61d7c5764c840cee1dfc7a46098bd9cf6f582b4
• Push with failures
• Failure Log Wr16
• Failure Log bc14
• Failure Log ba2
• Failure Log ba
• Failure Log Wr1

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a37bdf8dcd16
https://hg.mozilla.org/integration/autoland/rev/f4f540e39db0
Fix StyleResolve to account for element-backed pseudo elements; r=emilio

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9d515fcf2689
https://hg.mozilla.org/integration/autoland/rev/b95f29d53059
Revert "Bug 1954142 - Fix StyleResolve to account for element-backed pseudo elements; r=emilio" for causing failures at details-pseudo-elements-005.html.

Comment 13

[Atila Butkovits]

Backed out for causing failures at details-pseudo-elements-005.html.

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/b95f29d530590921e31991be6a80da8400881013

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f4f540e39db0ef832eb489456cfb62494c41acbf&selectedTaskRun=QGVm96rxRm2lRDCEn3hfFQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=520398796&repo=autoland&lineNumber=3269

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5616f3357073
https://hg.mozilla.org/integration/autoland/rev/3d7c0b3d2d0c
Fix StyleResolve to account for element-backed pseudo elements; r=emilio

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a30ff5eb040c
https://hg.mozilla.org/integration/autoland/rev/951307407cf2
Revert "Bug 1954142 - Fix StyleResolve to account for element-backed pseudo elements; r=emilio" for causing multiple wr failures.

Comment 16

[agoloman]

Backed out for causing multiple wr failures.

• Backout link
• Push with failures
• Failure Log

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ad088c223ff2
https://hg.mozilla.org/integration/autoland/rev/f5b95b5c2423
Fix StyleResolver to account for element-backed pseudo elements. r=keithamus

Comment 18

[Cristina Horotan [:chorotan]]

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

Comment 19

[Florin Bilt]

(In reply to pstanciu from comment #10)

Backed out for causing mass failures

• https://hg.mozilla.org/integration/autoland/rev/c61d7c5764c840cee1dfc7a46098bd9cf6f582b4
• Push with failures
• Failure Log Wr16
• Failure Log bc14
• Failure Log ba2
• Failure Log ba
• Failure Log Wr1

Perfherder has detected a awsy performance change from push c61d7c5764c840cee1dfc7a46098bd9cf6f582b4.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
0.07%	Base Content Explicit	windows11-64-24h2-shippable	fission	8,207,227.00 -> 8,201,523.33

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 46141

The following documentation link provides more information about this command.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Florin Bilt from comment #19)

Perfherder has detected a awsy performance change from push c61d7c5764c840cee1dfc7a46098bd9cf6f582b4.

That push was a backout, FWIW. Either it was noise (0.07% delta seems maybe-random) or it was an indication that this bug's actual patch was a very-slight regression here (and the benefit from the backout would presumably have been undone by the re-landing).

Florin, can you tell whether that benefit was balanced by regressions from the original landing (comment 14) and the eventual re-landing (comment 17)?

Comment 21

[Daniel Holbert [:dholbert]]

(or maybe it was just noise; not sure it's worth spending time on. 0.07% seems pretty inconsequential whether it's a benefit or a regression.)

Comment 22

[Mayank Bansal]

1. First regression : Pushlog which points to this bug
2. Return to baseline : Pushlog which correxposnds to the backout
3. Second regression: Pushlog pointing to this bug.
4. Return to baseline: Pushlog which contains the backout of this bug
5. Third regression: Pushlog which points to this bug.

Comment 23

[Florin Bilt]

Thanks Mayank
Also, you check this graph.

Comment 24

[Daniel Holbert [:dholbert]]

Thanks, Mayank and Florin! Sounds like this bug's patch made us use ever-so-slightly more memory (0.1% more) for an empty content process.

Judging by the commit message (which indicates that it's making us account for something we were previously ignoring), that seems probably fine and unsurprising. I'll defer to Emilio if he chooses to be worried by this, though. :)

(Looking at the documentation on this measurement, it looks like it's mostly there to watch for added content/JS that might get loaded synchronously at startup and hence gets forced to be included in this base amount of memory. That makes sense as something to watch for; and that's not the sort of thing that happened here -- rather, this just made the stuff-that-already-gets-loaded very slightly more expensive, for correctness reasons -- so I think we're OK.)

Comment 25

[Emilio Cobos Álvarez [:emilio]]

We should fix this tho, the new entries do waste a fair bit of space. Bug 1981540 is where the investigation is happening.

Comment 26

[Daniel Holbert [:dholbert]]

OK! Thanks.