New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Vacation
Closed: Jan 28
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocked on:
issue 915669
issue 920599
issue 920600



Sign in to add a comment
link

Issue 914784: Squad: unified computed style storage

Reported by futhark@chromium.org, Dec 13 Project Member

Issue description

We should store ComputedStyle on Element for rendered elements. Currently we store ComputedStyle for elements only for display:contents, a couple of other elements which do not get a LayoutObject, and ComputedStyle constructed for getComputedStyle inside display:none subtrees. Use the ComputedStyle member in NodeRenderingData for all such ComputedStyles and remove the ComputedStyle member from ElementRareData.
 

Comment 1 by bugdroid1@chromium.org, Dec 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d4424206c7dbd6b1aea1da44aab92ba30e955d4

commit 7d4424206c7dbd6b1aea1da44aab92ba30e955d4
Author: Rune Lillesveen <futhark@chromium.org>
Date: Fri Dec 14 09:23:04 2018

Use custom computed style instead of LayoutObjectIsNeeded.

Using display:none in the computed style in the UA shadow for native
appearance makes layout tree changes easier to detect than having to
know the LayoutObjectIsNeeded is going to return a different result.
This caused an issue in the WIP CL for unified computed style storage.

Bug:  914784 
Change-Id: Icad9726c91972e9aca7566403cbe5650ea821c93
Reviewed-on: https://chromium-review.googlesource.com/c/1375736
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616619}
[modify] https://crrev.com/7d4424206c7dbd6b1aea1da44aab92ba30e955d4/third_party/blink/renderer/core/html/shadow/progress_shadow_element.cc
[modify] https://crrev.com/7d4424206c7dbd6b1aea1da44aab92ba30e955d4/third_party/blink/renderer/core/html/shadow/progress_shadow_element.h

Comment 2 by futhark@chromium.org, Dec 17

Blockedon: 915669

Comment 3 by bugdroid1@chromium.org, Dec 17

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/157d9c65bb9a5b9c5fb0fb37543830679e1846d4

commit 157d9c65bb9a5b9c5fb0fb37543830679e1846d4
Author: Rune Lillesveen <futhark@chromium.org>
Date: Mon Dec 17 19:50:19 2018

Do not use a local WhitespaceAttacher for <content>.

<content> elements are not in the flat tree and should be treated like
display:contents in order to attach whitespace LayoutText objects
correctly.

This bug caused regressions for the unification of ComputedStyle
storage.

Bug:  915669 ,  914784 
Change-Id: I8afce3bd7fd67244a8784e17acbefc6dd068a8e3
Reviewed-on: https://chromium-review.googlesource.com/c/1379767
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617193}
[modify] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/renderer/core/dom/element.cc
[add] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/web_tests/fast/dom/shadow/content-whitespace-attach-expected.html
[add] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/web_tests/fast/dom/shadow/content-whitespace-attach-expected.txt
[add] https://crrev.com/157d9c65bb9a5b9c5fb0fb37543830679e1846d4/third_party/blink/web_tests/fast/dom/shadow/content-whitespace-attach.html

Comment 4 by bugdroid1@chromium.org, Jan 8

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bba43c0b375b05007b19acc3730616c35fa5038f

commit bba43c0b375b05007b19acc3730616c35fa5038f
Author: Rune Lillesveen <futhark@chromium.org>
Date: Tue Jan 08 18:16:44 2019

Check LayoutView parent for top layer siblings.

The check for the need for re-attachment would no longer work with the
current patch in [1]. Instead check if the sibling LayoutObject
candidate is in the top layer (LayoutView child) already, in which case
we can use it as a sibling. If the candidate needs re-attaching, it will
be removed and re-inserted in the correct place.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1323555

TEST=html/dialog

Bug:  914784 
Change-Id: I5df489382fb95ca6de8c5ea25d0270206f2fbe4a
Reviewed-on: https://chromium-review.googlesource.com/c/1400698
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620795}
[modify] https://crrev.com/bba43c0b375b05007b19acc3730616c35fa5038f/third_party/blink/renderer/core/dom/layout_tree_builder_traversal.cc

Comment 5 by bugdroid1@chromium.org, Jan 8

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66bc943f8f0557fc1d522546df16f4752db8014a

commit 66bc943f8f0557fc1d522546df16f4752db8014a
Author: Rune Lillesveen <futhark@chromium.org>
Date: Tue Jan 08 21:50:31 2019

We never ask for text style without parent style.

Just DCHECK that the layout tree parent node is non-null.

This is split out of:

https://chromium-review.googlesource.com/c/chromium/src/+/1323555.

Bug:  914784 

Change-Id: Ib1e17f2cb9279d1f3c87fb0d3d42af937d3ca3ca
Reviewed-on: https://chromium-review.googlesource.com/c/1400689
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620891}
[modify] https://crrev.com/66bc943f8f0557fc1d522546df16f4752db8014a/third_party/blink/renderer/core/css/resolver/style_resolver.cc

Comment 6 by futhark@chromium.org, Jan 10

Blockedon: 920599

Comment 7 by futhark@chromium.org, Jan 10

