Whitespace layout text attached between first letter and remaining text after style change |
||||
Issue descriptionChrome 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.
,
Aug 21
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.
,
Aug 21
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.
,
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
,
Aug 21
,
Aug 21
Changing owner to the one who fixed it... |
||||
►
Sign in to add a comment |
||||
Comment 1 by xiaoche...@chromium.org
, Aug 21