Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-highlight-api][css-contain] static ranges and css-contain #4598

Closed
frivoal opened this issue Dec 13, 2019 · 22 comments
Closed

[css-highlight-api][css-contain] static ranges and css-contain #4598

frivoal opened this issue Dec 13, 2019 · 22 comments

Comments

@frivoal
Copy link
Collaborator

frivoal commented Dec 13, 2019

The interaction of StaticRanges in a highlight range group and css-contain seems problematic: on a fully contained element, you should expect that DOM changes to descendants of that element will not cause invalidation and restyling/repainting of elements outside the contained one.

However, if a static range has a boundary point inside the contained subtree and another boundary point outside of it, and the DOM in the contained subtree is changed so that the boundary point inside no longer points to a valid node, the whole range should be ignored, which would affect painting outside the contained subtree. Is this a weakness of style containment, or of the invalidation logic in https://drafts.csswg.org/css-highlight-api-1/#range-invalidation, or something else?

Maybe we need to say that static ranges cannot cross the boundary of a contained element, either making it always invalid, or equivalent to a static range starting at its specified starting point, but truncated at the contained element boundary? or something else?

@dandclark
Copy link
Contributor

Discussed this with some other Edge folks.

It seems like the most straightforward thing is to not paint a range that crosses the boundary of a contained element, basically treating it as invalid. This could just be included as part of the invalidation in Range Updating and Invalidation where it would be another condition for which "the User Agent must ignore that range".

I expect that authors will generally not have a need to create a highlight that crosses a CSS contain boundary, but an author that does need to do this could use a Range instead of a StaticRange.

@sanketj
Copy link
Member

sanketj commented Jan 5, 2021

I think we should disallow highlights that cross contained element boundaries, regardless of whether the highlight was created with a Range or StaticRange. Even though we could technically allow this for Range, I don't think this is an interesting scenario for web authors. It seems simpler, and more consistent, to not paint any such range.

@BoCupp-Microsoft
Copy link
Contributor

Are we resolved on the behavior Dan described above? Just checking what the next steps are for this issue. Open a PR against the editor's draft to update the language on Range Updating and Invalidation?

@dandclark
Copy link
Contributor

I went ahead and wrote a PR for this: #6734

I think I agree with @sanketj that it's simpler to just not allow any ranges (including live ranges) to be painted across contain boundaries, unless there's disagreement about cross-contain highlights being unnecessary. cc @frivoal, what do you think about this?

@astearns astearns moved this from Temp to Cascade/Contain in APAC Nov 3 2021 TPAC meeting Oct 29, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-highlight-api][css-contain] static ranges and css-contain.

The full IRC log of that discussion <dael> Topic: [css-highlight-api][css-contain] static ranges and css-contain
<dael> github: https://github.com//issues/4598
<dael> dandclark: The problem that this issue is trying to addres sis if I have a highlight with a static range that crosses a contain boundry, if I remove a node holding one boundry point it causes entire highlight to not paint b/c it's not valid
<dael> dandclark: Prop solution is not to paint highlights that cross cotnain boundrys. Assumption is it's not a thing authors want
<florian> q+
<dael> dandclark: my understanding is some agreement from editors. Would like to confirm. Also suggestion from sanket this perhaps have same behavior for range to make it consistent to prevent dev confustion
<dael> dandclark: prop would be highlights backed by rangers or static ranges should not be painted if corss a boundry
<Rossen_> ack florian
<dael> florian: Good summary. Initial was jsut static but going for consistency why not. This could be paint containment only, but given no use cases we can make it all for simplicity
<dael> fantasai: Wanted to ask what we'd do about when highlight is similar to selection which frequently crosses the tree
<dael> fantasai: Contain is used for a lot of things and not clear ot me we want to disallow selection type highlights. Something to consider
<dael> florian: Good point. Need to look into in practice where is contain used. It's component-y. Probably not sprinkled through textual elements. But you're right selections cross boundries.
<dael> fantasai: I think you should not restain for simplicity. I think you should if it's necessary for impl reasons to restrict
<fantasai> minimal necessary restriction
<dael> Rossen_: dandclark to confirm, are you saying if I construct the range as you described in which case it will not be painted. From a dev PoV the range is observable; I can inspect it. but visually that is not going to paint based on circumstances range falls into
<dael> Rossen_: Sounds to me pretty unstable approach. to fantasai point I can imagine flickering if I move the ranges. Would be great if there's a CSSOM or other eventing that interacts or rejects ranges. Strong rejection than avoid paint
<dael> Rossen_: That was it could be handled by author. It will be within the hands of developers
<fantasai> +1 to Rossen_'s point, if we are rejecting a range it needs to be made obvious to the author
<fantasai> not just a failure to paint
<dael> Rossen_: From user PoV agree with fantasai that selection today is doing this quite a bit
<dael> florian: Two things we could do. 1: go back to having minimal which is static range only on paint containment only is invalid
<dael> florian: On top of that we might want to add an event or observer that lets you discover something has happened. Hard to design that on the fly
<dael> Rossen_: Shouldn't. I think the feedback here is enough to go back and design stronger.
<dael> florian: I think we could resolve on the type of range and containment, but not the whole story
<dael> dandclark: I think we're coming to resolve on minimal restriction. For highlithgt backed bys tatic ranges that cross a paint containment boundry are not painted
<dael> Rossen_: Opinions?
<dael> florian: Support and then I htink there's more to look into
<dael> Rossen_: I wouldn't object, but unsatisfying b/c opening a problem that makes the discovery of these ranges less obvious to author
<dael> florian: There is an inherit contradition between range and container. Not a problem in both direction, but range that's outside to inside a containment boundry and then you make the range not meaningful you want to stop painting. There is a contradiction somewhere
<dael> Rossen_: Yeah. A little unsatisfying. Consider you're the dev on this editing app with ranges and users are complaining they don't see highlight. How do you chase that? If not observable it will be hard to fix. Even if it's legitamite.
<dael> fantasai: Author will be able to reproduce the error if they select the same range b/c would refuse to paint. But on the fly can't tell via JS
<dael> florian: Another approach, though heavy, is if an author tries to create a range crossing an boundry or restyles to create boundry we forcably split the range to outer and inner
<dael> Rossen_: It's a little better
<dael> Rossen_: But then again not sure a mispelled word is good if you highlight half of it
<dael> florian: You wouldn't highlight half, but do both with separate ranges. Selection use case is more likely for this. Range for outer part of selection and range or inner part. If you change inner part to no longer make sense you would no longer highlight inner
<dael> Rossen_: I'm with you. Feels like we're designing on the fly. Authors will have all kinds of collections and if you create ranges under you need discoverablity. I think there's work to be done here
<dael> Rossen_: Fine with the problem and recongizing we don't want to have these ranges. But if we resolve on something that's strictly worse perhaps we should work more
<dael> florian: I want to preserve containment invarience. But how we do it needs more thinking
<dael> Rossen_: What if we put it back onto GH and when you work it out, sounds like you're starting to come up with solutions, work it out and then come back
<dael> Rossen_: Sound reasonable?
<dael> florian: Think so
<dael> dandclark: Want a gauge on dom obserable vs UA trying to be helpful with console warnings. Are we feeling have to be dom observable?
<dael> florian: I think it's not inheritly wrong to want static ranges. highlight can be anywhere. We should do something that lets author react when it happens
<dael> Rossen_: And being observable through script is key so they can take an action
<dael> Rossen_: Let's end here. I think there's good next steps.

@astearns astearns removed the Agenda+ label Nov 3, 2021
@dandclark
Copy link
Contributor

After spending more time thinking about this I'd like to revisit the assumption that any restrictions on painting across contain boundaries is needed.

As far as I can tell, painting of ranges (both normal Range and StaticRange) happens basically without regard for paint contain boundaries. If I have a live Range inside an element with contain:paint, and I call setStart to move the range's boundary point outside of the element, then I've caused a paint change across a contain boundary. When painting this range, paint containment can't really be taken into account.

The story is similar when painting highlight StaticRanges. In the Chromium implementation we have various conditions that can trigger repaints for them, but these don't take contain into account because ranges don't cleanly fit into the tree structure of the DOM, and their boundary points can be moved independently of DOM tree changes.

So is there anything specifically that would break if we allow StaticRange highlights to be painted across contain:paint boundaries, or any contain-related UA optimizations that would no longer be possible?

I know that we were concerned about cases like this, where changes inside an element with contain would lead to paint changes outside that element due to a range becoming invalid: https://codepen.io/daniec/pen/qBXMEaG. But another way to look at this case is that it's roughly equivalent to calling setStart to move the range's boundary point, which would trigger a contain-boundary-crossing paint update even for a live Range.

