New issue
Advanced search Search tips

Issue 847218 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Squad: first-letter update should not create layout objects during style recalc

Project Member Reported by futhark@chromium.org, May 28 2018

Issue description

Comment 1 by yosin@chromium.org, May 29 2018

Cc: yosin@chromium.org
"first-letter-removed-added.html" causes attaching layout object during RecalStyle().
See below stack trace:

Document::GetReattachLegacyLayoutObjectList() Line 336
LayoutTreeBuilderForElement::CreateLayoutObject() Line 141
LayoutTreeBuilderForElement::CreateLayoutObjectIfNeeded() Line 106
Element::AttachLayoutTree(blink::Node::AttachContext & context) Line 1965
PseudoElement::AttachLayoutTree(blink::Node::AttachContext & context) Line 144
FirstLetterPseudoElement::AttachLayoutTree(blink::Node::AttachContext & context) Line 245
Node::ReattachLayoutTree(blink::Node::AttachContext & context) Line 1095
Node::ReattachLayoutTree() Line 640
Element::UpdateFirstLetter(blink::Element * element) Line 3864
Element::UpdatePseudoElement(blink::PseudoId pseudo_id, blink::StyleRecalcChange change) Line 3815
Element::RecalcStyle(blink::StyleRecalcChange change) Line 2205
ContainerNode::RecalcDescendantStyles(blink::StyleRecalcChange change) Line 1334
Element::RecalcStyle(blink::StyleRecalcChange change) Line 2195
Document::UpdateStyle() Line 2241
Document::UpdateStyleAndLayoutTree() Line 2157
Document::FinishedParsing() Line 5857
HTMLConstructionSite::FinishedParsing() Line 621
HTMLTreeBuilder::Finished() Line 2750
HTMLDocumentParser::end() Line 898
HTMLDocumentParser::AttemptToRunDeferredScriptsAndEnd() Line 911
HTMLDocumentParser::PrepareToStopParsing() Line 244
HTMLDocumentParser::ProcessTokenizedChunkFromBackgroundParser(std::unique_ptr<blink::HTMLDocumentParser::TokenizedChunk,std::default_delete<blink::HTMLDocumentParser::TokenizedChunk> > pop_chunk) Line 549
HTMLDocumentParser::PumpPendingSpeculations() Line 595
HTMLDocumentParser::ResumeParsingAfterYield() Line 272
HTMLParserScheduler::ContinueParsing() Line 150

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?
Cc: xiaoche...@chromium.org
> 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.

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.

Status: Started (was: Assigned)
Thanks for the update! I'll wait until your patch lands.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment