Squad: first-letter update should not create layout objects during style recalc |
||||
Issue descriptionAccording to [1], we do. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1063891
,
Jul 23
In fast/css-generated-content/first-letter-next-sibling-crash.html, first-letter layout object is attached at the wrong place after style change and relayout.
Got:
LayoutBlockFlow {DIV}
LayoutInline {<pseudo:first-letter>}
LayoutTextFragment (anonymous) "P"
LayoutText {#text} " "
LayoutTextFragment {#text} "ASS if test does not crash."
Should be:
LayoutBlockFlow {DIV}
LayoutText {#text} " " (or omitted)
LayoutInline {<pseudo:first-letter>}
LayoutTextFragment (anonymous) "P"
LayoutTextFragment {#text} "ASS if test does not crash."
The root case seems to be that:
- During style recalc, first letter layout object is attached as the first child of LayoutBlockFlow {DIV}
- During layout, when examining the first child node " " of DIV, since the preceeding LayoutText "P" is non-whitespace, we create LayoutText " " for this whitespace text node and appended it to the layout tree
Will this be fixed if we stop first-letter layout object creation during style recalc?
,
Jul 23
,
Jul 26
> Will this be fixed if we stop first-letter layout object creation during style recalc? Most likely not. That sounds like a bug which needs to be addressed separately.
,
Jul 26
I have a patch getting rid of ReattachLayoutTree from style recalc, currently with two test regressions, but first-letter-next-sibling-crash.html still renders the space.
,
Jul 26
,
Jul 26
Thanks for the update! I'll wait until your patch lands.
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1fe30363aff9ce0192b7926a5018c6153c0fbe66 commit 1fe30363aff9ce0192b7926a5018c6153c0fbe66 Author: Rune Lillesveen <futhark@chromium.org> Date: Tue Jul 31 12:10:47 2018 Moved SVG check for ::first-letter to CanGeneratePseudoElement. Was currently just checked when trying to create the pseudo element. This change should have no effect, but is done in preparation for [1]. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1155591 Bug: 847218 Change-Id: I99fa06bed1256cb6405e6f24a5f116def366acf9 Reviewed-on: https://chromium-review.googlesource.com/1156389 Reviewed-by: Hayato Ito <hayato@chromium.org> Commit-Queue: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#579376} [modify] https://crrev.com/1fe30363aff9ce0192b7926a5018c6153c0fbe66/third_party/blink/renderer/core/dom/element.cc
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98bab02ddde547f174db834ec0efdbf920479c72 commit 98bab02ddde547f174db834ec0efdbf920479c72 Author: Rune Lillesveen <futhark@chromium.org> Date: Wed Aug 01 15:09:02 2018 ClearChildNeedsReattachLayoutTree after pseudo elements done. Instead of doing this call inside RebuildChildrenLayoutTrees, wait until we have finished all pseudo element children as well. This is done in preparation for ::first-letter update changes and split out from [1]. The other call-site for RebuildChildrenLayoutTrees, ShadowRoot::RebuildLayoutTree, already cleared this flag afterwards. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1155591 Bug: 847218 Change-Id: I72816c5249bcb863fab906ffef90c026619d8165 Reviewed-on: https://chromium-review.googlesource.com/1156387 Reviewed-by: Anders Ruud <andruud@chromium.org> Commit-Queue: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#579806} [modify] https://crrev.com/98bab02ddde547f174db834ec0efdbf920479c72/third_party/blink/renderer/core/dom/container_node.cc [modify] https://crrev.com/98bab02ddde547f174db834ec0efdbf920479c72/third_party/blink/renderer/core/dom/element.cc
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66094461eb2abfc3519124c70eb3e2797fff960f commit 66094461eb2abfc3519124c70eb3e2797fff960f Author: Rune Lillesveen <futhark@chromium.org> Date: Tue Aug 07 20:06:20 2018 Don't re-attach ::first-letter during style recalc. Update (create or destroy) the FirstLetterPseudoElement and its style at the point where the block level element's layout sub-tree is up-to- date. This means we will always attach the layout tree for the ::first-letter element in Element::AttachLayoutTree, but the pseudo element and its style may be created/updated at style recalc time, layout tree rebuild time, or layout tree attachment time depending on when we know what will be the LayoutText from which we will format the first letter if any. UpdateFirstLetterPseudoElement is split out from UpdatePseudoElement to make the code easier to read as the former case has some exceptional cases. We no longer use the pseudo style cache for ::first-letter as we will now compute the style only once per pass with the correct inheritance parent. Bug: 847218 Change-Id: I7a1e2a60122891fa38998ff85e566bec0a38b513 Reviewed-on: https://chromium-review.googlesource.com/1155591 Reviewed-by: Anders Ruud <andruud@chromium.org> Commit-Queue: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#581320} [add] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/WebKit/LayoutTests/external/wpt/css/css-pseudo/first-letter-and-sibling-display-change.html [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/WebKit/LayoutTests/fast/css/dynamic-class-pseudo-elements-expected.txt [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/WebKit/LayoutTests/fast/css/dynamic-class-pseudo-elements.html [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/blink/renderer/core/dom/element.cc [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/blink/renderer/core/dom/element.h [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/blink/renderer/core/dom/first_letter_pseudo_element.cc [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/blink/renderer/core/dom/first_letter_pseudo_element.h [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/blink/renderer/core/dom/layout_tree_builder.cc [modify] https://crrev.com/66094461eb2abfc3519124c70eb3e2797fff960f/third_party/blink/renderer/core/style/computed_style.h
,
Aug 7
|
||||
►
Sign in to add a comment |
||||
Comment 1 by yosin@chromium.org
, May 29 2018