So I'm no longer convinced that allowing StaticRanges to paint across contain:paint boundaries is really a problem, if UAs and specs consider the painting of ranges to be something that needs to be computed without considering contain:paint, which I hypothesize is the case.

@ffiori
Copy link
Contributor

ffiori commented Nov 11, 2021

Following up on @dandclark's proposal, it doesn't seem like it would break specifications. I didn't find any direct reference to ranges in css-contain specs or viceversa. But fwiw, the following piece might be relevant (from css-contain specs):

"The contain property allows an author to indicate that an element and its contents are, as much as possible, independent of the rest of the document tree."

So there could be some implication outside of the contained element from changes inside it, like in this case, if I'm not misunderstanding.

@ffiori
Copy link
Contributor

ffiori commented Nov 15, 2021

Friendly ping @frivoal, have you got any opinions on Dan's proposal?

@ffiori ffiori added the Agenda+ label Nov 15, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed statis ranges and highlights.

The full IRC log of that discussion <TabAtkins> Topic: statis ranges and highlights
<TabAtkins> github: https://github.com//issues/4598
<TabAtkins> sanketj: there was discussion in a previous meeting to just add a paint step taht ignores static ranges that cross a containment boundary
<TabAtkins> sanketj: it didn't seem like it was something unique to the highlight api
<TabAtkins> sanketj: selection also adds ranges, and they're allowed to cross containment boundaries
<TabAtkins> sanketj: so if this should be disallowed for highlights, it should be done at a more general level
<TabAtkins> sanketj: so Dan's proposal is that we shouldn't need to handle this case in the Highlight API specifically
<Rossen_> q
<TabAtkins> sanketj: but there's also a question of what forum this q should be taken up in
<florian> q+
<TabAtkins> dandclark: even for live ranges today, if i'm using a live rang eto paint a selection, i can use the range API to make it encompass a paint-contained element and cause paint changes inside that contained element
<TabAtkins> dandclark: so we already don't really pay attention to paint contain, and that makes sense - they're not really bound to the tree hiearchy
<TabAtkins> dandclark: before we were looking at this the wrong way - if i have a range across a paint boundary, and i remove the element that one end is on, we ahve to invalidate it and that requires more invalidation than seemed reasonable
<dandclark> Proposed resolution: Allow static-range-backed custom highlights to be painted across paint-contain boundaries. Changes in static range validity can invalidate an intersected element with paint-contain. This is OK, and we will document this interaction in the highlight spec.
<TabAtkins> dandclark: But really this sort of invalidation is akin to live range invalidation
<TabAtkins> dandclark: So proposed resolution [above]
<Rossen_> ack florian
<TabAtkins> florian: I think i disagree
<TabAtkins> florian: Problem is not whether something crosses a paint boundary. Crossing is fine
<TabAtkins> florian: The initial problem with static range is that if it starts outside the paint contain and goes inside, and the dom changes within the paint-contain area, and the paint-contain area is offscreen, then in that case there is part of the range inside and outside
<TabAtkins> florian: the part that is outside isn't changing, it still goes up to the edge, but the part inside does change, becuas eyou changed the DOM. But that breaks an invariant of paint containment, that you don't need to worrya bout the painting of a contained element that's offscreen
<TabAtkins> florian: I think that's different from your examples
<TabAtkins> florian: If you ahve a live range that you change, so the part that's outside shoudl be different, well you changed it, of course you have to repaint
<TabAtkins> florian: with selection it's the same - if you change the selection, surely you ahve to repaint it
<TabAtkins> florian: also, paint containment allows browsers to skip painting, and paint-based invalidation. it doesn't allow browser to skip dom updates.
<TabAtkins> florian: So if dom updates you do still have to handle that
<Rossen_> q?
<TabAtkins> florian: So that's why the live vs static range is important
<BoCupp> q+ to say in Florian's example state of the range is changed, and if a range intersects a contained element and its state changes its allowed to invalidate its painting
<TabAtkins> florian: live ranges are effectively part of the dom, so modifying the dom modifies the live range
<TabAtkins> florian: but the point of the static range is that it doesn't get updated; the author is meant to take care of that
<TabAtkins> florian: and it might get inconsistent where the entire thing shouldn't be painted
<TabAtkins> florian: So statics can give us an inconsistent state that live ranges can't
<TabAtkins> florian: Just not painting ranges that cross paint boundaries might not be right, we can explore this sapce more
<TabAtkins> BoCupp: In both cases, it's the state of the range that's changing. The range that's intersecting an element with paint containment needs to be able to invalidate inside that boundary
<TabAtkins> BoCupp: Misleading us is that we're using nodes that are inside or outside the containment, to trigger changes in the range.
<TabAtkins> BoCupp: I don't think it matters that the state is computed for static ranges so you can tell they're valid or invalid, or you change the start node so it moves from inside to outside the containment
<TabAtkins> BoCupp: so we don't think there's a new case here. we might need to dig more into what florian is bringing up
<TabAtkins> BoCupp: ultimately this is about optimizations browser can pursue, and rather than put in complicated rules, put the onus on browsers to pay attention to ranges that might intersect containments
<TabAtkins> florian: That invalidates the point of contain, tho. Point is that it removes several things that it no longer needs to pay attention to.
<TabAtkins> BoCupp: It only requires them to pay attention to this one thing, ranges.
<TabAtkins> Rossen_: Sounds like this discussion should continue on GH, there's more detail to be ironed out before we're ready for resolution.

@astearns astearns removed the Agenda+ label Dec 2, 2021
@BoCupp-Microsoft
Copy link
Contributor

Clarifying this one comment above from the scribe:

BoCupp: It only requires them to pay attention to this one thing, ranges.

I was saying that optimizations around painting containment on an element E need to consider ranges that are intersecting E - whether or not they are live or static.

A static range that is invalid (because of disconnected nodes) is no longer intersecting E, just like a live range that has its boundary points updated can be made to no longer intersect E.

I think the reason this issue exists is because of a perception that tracking tree changes under the scope of E can enable browser optimizations, but what we're trying to point out is that ranges and the pseudo-element rules that style them also need special consideration (and likely special code written) to track when to invalidate the painting of elements they intersect.

I'd like to put the onus on browser implementors to write that code, rather than have authors experience a strange behavior when intersecting elements with containment.

@dandclark
Copy link
Contributor

To pick up the discussion where we left off in the meeting, from a practical perspective the only optimizations that would be affected here are those involving a static-range backed custom highlight with a boundary point in an off-screen element with paint-contain.

I think the simplicity and convenience for developers of just painting the range in this scenario would not be outweighed by whatever theoretical optimizations that browsers could achieve for this particular case.

@frivoal
Copy link
Collaborator Author

frivoal commented Dec 8, 2021

turning on contain:paint is supposed to be a sufficient condition to allow the browser to know that no change inside of the contained area can cause a need to repaint something outside of the contained area.

If we continue to say that invalid static ranges aren't painted, and add no other logic, then that is violated: a DOM change inside the contained area can make a static range invalid. If that range extended outside of the contained area, we now have a problem. It's fine that the part of the range inside the contain area is invalid. If the browser has no particular need to repaint the contained area (for example, it is off-screen), then it doesn't, and it doesn't matter than this part of the range was invalid. However, the part of the range outside of the contain area is also invalid. Which means that it needs to be repainted, even if the contain area was offscreen and thus ignorable. Thus, the promise of paint-containment is broken.

The same situation does not cause problems with live ranges. If you change the DOM inside of the contained area, that change causes you to update the boundary of the range (that's what it means for a range to be live). This ensures the range is not invalid. Because the content of the contained area has changed, the part of the range that's inside of it has changed, so it may need repainting if that area itself is onscreen, but the part of the range that is outside the contained area is still valid and unchanged, and thus does not need repainting.

Here's an example: that you have a big div, filled with multiple screen worth of screen. It's text-color is black. At the bottom of that, there's a footer, currently off-screen. To keep the example simple, let's say the footer not only has paint containment, but actually strict containment, so that we don't have to worry about relayouts and whatnot. There's a highlight backed by a static range, which goes from the start of the div to somewhere in the middle of the footer, and makes the highlighted text red. Now some DOM operation deletes the text content of the footer. The end offset of the range is no longer valid, so the range is no longer valid, so it shouldn't be painted. So we need to repaint the whole div to switch the text from red to back, even though the only change we had was in an off-screen strictly contained element. Containment is supposed to allow the UA to not have to worry about that.

Had we used a live range, at the point where the DOM operation deleted the text content of the footer, making it shorter, the end offset of the range would have been adjusted as well (that's what live ranges do), so the range wouldn't be invalid, and the on-screen text of the div would have stayed red, and no repainting would be needed.


So, if we do add that static ranges crossing a containment boundary are never painted, we preserve the promise of paint containment (and we could, but don't need to, add the same thing for live ranges). But it does make for a bad author experience and user experience, because all of a sudden, you've got some ranges that are mysteriously not showing up if they happen to hit a contain boundary.

Can we get out of that problem by (dynamically) doing some automatic adjustement to static ranges when they cross a boundary? Seems to partially defeat the point of static ranges, which aren't supposed to be watched by the browser and updated, and to make life hard for authors when they try to implement they're own logic to update invalidated static range.

Can we get out of that problem by creating some kind of static range invalidation observer, letting authors know one just went bad due to crossing a containment boundary? Doesn't seem to work to me, because:

  • the whole point of static ranges is that the browser doesn't need to do bookkeeping on them, which makes them cheap, and that the author is supposed to know when they do something that invalidates them, and recreate them then. This would defeat that
  • that doesn't help users, when the author has not used this observer, and a range that seems like it ought to work just doesn't show up because it crosses and invisible (to the user) boundary.

So I'm not quite sure what we should do.

@chrishtr
Copy link
Contributor

chrishtr commented Dec 9, 2021

turning on contain:paint is supposed to be a sufficient condition to allow the browser to know that no change inside of the contained area can cause a need to repaint something outside of the contained area.

I think we don't need to be quite this strict. It's a sufficient condition for the browser to know that no "computationally expensive rendering work" of the contained area can cause a need to repaint elsewhere. DOM state affecting such can change, so long as it's not computationally expensive rendering work. The intent is to save the browser the work of doing style, layout, paint and compositing work in that subtree, not work maintaining the DOM tree or necessarily rendering work for other DOM content in general (various kinds of containment like contain:size reduce the likelihood of work elsewhere though).

Therefore, the work to invalidate a static range within a contain:paint subtree, including induced paint changes elsewhere, is only a problem if some computationally expensive rendering work related to paint had to be recomputed in that subtree in order to figure out what to invalidate. It's ultimately a gray area what counts as "computationally expensive rendering work", but I'd say anything that forces style recalc or any of the later rendering phases of a typical browser pipeline are "computationally expensive". DOM state update is generally not.

Reasoning from the definition, it depends only on presence of the start or end Node, its position in the DOM, the number of their children, the length of text content under a Node. Therefore, determining whether a StaticRange has been invalidated seems not especially computationally expensive to me. The worst case seems to be moving it around in the DOM and having to figure out what that means relative to the other endpoint, but that seems acceptable because it's about validating DOM state, optimizing which is out of the scope of contain, a feature that is targeted at rendering work only.

This seems similar to the effect of using regular browser highlighting with a mouse gesture to cover a contain:paint subtree plus other content, then scrolling the subtree offscreen and subsequently performing a mutation to remove the endpoint of the highlight.

@frivoal
Copy link
Collaborator Author

frivoal commented Dec 15, 2021

The intent is to save the browser the work of doing style, layout, paint and compositing work in that subtree

Actually, the intent is pretty much the exact opposite: the intent is to make sure that any work the Browser has to do is limited to that subtree, and saving from having to do work elsewhere.

Here's another example, more realistic since unlike the previous one, it could have performance implications.

Say you have a website tracking stock tickers, each represented by some card. You're tracking the whole stock market, so you have lots of cards (a few thousands), and since they're so many, most of them are offscren. Also, they frequently update (possibly several times per second for highly traded stock), because you're following the market in real time.

You set contain:strict on each card, to make sure that updating one card doesn't cause the browser to need to reevaluate anything else (and so that if that particular card is off-screen, it doesn't need to relayout/repaint anything at all).

On top of that, let's say you implement some kind of custom find-in-page (maybe with regexp support?) using custom highlights. If the user searches for something common (e.g. \s+\d+\s+), you might have a lot of ranges on your page. Live-ranges are not appropriate: each time the DOM updates and changes the text in some card, you don't want the UA to update the start/end points of the range to the nearest valid offset, since that may not match the searched pattern. When the DOM changes, your script handles trashing and/creating ranges as appropriate. Maybe it throttles these updates and only does them every 100ms rather than on every change, to avoid busy work. Letting the browser adjust the ranges before deleting and recreating them would just be a waste of time.

Now, if the UA simply allows highlights backed by static ranges to cross containment boundaries and be painted as long as they're valid, then, every time the DOM in a card is updated, you need to:

  • find all the ranges that previously has at least one end in the updated card and check if it's still valid, even if the card is offscreen (because part of the range might be on-screen)
  • For any that did become invalid, you need to repaint parts of the document that were outside of the contained area if they were covered and affected by that no-longer-valid range, even if the updated card is offscreen
  • Right after you do that, the author's script will likely trash that range that you just did some work for, and possibly recreate a new one (if it found a new match for the user's search), which possibly will cause you to need to repaint again, and as that too could be over the whole document.

So, potentially, you're calling for evaluation of range validity, and potential repaints of the whole document many thousands of time per second. :(

Instead, if highlights backed by static ranges never paint when a range crosses a containment boundary, this whole thing gets short-circuited most of the time. The first step is to check whether the updated card is off-screen, and if so, you can just stop there. No need to check for any range's validity. No need to repaint anything at all. If it is on screen, you just need to repaint that particular card (and skip now-invalid ranges while doing that) and nothing else.

Now, admittedly, if you did intent to let the user find things that span across containment boundaries, then that's unfortunate, because that speed-up is achieved by making that impossible. Maybe authors just need to be aware of that, and only use static ranges either when they're not using containment know that they don't need to cross containment boundaries, and use live ranges when that's not an option. That doesn't sound author friendly though.

This seems similar to the effect of using regular browser highlighting with a mouse gesture to cover a contain:paint subtree plus other content, then scrolling the subtree off-screen and subsequently performing a mutation to remove the endpoint of the highlight.

This is not similar, because that range is a live one.

  • You need to update its end point anyway when you mutate the DOM. Using static ranges is exactly intended to let you opt out of that, so if static ranges need re-validation when the DOM changes, the benefit of using static ranges goes away
  • A mutation within a contained subtree may move the end-point of a live-range that was within that subtree. If that subtree is on screen, it will need to be repainted (it had to anyway, since the DOM has changed), but as that moving end-point cannot be moved to outside of the contained subtree by a DOM change within the subtree, the part of the range that is outside of the contained area cannot be changed by this operation, and doesn't thus cannot trigger a repaint of a broader part of the document.

@chrishtr
Copy link
Contributor

The intent is to save the browser the work of doing style, layout, paint and compositing work in that subtree

Actually, the intent is pretty much the exact opposite: the intent is to make sure that any work the Browser has to do is limited to that subtree, and saving from having to do work elsewhere.

I'd say it's most correct to say both. Yes, containment prevents various kinds of rendering invalidation from causing work outside of the subtree. But that's exactly what allows potentially doing no work when the content is offscreen, or script is calling a DOM API on an element outside of the subtree. To summarize in a different way, there there are two optimization opportunities:

  1. Faster rendering updates when mutations only occur in the subtree (because the rendering update is limited to that subtree, depending on the contain value.
  2. Avoiding rendering work in that subtree when the API or screen doesn't need it (offscreen or DOM API call).
  • find all the ranges that previously has at least one end in the updated card and check if it's still valid, even if the card is offscreen (because part of the range might be on-screen)
  • For any that did become invalid, you need to repaint parts of the document that were outside of the contained area if they were covered and affected by that no-longer-valid range, even if the updated card is offscreen

Yes, but I think this is fine, because the static range went outside of the contained subtree, and the computation doesn't require doing any rendering work for the contained subtree. The work required is some DOM traversal/validation, and paint of the content outside the subtree. It also has better UX, because the feature of highlighting will work in cases where, as far as the user can see, it should.

Further, if the developer is truly concerned about optimizing this then they can avoid creating static ranges that cross their own containment boundaries and/or avoid invalidating the static ranges with DOM mutations. In the case of a stock ticker and them implementing their own find-in-page, I don't think that is all that hard.

  • Right after you do that, the author's script will likely trash that range that you just did some work for, and possibly recreate a new one (if it found a new match for the user's search), which possibly will cause you to need to repaint again, and as that too could be over the whole document.

The whole document except the subtree.

Now, admittedly, if you did intent to let the user find things that span across containment boundaries, then that's unfortunate, because that speed-up is achieved by making that impossible. Maybe authors just need to be aware of that, and only use static ranges either when they're not using containment know that they don't need to cross containment boundaries, and use live ranges when that's not an option. That doesn't sound author friendly though.

The user is more important than the developer. Further, I don't think the performance is really a big problem in terms of the performance browsers should provide, since as I said the repaint is limited to the parts of the screen the user cares about, and not the subtree that was contained and offscreen.

This seems similar to the effect of using regular browser highlighting with a mouse gesture to cover a contain:paint subtree plus other content, then scrolling the subtree off-screen and subsequently performing a mutation to remove the endpoint of the highlight.

This is not similar, because that range is a live one.

Yes, but live in this example just means the UA is doing the heavy lifting rather than script. Isn't it the same otherwise?

  • A mutation within a contained subtree may move the end-point of a live-range that was within that subtree. If that subtree is on screen, it will need to be repainted (it had to anyway, since the DOM has changed), but as that moving end-point cannot be moved to outside of the contained subtree by a DOM change within the subtree, the part of the range that is outside of the contained area cannot be changed by this operation, and doesn't thus cannot trigger a repaint of a broader part of the document.

I think a static or live range that spans containment is not contained within the subtree, and so containment can't really help it unless we disallow the feature for this situation. I think we agree on this point though. My view is that (a) the rendering cost is acceptable given the benefits to the user, and (b) the rendering doesn't need to do anything in the offscreen subtree, so the goal of use case (2) I mentioned above of CSS contain is obtained.

@frivoal
Copy link
Collaborator Author

frivoal commented Dec 22, 2021

@tabatkins, could you chime in? I suspect you are in a good place to both grok what @chrishtr and I are trying to get at, and help either or both of us see what we seem to be missing in each-other's points.

I could try to respond to @chrishtr 's points above, but I feel like we're going in circles, somehow understanding each other at the surface level yet missing eachother's deeper point, and I hope a fresh perspective could get us of that.

@dandclark
Copy link
Contributor

Ping @tabatkins, it would be great to have your thoughts on this issue.

I think there's general agreement that not painting highlights across contain boundaries would be a suboptimal experience for developers and users. I'm in agreement with the points that @chrishtr has been making above, concluding that it would be reasonable to just go ahead and paint the highlights, but short of trying to rephrase those I'm not sure what else I can add.

This is one of the last things preventing us from shipping Highlight API in Blink, so I'd really like to find a solution. @frivoal do you have any ideas on how we can move forward?

@dandclark
Copy link
Contributor

dandclark commented Jan 28, 2022

I’ve been looking through Chomium’s range-related paint invalidation code to see if would help articulate some of the points being made here more precisely.

We’ve argued that conceptually, painting for both live ranges and static ranges should be considered as happening independently of CSS contain, because ranges can span arbitrary sections of the tree and are therefore unsuited for optimizations like CSS contain that are applied to specific subtrees.

This turns out to be true in the Chromium range invalidation code, which happens at a document-wide level without regard for scoping into individual subtrees.

For Selection, backed by a live range, LayoutSelection::Commit is called prior to painting. LayoutSelection remembers the set of nodes that had selection styles last time a paint was performed. It calculates the set of nodes that should have selection styles given the current position of the selection range. Then, paint is invalidated for the nodes whose selection state has changed.

This operation is triggered independently of CSS containment, and it’s difficult to see how taking paint containment into account could make it significantly faster. The operation just isn’t tree-based in a way that could take advantage of knowledge of contained subtrees.

Highlight invalidation for StaticRanges is performed in a different part of the codebase but uses a similar technique. The DocumentMarkerController has a map containing all Text nodes currently being painted with any highlight style. During pre-paint, a StaticRange validation step walks registered highlights and checks the validity of each StaticRange backing these Highlights. The StaticRanges that are no longer valid will be removed from the DocumentMarkerController. Removing a StaticRange in this way causes the DocumentMarkerController to invalidate paint for each Text node spanned by that range.

This whole operation is performed without regard for CSS contain boundaries. It’s difficult to see what optimization could be added to this StaticRange painting invalidation that would take advantage of paint-contain information. In @frivoal’s stock ticker example, when a stock price is updated (offscreen or otherwise), we'll detect if a static range is no longer valid and invalidate all affected nodes by consulting the DocumentMarkerController’s record of which nodes were previously covered by highlights. The invalidation can be done precisely, only invalidating the nodes that need repainting, which could then be limited to on-screen nodes. CSS contain doesn't influence this type of range based invalidation.

Thus, we’re not giving up any important performance opportunities by painting StaticRange highlights across contain boundaries.

@chrishtr
Copy link
Contributor

Thanks for this analysis @dandclark . I've put agenda+ for next week, which is an APAC call that will I hope be convenient for your timezone @frivoal.

@dandclark
Copy link
Contributor

Since this is potentially on the agenda for tomorrow, pinging @frivoal one more time -- did you still have concerns here that we could potentially work out in this thread or in a smaller call before the CSSWG meeting? We might have a small timeslot for this topic given the packed agenda so if there's anything we can work through beforehand that would be helpful.

@tabatkins
Copy link
Member

Sorry for being late to this convo.

Unfortunately, @frivoal, I don't think you're right here. Paint containment does not ensure that arbitrary changes in the subtree don't result in paint changes outside the subtree, it ensures that painting originating inside of the subtree doesn't escape the bounds and require invalidation of a larger area (and lets us avoid having to defensively optimize against such an event with overly-large textures, etc). Various details not related to "doing painting" can absolutely affect what's outside of the subtree and cause it to need to update in various ways, up to and including painting.

So in this particular situation, the presence of a range (static or live) intersecting with a paint-contained subtree is definitely outside the set of things covered by that containment. Similarly, none of the other containments would restrict this; it's outside the remit of even style containment.

So, I think containment has nothing to say about ranges.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-highlight-api][css-contain] static ranges and css-contain, and agreed to the following:

  • RESOLVED: Allow static range backed custom highlights to be painted across paint contain boundaries
