Implement Element.matchContainer [chromium/src : main]
Oriol Brufau (Gerrit)
Oriol Brufau added 2 comments![]( "Open in Gerrit")
This is increasing the compile size too much: https://ci.chromium.org/ui/p/chromium/builders/try/compile-size/561145/infra
interface ContainerQueryListEvent : EventI don't think this should be exposed to stable before shipping
Related details
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 2 comments![]( "Open in Gerrit")
This is increasing the compile size too much: https://ci.chromium.org/ui/p/chromium/builders/try/compile-size/561145/infra
Ah yes. Fixed via removing Forward declaration and remove unnecessary includes
I don't think this should be exposed to stable before shipping
- Oriol Brufau
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
Hi folks,
I'd like to request an early review of the implementation approach. Since the spec is still [a work in progress](https://github.com/w3c/csswg-drafts/pull/13551#issuecomment-4235816324), the naming and API surface are subject to change. However, I'd appreciate your feedback on the overall logic and architecture at this stage. We can keep this CL as a draft until the spec is finalized, or I can split it into smaller CLs if preferred.
Please take a look when you have a moment. Thanks!
Related details
- Anders Hartvoll Ruud
- Oriol Brufau
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
Hi folks,
I'd like to request an early review of the implementation approach. Since the spec is still [a work in progress](https://github.com/w3c/csswg-drafts/pull/13551#issuecomment-4235816324), the naming and API surface are subject to change. However, I'd appreciate your feedback on the overall logic and architecture at this stage. We can keep this CL as a draft until the spec is finalized, or I can split it into smaller CLs if preferred.
Please take a look when you have a moment. Thanks!
+A CSSWG expert will be reviewing the spec side of this soon
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 2 comments![]( "Open in Gerrit")
Seokho SongHi folks,
I'd like to request an early review of the implementation approach. Since the spec is still [a work in progress](https://github.com/w3c/csswg-drafts/pull/13551#issuecomment-4235816324), the naming and API surface are subject to change. However, I'd appreciate your feedback on the overall logic and architecture at this stage. We can keep this CL as a draft until the spec is finalized, or I can split it into smaller CLs if preferred.
Please take a look when you have a moment. Thanks!
+A CSSWG expert will be reviewing the spec side of this soon
I indeed think you should split this up. For example, defer everything related to events to the next CL. (But you know better how to split this up.)
ContainerFeaturesChanged();I don't think we can queue events in the middle of style recalc like this. This can happen any number of times due to speculative styling, before we finally change our mind about the result.
The timing of these events need to be specified.
Related details
- Oriol Brufau
- Rune Lillesveen
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
Seokho SongHi folks,
I'd like to request an early review of the implementation approach. Since the spec is still [a work in progress](https://github.com/w3c/csswg-drafts/pull/13551#issuecomment-4235816324), the naming and API surface are subject to change. However, I'd appreciate your feedback on the overall logic and architecture at this stage. We can keep this CL as a draft until the spec is finalized, or I can split it into smaller CLs if preferred.
Please take a look when you have a moment. Thanks!
Anders Hartvoll Ruud+A CSSWG expert will be reviewing the spec side of this soon
I indeed think you should split this up. For example, defer everything related to events to the next CL. (But you know better how to split this up.)
Yeah, that's a good idea. I'll remove the WPT case and the EnqueueUniqueAnimationFrameEvent() call in this CL to defer the implementation mentioned in the other comment.
Related details
- Anders Hartvoll Ruud
- Oriol Brufau
- Rune Lillesveen
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
ContainerFeaturesChanged();I don't think we can queue events in the middle of style recalc like this. This can happen any number of times due to speculative styling, before we finally change our mind about the result.
The timing of these events need to be specified.
I have draft...comment here but just vanished.., anyway..
Hmm, probably the right place to fire the event should be the same as matchMedia's? `Document::UpdateStyleAndLayoutTreeForThisDocument()` -> `Document::EvaluateMediaQueryListIfNeeded()`
The idea is to mark them as changed in this place and fire the event after recalc same with matchMedia. Do you think it is feasible?
Related details
- Anders Hartvoll Ruud
- Oriol Brufau
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
ContainerFeaturesChanged();Seokho SongI don't think we can queue events in the middle of style recalc like this. This can happen any number of times due to speculative styling, before we finally change our mind about the result.
The timing of these events need to be specified.
I have draft...comment here but just vanished.., anyway..
Hmm, probably the right place to fire the event should be the same as matchMedia's? `Document::UpdateStyleAndLayoutTreeForThisDocument()` -> `Document::EvaluateMediaQueryListIfNeeded()`
The idea is to mark them as changed in this place and fire the event after recalc same with matchMedia. Do you think it is feasible?
Hmm, probably the right place to fire the event should be the same as matchMedia
No, that happens _before_ style and layout. That's fine for media, since it doesn't depend on style, but container queries _do_ depend on style, so it's not that simple. It probably needs to work more like ResizeObserver.
Related details
- Oriol Brufau
- Rune Lillesveen
- Seokho Song
Seokho Song (Gerrit)
Seokho Song added 2 comments![]( "Open in Gerrit")
Seokho SongHi folks,
I'd like to request an early review of the implementation approach. Since the spec is still [a work in progress](https://github.com/w3c/csswg-drafts/pull/13551#issuecomment-4235816324), the naming and API surface are subject to change. However, I'd appreciate your feedback on the overall logic and architecture at this stage. We can keep this CL as a draft until the spec is finalized, or I can split it into smaller CLs if preferred.
Please take a look when you have a moment. Thanks!
Anders Hartvoll Ruud+A CSSWG expert will be reviewing the spec side of this soon
Seokho SongI indeed think you should split this up. For example, defer everything related to events to the next CL. (But you know better how to split this up.)
Yeah, that's a good idea. I'll remove the WPT case and the EnqueueUniqueAnimationFrameEvent() call in this CL to defer the implementation mentioned in the other comment.
Back WPTs and some impls. I will revert it with PoC after PoC done.
ContainerFeaturesChanged();Seokho SongI don't think we can queue events in the middle of style recalc like this. This can happen any number of times due to speculative styling, before we finally change our mind about the result.
The timing of these events need to be specified.
Anders Hartvoll RuudI have draft...comment here but just vanished.., anyway..
Hmm, probably the right place to fire the event should be the same as matchMedia's? `Document::UpdateStyleAndLayoutTreeForThisDocument()` -> `Document::EvaluateMediaQueryListIfNeeded()`
The idea is to mark them as changed in this place and fire the event after recalc same with matchMedia. Do you think it is feasible?
Hmm, probably the right place to fire the event should be the same as matchMedia
No, that happens _before_ style and layout. That's fine for media, since it doesn't depend on style, but container queries _do_ depend on style, so it's not that simple. It probably needs to work more like ResizeObserver.
I've uploaded a new PatchSet as a PoC. The core idea is:
Deferring the evaluation and firing of the event until after the style and layout phases are fully resolved. Since `ContainerQueryEvaluator` evaluates and stores the final computed values (e.g., size), we simply collect the "dirty" `ContainerQueryList` targets in a Set during style recalc. Then, at the end of the lifecycle loop, we sweep this Set, evaluate the final state, and fire any necessary events, likely the timing `ResizeObserver` uses. (I assume this is a spec-defined behavior? Probably this one [1]? If so, should we define the exact timing for our case too? Is this what you meant by "The timing of these events needs to be specified"?)
For the PoC, I placed the tracking Set and logic directly into `Document`. It would probably be better to place this into `-Controller` (similar to `ResizeObserverController`).
WDYT about this approach?
[1] https://drafts.csswg.org/resize-observer/#broadcast-resize-notifications-h
Related details
- Anders Hartvoll Ruud
- Oriol Brufau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
ContainerFeaturesChanged();Seokho SongI don't think we can queue events in the middle of style recalc like this. This can happen any number of times due to speculative styling, before we finally change our mind about the result.
The timing of these events need to be specified.
Anders Hartvoll RuudI have draft...comment here but just vanished.., anyway..
Hmm, probably the right place to fire the event should be the same as matchMedia's? `Document::UpdateStyleAndLayoutTreeForThisDocument()` -> `Document::EvaluateMediaQueryListIfNeeded()`
The idea is to mark them as changed in this place and fire the event after recalc same with matchMedia. Do you think it is feasible?
Seokho SongHmm, probably the right place to fire the event should be the same as matchMedia
No, that happens _before_ style and layout. That's fine for media, since it doesn't depend on style, but container queries _do_ depend on style, so it's not that simple. It probably needs to work more like ResizeObserver.
I've uploaded a new PatchSet as a PoC. The core idea is:
Deferring the evaluation and firing of the event until after the style and layout phases are fully resolved. Since `ContainerQueryEvaluator` evaluates and stores the final computed values (e.g., size), we simply collect the "dirty" `ContainerQueryList` targets in a Set during style recalc. Then, at the end of the lifecycle loop, we sweep this Set, evaluate the final state, and fire any necessary events, likely the timing `ResizeObserver` uses. (I assume this is a spec-defined behavior? Probably this one [1]? If so, should we define the exact timing for our case too? Is this what you meant by "The timing of these events needs to be specified"?)
For the PoC, I placed the tracking Set and logic directly into `Document`. It would probably be better to place this into `-Controller` (similar to `ResizeObserverController`).
WDYT about this approach?
[1] https://drafts.csswg.org/resize-observer/#broadcast-resize-notifications-h
Yes, something along those lines, although timing issues are a CSSWG/standards decision, not a Blink decision. I will have to look more closely at the details later. For now (for the first CL), I still think we should remove anything that involves events/dirtying, etc.
If so, should we define the exact timing for our case too? Is this what you meant by "The timing of these events needs to be specified"?
Yes, exactly. We also need to know if the event can be fired multiple times in the same frame (like RO), and if so, what mechanism guarantees that the process actually finishes.
The necessity of being technically similar to RO is the reason why I'm skeptical that this API should exist in the first place ...
Related details
- Oriol Brufau
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
ContainerFeaturesChanged();Seokho SongI don't think we can queue events in the middle of style recalc like this. This can happen any number of times due to speculative styling, before we finally change our mind about the result.
The timing of these events need to be specified.
Anders Hartvoll RuudI have draft...comment here but just vanished.., anyway..
Hmm, probably the right place to fire the event should be the same as matchMedia's? `Document::UpdateStyleAndLayoutTreeForThisDocument()` -> `Document::EvaluateMediaQueryListIfNeeded()`
The idea is to mark them as changed in this place and fire the event after recalc same with matchMedia. Do you think it is feasible?
Seokho SongHmm, probably the right place to fire the event should be the same as matchMedia
No, that happens _before_ style and layout. That's fine for media, since it doesn't depend on style, but container queries _do_ depend on style, so it's not that simple. It probably needs to work more like ResizeObserver.
Anders Hartvoll RuudI've uploaded a new PatchSet as a PoC. The core idea is:
Deferring the evaluation and firing of the event until after the style and layout phases are fully resolved. Since `ContainerQueryEvaluator` evaluates and stores the final computed values (e.g., size), we simply collect the "dirty" `ContainerQueryList` targets in a Set during style recalc. Then, at the end of the lifecycle loop, we sweep this Set, evaluate the final state, and fire any necessary events, likely the timing `ResizeObserver` uses. (I assume this is a spec-defined behavior? Probably this one [1]? If so, should we define the exact timing for our case too? Is this what you meant by "The timing of these events needs to be specified"?)
For the PoC, I placed the tracking Set and logic directly into `Document`. It would probably be better to place this into `-Controller` (similar to `ResizeObserverController`).
WDYT about this approach?
[1] https://drafts.csswg.org/resize-observer/#broadcast-resize-notifications-h
Yes, something along those lines, although timing issues are a CSSWG/standards decision, not a Blink decision. I will have to look more closely at the details later. For now (for the first CL), I still think we should remove anything that involves events/dirtying, etc.
If so, should we define the exact timing for our case too? Is this what you meant by "The timing of these events needs to be specified"?Yes, exactly. We also need to know if the event can be fired multiple times in the same frame (like RO), and if so, what mechanism guarantees that the process actually finishes.
The necessity of being technically similar to RO is the reason why I'm skeptical that this API should exist in the first place ...
Thanks! I added a comment regarding the event timing to the [PR comment](https://github.com/w3c/csswg-drafts/pull/13551#issuecomment-4280237523) :)
> For now (for the first CL), I still think we should remove anything that involves events/dirtying, etc.
- Anders Hartvoll Ruud
- Oriol Brufau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 2 comments![]( "Open in Gerrit")
Seokho SongHi folks,
I'd like to request an early review of the implementation approach. Since the spec is still [a work in progress](https://github.com/w3c/csswg-drafts/pull/13551#issuecomment-4235816324), the naming and API surface are subject to change. However, I'd appreciate your feedback on the overall logic and architecture at this stage. We can keep this CL as a draft until the spec is finalized, or I can split it into smaller CLs if preferred.
Please take a look when you have a moment. Thanks!
Anders Hartvoll Ruud+A CSSWG expert will be reviewing the spec side of this soon
Seokho SongI indeed think you should split this up. For example, defer everything related to events to the next CL. (But you know better how to split this up.)
Seokho SongYeah, that's a good idea. I'll remove the WPT case and the EnqueueUniqueAnimationFrameEvent() call in this CL to defer the implementation mentioned in the other comment.
Back WPTs and some impls. I will revert it with PoC after PoC done.
Reverted.
Reverted event-related changes, and left TODOs and NOTIMPLEMENTED(). The PoC is in ~PatchSet 42.
Related details
- Anders Hartvoll Ruud
- Oriol Brufau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
OK, but this is still a lot. What is the _smallest_ first step we can make here?
Related details
- Oriol Brufau
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
I suppose the smallest first step is:
1. Adding a feature flag.
2. Adding `matchContainer()` to Element with a implementation that returns null with `NOTIMPLEMENTED()`.
(I think we may also need to define `ContainerQueryList` without details. Marking every details as `NOTIMPLEMENTED` ?).
WDYT?
Related details
- Anders Hartvoll Ruud
- Oriol Brufau
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
Right, adding a feature flag alone may be the smallest step, but ideally each step gives us _something_ new to write tests against.
So I think the smallest _meaningful_ step is to add just this:
```
[
ActiveScriptWrappable,
RuntimeEnabled=ElementMatchContainer,
Exposed=Window
] interface ContainerQueryList : EventTarget {
readonly attribute boolean matches;
};
```
Reading `matching` should just evaluate the query to begin with (I assume the answer should be "live"?). We avoid anything related to events, raredata, caching, dirtiness. Those interactions need to be carefully thought about, tested, (specified), etc, so we have to ease into that.
Marking every details as NOTIMPLEMENTED.
Instead of placeholders with `NOTIMPLEMENTED()`, just omit the entire placeholder. (Except of course that which is strictly required to compile due to e.g. pure virtual functions, if any.)
Related details
- Oriol Brufau
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |