1697998 - Dynamic ::marker 'content' change doesn't update layout

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase #1

STEPS TO REPRODUCE

1. load the attached testcase

EXPECTED RESULTS
"PASS"

ACTUAL RESULTS
"FAIL"

Zooming in a few steps makes the right text appear.

(We have some a11y list-style-type tests that starts failing when I create proper generated content for them in bug 1542807.)

Comment 1

[Mats Palmgren (inactive)]

So, I debugged this for a bit and it looks like we never get a call to RestyleManager::ProcessPostTraversal for this change. After zooming in a few times we do get it though, so it seems the style system is aware of this change but don't propagate it until something else happens.

Fwiw, if I make ::marker an "eager" pseudo by adding it to IsEagerlyCascadedInServo and EAGER_PSEUDOS then the bug disappears.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I can poke.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

[rr 88393 59171]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::state_and_attributes^[[0m^[[38;5;8m]^[[0m Collecting changes for: <ol id="list" class="on"> (0x7fbe9c603670)
[rr 88393 59175]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::state_and_attributes^[[0m^[[38;5;8m]^[[0m  > class changed: +[Atom(0x7fbe9dcf1440, on)] -[]
[rr 88393 59179]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::state_and_attributes^[[0m^[[38;5;8m]^[[0m  > attributes changed, old:  id="list"
[rr 88393 59183]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::state_and_attributes^[[0m^[[38;5;8m]^[[0m TreeStyleInvalidator::scan_dependency(<ol id="list" class="on"> (0x7fbe9c603670), Dependency { selector: Selector(.on ::marker, specificity = 0x401), selector_offset: 3, parent: None })
[rr 88393 59187]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m Collected invalidations (self: false): 
[rr 88393 59191]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m  > self: 0, []
[rr 88393 59195]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m  > descendants: DescendantInvalidationLists { dom_descendants: [Invalidation()], slotted_descendants: [], parts: [] }
[rr 88393 59199]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m  > siblings: 0, []
[rr 88393 59203]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m StyleTreeInvalidator::invalidate_descendants(<ol id="list" class="on"> (0x7fbe9c603670))
[rr 88393 59207]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m  > DescendantInvalidationLists { dom_descendants: [Invalidation()], slotted_descendants: [], parts: [] }
[rr 88393 59211]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m TreeStyleInvalidator::process_invalidation(<li> (0x7fbe9c603700), Invalidation(), Descendant(Dom))
[rr 88393 59215]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m  > Invalidation matched, next: Invalidation(::marker), (PseudoElement)
[rr 88393 59219]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m StyleTreeInvalidator::invalidate_descendants(<li> (0x7fbe9c603700))
[rr 88393 59223]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m  > DescendantInvalidationLists { dom_descendants: [Invalidation(::marker), Invalidation()], slotted_descendants: [], parts: [] }                                                                                                                                                                                                             
[rr 88393 59227]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m TreeStyleInvalidator::process_invalidation(<_moz_generated_content_marker> (0x7fbe9c603790), Invalidation(::marker), Descendant(Dom))
[rr 88393 59231]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m  > Invalidation matched completely
[rr 88393 59235]^[[0m^[[38;5;8m[^[[0m2021-03-14T01:19:33Z ^[[0m^[[34mDEBUG^[[0m style::invalidation::element::invalidator^[[0m^[[38;5;8m]^[[0m TreeStyleInvalidator::process_invalidation(<_moz_generated_content_marker> (0x7fbe9c603790), Invalidation(), Descendant(Dom))

We match the invalidation alright, afaict. So something is going wrong later....

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Something like this fixes it:

diff --git a/layout/style/ComputedStyle.cpp b/layout/style/ComputedStyle.cpp
index 42403066e02c..b62f1991b3d9 100644
--- a/layout/style/ComputedStyle.cpp
+++ b/layout/style/ComputedStyle.cpp
@@ -390,7 +390,7 @@ ComputedStyle* ComputedStyle::GetCachedLazyPseudoStyle(
     PseudoStyleType aPseudo) const {
   MOZ_ASSERT(PseudoStyle::IsPseudoElement(aPseudo));
 
-  if (nsCSSPseudoElements::PseudoElementSupportsUserActionState(aPseudo)) {
+  if (true || nsCSSPseudoElements::PseudoElementSupportsUserActionState(aPseudo)) {
     return nullptr;
   }

So the issue is that we detect the reframe alright, update the style of the ::marker, but the style of the list-item itself doesn't change. Usually that shouldn't matter since the ::marker got restyled and has its own (updated) style, that's how the pseudo cache is intended to work.

However since the ::marker is NAC, reframing it implies reframing the whole block (the <li> in this case). That goes look into the cache and grabs the old style.

So the ideal fix is "don't reframe the ancestor without their style having changed". That is, if in this test-case we reframe (but don't recreate) the ::marker content, then we get the right style and everything is great.

Tweaking this condition to remove the "has no marker" bit will over-invalidate, but will also fix this test-case.

Ideally that wouldn't be needed, this should be an issue for other pseudo-elements too... We could clear the inheriting style cache when hitting that condition, that'd be the best fix.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1697998 - Invalidate a bit more aggressively when a pseudo-element matches, in order to also invalidate the cached pseudo-styles on the parent. r=mats

This should probably be fine. If it becomes a perf issue somehow we can
implement the RESTYLE_PSEUDOS hint or what not.

Comment 6

[Mats Palmgren (inactive)]

So the ideal fix is "don't reframe the ancestor without their style having changed".

Yeah, I don't see a reason why we need to do that for pseudos in general (other than the reasons we do that for normal children too). We need to teach the frame constructor how to handle that though, but probably worth filing as a follow-up bug. (We should wait for bug 1542807 before attempting to implement that though.)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acd959de0b61
Invalidate a bit more aggressively when a pseudo-element matches, in order to also invalidate the cached pseudo-styles on the parent. r=mats

Comment 8

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

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

Comment 9

[Narcis Beleuzu [:NarcisB]]

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

Comment 10

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

Upstream PR merged by moz-wptsync-bot