The full IRC log of that discussion <dael> Topic: [css-highlight-api][css-contain] static ranges and css-contain
<dael> github: https://github.com//issues/4598
<dandclark> Proposed resolution: Allow static-range-backed custom highlights to be painted across paint-contain boundaries.
<dael> dandclark: This is a issue regarding if custom highlights backed by static range should paint of cross bounderies
<dael> dandclark: Would like to allow static range backed to be painted across bounderies. Think it's better user and dev experience. For user won't have issues when things disappear when crossing arbitrary bounderies
<dael> dandclark: TabAtkins had point that htis is outside of what paint contain requires. If element is offscreen you can skip painting. This doens't follow the definition. Skpping offscreen we would not break
<dael> dandclark: Did due diligince in chromium. We believe optimizations lost would be minor since we would jsut need to do validity checks. When painting we invalidate all nodes and can still skip when doing painting step if offscreen
<dael> dandclark: Overall the pros outweight the cons
<Rossen_> q
<sanketj> SGTM
<dael> florian: I've been convinced I'm wrong. Thank you for doing the diligence to give me confidence in the solution
<dael> Rossen_: Any others who were in florian 's camp?
<dael> Rossen_: Prop?
<dael> dandclark: Allow static range backed custom highlights to be painted across paint contain bounderies
<dael> Rossen_: Objections?
<dael> RESOLVED: Allow static range backed custom highlights to be painted across paint contain boundaries

@frivoal frivoal added Closed Accepted by CSSWG Resolution Testing Unnecessary Memory aid - issue doesn't require tests labels Feb 3, 2022
@frivoal frivoal closed this as completed Feb 3, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 4, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 4, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 4, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}
aarongable pushed a commit to chromium/chromium that referenced this issue Feb 4, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 4, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}
mattwoodrow pushed a commit to mattwoodrow/wpt that referenced this issue Feb 15, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 26, 2022
…ainting, a=testonly

Automatic update from web-platform-tests
Highlight API: Ignore CSS contain when painting

Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}

--

wpt-commits: d6608a62788dd7c1595b73135d12fc6fb15f2b13
wpt-pr: 32688
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this issue Feb 28, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 1, 2022
…ainting, a=testonly

Automatic update from web-platform-tests
Highlight API: Ignore CSS contain when painting

Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}

--

wpt-commits: d6608a62788dd7c1595b73135d12fc6fb15f2b13
wpt-pr: 32688
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 8, 2022
…ainting, a=testonly

Automatic update from web-platform-tests
Highlight API: Ignore CSS contain when painting

Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}

--

wpt-commits: d6608a62788dd7c1595b73135d12fc6fb15f2b13
wpt-pr: 32688
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 8, 2022
…ainting, a=testonly

Automatic update from web-platform-tests
Highlight API: Ignore CSS contain when painting

Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}

--

wpt-commits: d6608a62788dd7c1595b73135d12fc6fb15f2b13
wpt-pr: 32688
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Per the CSSWG resolution [1], ignore CSS contain boundaries when
painting.

Update tests, and remove some tests that are no longer interesting
now that StaticRange painting no longer needs to take contain
boundaries into account.

[1] w3c/csswg-drafts#4598

Bug: 1164461
Change-Id: Iad9249867ea704f924827db810816935069363c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3437210
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Fernando Fiori <ffiori@microsoft.com>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#967368}
NOKEYCHECK=True
GitOrigin-RevId: 5217f39d5be790d9bd340a54d4d87c71e566d250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

9 participants