Blockedon: 920600

Comment 8 by bugdroid1@chromium.org, Jan 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55321ed58a44b095732e4c59ad889923ab3f676e

commit 55321ed58a44b095732e4c59ad889923ab3f676e
Author: Rune Lillesveen <futhark@chromium.org>
Date: Mon Jan 14 11:01:08 2019

Avoid style recalc in display:none subtrees.

ParentComputedStyle() should be null in display:none subtrees to avoid
computing style unnecessarily when we start at a recalc root inside a
display:none subtree where the parent of the recalc root has a
ComputedStyle because it's forced by Window.getComputedStyle().

First, we introduce a flag to mark ComputedStyle as being forced inside
a display:none subtree. This is strictly not necessary for the current
code since we can check where it is stored, but for the
unify-computed-style storage issue (914784), we will start storing
ComputedStyle in the same place for both rendered and display:none
elements, so we'll need it soon.

Let ParentComputedStyle() return null when this flag is set to make sure
display:none subtrees stay free of ComputedStyles unless enforced.

Bug:  920600 ,  914784 
Change-Id: Iea07af009c0237d4ac6ba155af774d2e3dece354
Reviewed-on: https://chromium-review.googlesource.com/c/1404171
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622414}
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/css/style_engine_test.cc
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/dom/node_computed_style.h
[modify] https://crrev.com/55321ed58a44b095732e4c59ad889923ab3f676e/third_party/blink/renderer/core/style/computed_style_extra_fields.json5

Comment 9 by bugdroid1@chromium.org, Jan 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f930f101b81d07cd66b0642d10c88edd1a93d2d7

commit f930f101b81d07cd66b0642d10c88edd1a93d2d7
Author: Rune Lillesveen <futhark@chromium.org>
Date: Mon Jan 14 12:31:01 2019

Don't compute style in display:none iframes.

display:none iframes do not allow the layout tree to be generated, but
when Squad introduced style recalc for reattachment and
SetNonAttachedStyle(), style computation was no longer blocked by
AttachLayoutTree and the check in LayoutView::CanHaveChildren(). This
lead to style being computed as normal in display:none iframes.

Return null from ParentComputedStyle() when ChildrenCanHaveStyle()
returns false and override ChildrenCanHaveStyle() to return false for
Document when LayoutView::CanHaveChildren() returns false.

Bug:  920599 ,  914784 
Change-Id: Ibb8f4033cb1347c535e0b53d3e2ab9b46aacbd4e
Reviewed-on: https://chromium-review.googlesource.com/c/1405010
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622431}
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/container_node.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/dom/node_computed_style.h
[modify] https://crrev.com/f930f101b81d07cd66b0642d10c88edd1a93d2d7/third_party/blink/renderer/core/layout/layout_view_test.cc

Comment 11 by 42576172...@developer.gserviceaccount.com, Jan 14

Project Member

Comment 13 by 42576172...@developer.gserviceaccount.com, Jan 25

Project Member

Comment 14 by bugdroid, Jan 28

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e610731bc6f24b929249d336835a865a791a69b

commit 3e610731bc6f24b929249d336835a865a791a69b
Author: Rune Lillesveen <futhark@chromium.org>
Date: Mon Jan 28 10:03:22 2019

Store ComputedStyle in NodeRenderingData for Element.

We used to store ComputedStyle in NodeRenderingData temporarily between
style recalc and layout tree building as non-attached-style. After
layout tree attachment, we would have to look up ComputedStyle from the
LayoutObjects. Additionally, for element which were not rendered like
elements styled with display:none or display:contents, we would store
ComputedStyle in ElementRareData.

This CL changes that to persist ComputedStyle in NodeRenderingData both
when rendered with a LayoutObject, or display:content, or when ensured
from getComputedStyle in display:none subtrees.

There are a few reasons for doing this:

* We don't want to store ComputedStyle in two different place in
  Element.
* For LayoutNG and Houdini, we might not have LayoutObjects to pull the
  ComputedStyle from.
* Changing the display property of a subtree root should not cause a
  full style recalc of the subtree if only that root display type is
  the only style change.
* We got the possibility to move ComputedStyle/animation/recalc
  dependencies out of AttachLayoutTree methods in this CL.

In addition to store the ComputedStyle in a single place in Element, we
make the following changes:

1. New nodes are now style clean per default, and all nodes are style
clean before layout tree building. Instead of requiring a full style
recalc traversal of the whole subtree when inserting a subtree, we mark
the root with kLocalStyleChange and skip recalc of display:none
subtrees in the following recalc.

2. Made StyleRecalcChange a class instead of an enum to separate style
   recalc propagation and the need for layout tree reattachment.

3. Style recalc now happens down the flat tree which means we got rid
   of the hack of marking shadow host children dirty for inheritance
   through slots.

Bug:  914784 

Change-Id: I55a9ac7b4e3bbf0b23c78f10941dfb2a86e1bb88
Reviewed-on: https://chromium-review.googlesource.com/c/1323555
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626478}

Comment 15 by futhark@chromium.org, Jan 28

Status: Fixed (was: Started)

Sign in to add a comment