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

[@container] Make UpdateStyleAndLayoutTreeForNode understand CQ #32904

Merged
merged 1 commit into from Mar 14, 2022

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Feb 18, 2022

We currently have pretty serious bugs in getComputedStyle (and similar
APIs that force style), since NeedsLayoutTreeUpdateForNodeIncluding-
DisplayLocked is completely unaware of container queries: if nothing
is style dirty in the ancestor chain, we will early-out from
UpdateStyleAndLayoutTreeForNode, and never actually update anything.
However, for container queries, we have to also check for layout-
dirtiness, since doing that layout can affect container-query-
dependent elements.

This CL tries to formalize the concept of a "layout upgrade", which
is a name for when UpdateStyleAndLayoutTree needs to also call
UpdateStyleAndLayout due layout dependencies.

  • NeedsLayoutTreeUpdateForNodeIncludingDisplayLocked needs to return
    true whenever we may need to upgrade. Otherwise we'll early-out,
    and never even get the chance to upgrade. Hence, we look at the
    target node and the ancestor chain, and try to figure out if a
    layout upgrade will be needed. We can not know this for sure
    at this time, because the subsequent call to update style may
    (for example) remove all container-query containers, i.e.
    remove layout dependencies.
  • UpdateStyleAndLayoutTree now accepts a LayoutUpgrade object, which
    decides how or if an upgrade should take place. NodeLayoutUpgrade
    being the most interesting of these objects.
  • NodeLayoutUpgrade::ShouldUpgrade does a second traversal of the
    ancestor chain to understand if an upgrade is needed. It is not
    possible to answer this question in the first traversal
    definitively, since style (first pass) has not been updated yet
    at that time.

For forced updates which target nodes in display:none (or elements
without ComputedStyle in general), we can not tell ahead-of-time
whether or not something may require an upgrade, since the
DependsOnContainerQueries exists on ComputedStyle. Hence, for those
situations we defensively assume that we require an upgrade if we're
inside an interleaving root [1].

Pseudo-elements also needed adjustment in this CL (the change in
ElementRuleCollector). getComputedStyle will call NeedsLayoutTree-
UpdateForNodeIncludingDisplayLocked with the originating element,
so it is not enough to mark the pseudo-style with as
DependsOnContainerQueries, since we never actually observe that style
in NeedsLayoutTreeUpdateForNode(etc). Hence we mark the originating
element as well, although this will cause some over-invalidation in
some cases. (It's possible to iterate on this if needed).

[1] Interleaving root = basically a container-for-container-queries
that is not display:none.

Fixed: 1295717
Change-Id: I2c7d3a82dd513b618ad5245df9b3b6cd7e306d9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3472067
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#980529}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

We currently have pretty serious bugs in getComputedStyle (and similar
APIs that force style), since NeedsLayoutTreeUpdateForNodeIncluding-
DisplayLocked is completely unaware of container queries: if nothing
is *style dirty* in the ancestor chain, we will early-out from
UpdateStyleAndLayoutTreeForNode, and never actually update anything.
However, for container queries, we have to also check for layout-
dirtiness, since doing that layout can affect container-query-
dependent elements.

This CL tries to formalize the concept of a "layout upgrade", which
is a name for when UpdateStyleAndLayoutTree needs to also call
UpdateStyleAndLayout due layout dependencies.

 - NeedsLayoutTreeUpdateForNodeIncludingDisplayLocked needs to return
   true whenever we *may* need to upgrade. Otherwise we'll early-out,
   and never even get the chance to upgrade. Hence, we look at the
   target node and the ancestor chain, and try to figure out if a
   layout upgrade *will* be needed. We can not know this for sure
   at this time, because the subsequent call to update style may
   (for example) remove all container-query containers, i.e.
   remove layout dependencies.
 - UpdateStyleAndLayoutTree now accepts a LayoutUpgrade object, which
   decides how or if an upgrade should take place. NodeLayoutUpgrade
   being the most interesting of these objects.
 - NodeLayoutUpgrade::ShouldUpgrade does a second traversal of the
   ancestor chain to understand if an upgrade is needed. It is not
   possible to answer this question in the first traversal
   definitively, since style (first pass) has not been updated yet
   at that time.

For forced updates which target nodes in display:none (or elements
without ComputedStyle in general), we can not tell ahead-of-time
whether or not something may require an upgrade, since the
DependsOnContainerQueries exists on ComputedStyle. Hence, for those
situations we defensively assume that we require an upgrade if we're
inside an interleaving root [1].

Pseudo-elements also needed adjustment in this CL (the change in
ElementRuleCollector). getComputedStyle will call NeedsLayoutTree-
UpdateForNodeIncludingDisplayLocked with the originating element,
so it is not enough to mark the pseudo-style with as
DependsOnContainerQueries, since we never actually observe that style
in NeedsLayoutTreeUpdateForNode(etc). Hence we mark the originating
element as well, although this will cause some over-invalidation in
some cases. (It's possible to iterate on this if needed).

[1] Interleaving root = basically a container-for-container-queries
that is not display:none.

Fixed: 1295717
Change-Id: I2c7d3a82dd513b618ad5245df9b3b6cd7e306d9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3472067
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#980529}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants