Nested activation problem.
Closed this issue · 8 comments
I think we might have a problem with nested locks since we've switched to CSS version. The problem is that we don't know if we're locked or not until style is done. Why is this a problem? Well it is a problem in a nested case.
Consider:
<style>
#a { render-subtree: invisible skip-viewport-activation; }
#b { render-subtree: invisible skip-viewport-activation; }
</style>
<div id=a>
a
<div id=b>
b
<div id=c>
c
</div>
</div>
</div>
<script>
onload = () => c.scrollIntoView();
</script>What do we expect to happen here? Well, we have 2 nested locks (a and b) and a target c that we scroll into view which is nested under them. We expect to see the following since all of the locks are activated by the scrollIntoView() call.
a
b
c
What actually happens is that we prevent style updates for a's subtree. This means that b is not locked while a is locked. In turn, it means that c's scrollIntoView does two things: activates a and causes style to process b and lock it. So what we see is
a
That's by design of skipping work... Oops.
I think a similar issue happens in something like the following (no nesting though):
<script>
const el = document.createElement("div");
el.innerHTML = "...";
el.style = "render-subtree: inivisible skip-activation";
document.body.appendChild(el);
el.firstElementChild.offsetTop; // ???
</script>The problem is that we don't lock the element until style runs. offsetTop call here forces all existing locks, but then after forcing, we run UpdateStyleAndLayout and create a new lock.
scrollIntoView() doesn't activate...
For the second example you gave, isn't it just a (smallish?) implementation problem? I don't see set where semantics are messed up.
Yeah in the previous/attribute implementation we force the style run to go inside a locked subtree if there is a new locking/unlocking activity, but there is no way to do this with the CSS version.
So when we want to force layout for all ancestors of X, we can't just collect all locked ancestors of X and force layout, because we might encounter more newly-locked ancestors during the force layout run.
I think though, since the unlocking actually happens during the forced layout, we can just do this:
whenever we encounter a newly-locked element, check if it's the ancestor of X (so we need to keep X). This might be a problem when X is actually a set of nodes, so we need to be efficient here. One way to do this is to actually keep a bool "is ancestor of X" during style and it's true at the root, and pass it down when traversing down to children (we will only pass it down to one child, and we should know which child already?). It can be a count if X is a set of nodes (and we're passing it down to multiple children probably). Hopefully it's not too bad in terms of performance?
Don't know if any spec parts need to change because of this? Probably not.
Forced layouts we can work around easily by doing what you suggest, but activation from inner elements is harder, since we activate an element and then let the lifecycle do its thing. Now, we have to somehow figure out which new locks are as a result of an element that was activated. Just checking ancestors isn't enough, since (1) it's completely feasible for an element to be activated and then script changes the style to put in a new lock in its subtree and (2) sibling locks of the activated element ancestor path should not be activated. From the lifecycle perspective all those situations would look exactly the same. However, in one we should immediately activate the lock, in the other ones we shouldn't.
I can't really think of a way to do this without keeping track of what caused the activation until we reach that element in style (assuming it needs style to begin with). If we reached it, then we're done. If a lock prevented reaching it, then we need to activate it. We could also activate and run style in a loop until we can't activate anything else.
This highlights another problem that a nested lock can be non-activatable. This poses two problems in the implementation: first we need to be able to abort things since if we are trying to reach an element and one of the locks along the way happens to be activation-blocking, then we're done. Second, we have some optimizations in the code built in around the fact that we keep counts of number of locks and number of activation blocking locks. We should at least check that all those calls are OK if one of the activation blocking locks is 'hidden' behind an activatable one.
I see, so this is complicated because beforeactivation is sent before the activation and can possibly add new locks under the element to be activated, and also between the activated element and the topmost locked ancestor. I think for the former we can make sure that we only activate elements that are ancestors of the element to be activated, for the latter.. I don't know. Is this still a problem with the new not-related-to-rendersubtree activation event? And what's the timing for that event? Is there a way to time them to make sure they won't be able to create these cases.
Not sure I understand case (2) in your comment (sibling locks?). Also, a locked (but not marked as locked yet) element in a locked subtree is always style-dirty, right? (Otherwise its style hasn't changed at all since before its ancestor gets locked)
For the non-activatable case (skip-viewport-activation or skip-activation), I thought we're not actually going to activate anything? We're just going to send events and let the author unlock them?
To elaborate on my cases, if you have:
A
|------|
B C
|
D
(root A with 2 children B and C, and B has one child D), and all things have locks, then normally only A is "actually" locked in the implementation. If D triggers an activation, then A is activated. Whether or not to activate B is case (1) (ie we don't know if this was there before or did the script just put this lock in after activation), and whether or not to activate C is case (2) (ie it's a sibling of one of the ancestors, we shouldn't activate it, but without knowing that D caused this lifecycle we can't figure this out).
The problem with non-activatable case of skip-activation, is consider that A has "invisible" and B has "invisible skip-activation". We want to make sure that find-in-page or some other thing does not trigger this event (whether or not it is a part of the render-subtree spec).
However, again, B in the implementation isn't actually locked, so all we see is that all ancestors are activatable.
But as you say, all of those conceptually locked, but not implementaiton locked elements would have to be style dirty. And if they are style dirty, then we just have to make sure to force clean style whenever we need these decisions. So I think this is an implementation concern not a spec one.
Yeah I think collecting all currently-known locked ancestors of the element of interest and forcing styles in the subtrees of these ancestors to get the rest of the ancestors (without activating/sending events) should be enough.
This has been more or less resolved by ensuring that a forced update remembers the element which caused the forced scope to be created and forcing any new locks that appear on the path between the node and the existing forced locks (all of this in Chromium)