New issue
Advanced search Search tips

Issue 876150 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 699017



Sign in to add a comment

Whitespace layout text attached between first letter and remaining text after style change

Project Member Reported by xiaoche...@chromium.org, Aug 21

Issue description

Chrome Version: 68.0.3440.103
OS: Linux

What steps will reproduce the problem?
(1) Load http://jsfiddle.net/03yLz86u/12/
(2) Observe

What is the expected result?

Shouldn't render a space between "f" and "oo"

What happens instead?

A space is rendered between "f" and "oo"

Please use labels and text to provide additional information.

If this is a regression (i.e., worked before), please consider using the
bisect tool (https://www.chromium.org/developers/bisect-builds-py) to help
us identify the root cause and more rapidly triage the issue.

For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.


 
Blocking: 699017
Note: this bug also affects LayoutNG, as it breaks an assumptions in NGOffsetMapping that, text layout objects of the same node should appear consecutively in the layout tree.
Cc: futhark@chromium.org
There seem to be multiple causes of the issue, making it tricky to make the issue user-visible. One test case is:

<div> <!---->foo</div>

The style change is

div::first-letter{float: right; color: red} => div::first-letter{color: red}

==========================================

Recap:

DOM tree (which remains the same after style change)

<div>
  <pseudo:first-letter>
  " "
  comment
  "foo"

0. Old layout tree before relayout

LayoutBlockFlow div
  LayoutBlockFlow float pseudo:first-letter
    LayoutTextFragment "f"
  LayoutTextFragment "oo"

1. Style recalc, which doesn't change DOM

2. Element::RebuildLayoutTree invoked on <div>, which visits child nodes in backward ordering

3. "foo", comment and " " are visited, without reattaching any layout object

4. <pseudo:first-letter> is visited and has its layout tree rebuilt

5. WhitespaceAttacher scans the siblings of <pseudo:first-letter> to decide if any whitespace text node needs reattaching.
  5.1 WA sets AttachContext.previous_in_flow as <pseudo:first-letter>'s layout object
  5.2 WA (indirectly) calls Text::TextLayoutObjectIsNeeded() on the next sibling " " with the AttachContext, which decides that a text layout object is needed

6. A LayoutText for " " is created and inserted into LayoutTree. The insertion point is calculated by LayoutTreeBuilder::NextLayoutObject() as the remaining text fragment "oo"

============================================

There are two questionable steps in the above recap.

- The purpuse of step 5 seems wrong. Step 5 tries to find any whitespace text node that should be attached after the first-letter layout object. However, in our example, " " should be either part of first letter, or before first letter

- The insertion point calculated in step 6 also seems wrong. In any case, we shouldn't insert another text layout object immediately before the remaining text layout object.
We cannot use the WhitespaceAttacher for that case, or we need to initialize it with the correct text node for the ::first-letter case. Still we want to make sure we detach the whitespace text node preceding the first-letter text node when it's predecessor was detached. You could have the case:

<style>.float::first-letter { float: left } div::first-letter { color:green }</style>
<div id=d class=float><span id=s>X</span> <!---->Foo</span></div>
<script>
  s.style.display = "none";
  d.className = "";
</script>

I think the solution is to pass a clean WhitespaceAttacher (not child_attacher) for RebuildPseudoElementLayoutTree for ::first-letter and walk layout tree siblings of the first-letter text node if the first-letter includes the last non-white-space code point of the text node.

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/046e4ea777d97afc039f995149956733527586ab

commit 046e4ea777d97afc039f995149956733527586ab
Author: Rune Lillesveen <futhark@chromium.org>
Date: Tue Aug 21 18:55:49 2018

Don't do whitespace re-attachment for ::first-letter.

If we have a white-space node before the text node with the first
letter, we would try to re-attach the white-space node with an incorrect
previous in-flow LayoutObject because we attach the ::first-letter after
all children have been attached, not where the ::first-letter would end
up in the layout tree.

We don't need to re-attach whitespace after ::first-letter because:

* We always create a remaining LayoutTextFragment for the text node
  which contains the first formatted letter. Wether the remaining text
  consists of white-space or is even empty.

* If the LayoutObject sibling following first-letter+remaining-text
  consists of white-space, it would have gotten a LayoutObject when
  attaching the LayoutText for the text node where the first-letter
  comes from.

That is, we will always have a LayoutObject for a white-space LayoutText
sibling of a first-letter LayoutObject.

Split out a separate method for ::first-letter for less complexity and
cost with which WhitespaceAttacher to use.

Bug:  876150 
Change-Id: Ib1f34f0ba596d5fa5f1c7fcbd7c75550dd4f9d12
Reviewed-on: https://chromium-review.googlesource.com/1183231
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584866}
[add] https://crrev.com/046e4ea777d97afc039f995149956733527586ab/third_party/WebKit/LayoutTests/external/wpt/css/css-pseudo/first-letter-and-whitespace-ref.html
[add] https://crrev.com/046e4ea777d97afc039f995149956733527586ab/third_party/WebKit/LayoutTests/external/wpt/css/css-pseudo/first-letter-and-whitespace.html
[modify] https://crrev.com/046e4ea777d97afc039f995149956733527586ab/third_party/WebKit/LayoutTests/fast/css-generated-content/first-letter-next-sibling-crash-expected.txt
[modify] https://crrev.com/046e4ea777d97afc039f995149956733527586ab/third_party/WebKit/LayoutTests/fast/css-generated-content/float-first-letter-siblings-convert-to-inline-expected.txt
[modify] https://crrev.com/046e4ea777d97afc039f995149956733527586ab/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/046e4ea777d97afc039f995149956733527586ab/third_party/blink/renderer/core/dom/element.h

Status: Fixed (was: Started)
Cc: -futhark@chromium.org xiaoche...@chromium.org
Owner: futhark@chromium.org
Changing owner to the one who fixed it...

Sign in to add a comment