Pass nextTextSibling to ::before layout rebuild.

Pass nextTextSibling to ::before layout rebuild.

Correct whitespace re-attachment relies on the next text node being
tracked in order to re-attach when a previous sibling element changes
display type. We did that just while traversing light tree siblings or
slotted siblings.

::before pseudo element display type may affect following whitespace
text node, so we need to pass on the next text sibling result from the
descendant traversal.

As we traverse siblings from right-to-left, we should also rebuild
::after before DOM siblings, and ::before after DOM siblings.

This fixes the case where an element, whose left-most child, or shadow
root child, is a whitespace text node, changes its ::before element's
display type from block to inline.

The pseudo element layout tree rebuild is renamed from reattach* to
rebuild* to match the semantics of the rest of the methods.

R=esprehn@chromium.org
BUG=648951
Review-Url: https://codereview.chromium.org/2766163002
Cr-Commit-Position: refs/heads/master@{#460737}
Committed: https://chromium.googlesource.com/chromium/src/+/06fa41652fb4ba0654a28453491c8347109d5620

[rune, 2017-03-22 10:57:09]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 10:57:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 12:23:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 12:23:26]

Dry run: This issue passed the CQ dry run.

[rune, 2017-03-22 14:11:52]

rune@opera.com changed reviewers:
+ nainar@chromium.org

[rune, 2017-03-22 14:11:52]

This is fixing whitespace reattachment involving ::before. This is one instance
of the issue where the whitespace layout object sibling is not a DOM light tree
sibling of the node which has the layout object it relies on. Two other cases
(first being pseudo elements) are flat tree siblings and children of
display:contents siblings.

This change illustrates how nextTextSibling needs to be passed on between
methods to be re-attached at appropriate places. The plan[1] is to contain more
of the nextTextSibling logic inside a WhitespaceAttacher object that is passed
instead of the Text* reference.

[1]
https://docs.google.com/document/d/1CFk5_3Gx8bPSsKdJ2twZhceUIrtySasdsij9t1niN...

[rune, 2017-03-27 08:22:59]

ping

[nainar, 2017-03-28 18:44:10]

lgtm

[rune, 2017-03-28 19:41:18]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2017-03-28 19:41:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2017-03-28 20:53:25]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2017-03-28 20:56:51]

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1331: Text*
ContainerNode::findNextTextSibling() const {
This needs a better name, it's doing something very specific that's not just
about next text siblings, it's also looking at stuff like what's in and out of
flow.

This is only used for ShadowRoot, put it in ShadowRoot.cpp as a function not a
method?

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1332: for (Node* sibling =
firstChild(); sibling;
This is confusing, the function says sibling, but the method is looking at kids?

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1337: if (layoutObject &&
!layoutObject->isFloatingOrOutOfFlowPositioned())
Why is it safe to call this here? Wouldn't the style be out of date?

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:2044:
rebuildPseudoElementLayoutTree(PseudoIdBefore, nextTextSibling);
Why do we only need this for ::before?

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:2069: nextTextSibling =
root->findNextTextSibling();
ShadowRoot can't have siblings so this doesn't make sense.

[esprehn, 2017-03-28 20:57:37]

I canceled the commit since ContainerNode::findNextTextSibling is a super
generic name, and the function is not finding siblings, and is also not just
finding text but looking at style and in/out of flow state.

[rune, 2017-03-29 10:39:47]

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1331: Text*
ContainerNode::findNextTextSibling() const {
On 2017/03/28 20:56:51, esprehn wrote:
> This needs a better name, it's doing something very specific that's not just
> about next text siblings, it's also looking at stuff like what's in and out of
> flow.
> 
> This is only used for ShadowRoot, put it in ShadowRoot.cpp as a function not a
> method?

PS2 adds a new method on ShadowRoot instead.

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1332: for (Node* sibling =
firstChild(); sibling;
On 2017/03/28 20:56:51, esprehn wrote:
> This is confusing, the function says sibling, but the method is looking at
kids?

Acknowledged.

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContainerNode.cpp:1337: if (layoutObject &&
!layoutObject->isFloatingOrOutOfFlowPositioned())
On 2017/03/28 20:56:51, esprehn wrote:
> Why is it safe to call this here? Wouldn't the style be out of date?

This code should be executed during rebuildLayoutTree only, at which point the
distribution and style is all clean, and the shadow subtree does not need layout
tree rebuilding. See PS2 which adds a lot of DCHECKs.

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:2044:
rebuildPseudoElementLayoutTree(PseudoIdBefore, nextTextSibling);
On 2017/03/28 20:56:51, esprehn wrote:
> Why do we only need this for ::before?

We always evaluate the need for a whitespace layout object based on the
preceding sibling layout object or its parent layout object. Without support for
display:contents, you cannot have ::after as the preceding layout object sibling
of a text layout object, but you can have a ::before as the preceding layout
object sibling:

<span><span::before>A</span::before> <span::after>B</span></span>

https://codereview.chromium.org/2766163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:2069: nextTextSibling =
root->findNextTextSibling();
On 2017/03/28 20:56:51, esprehn wrote:
> ShadowRoot can't have siblings so this doesn't make sense.

Rewrote this for PS2.

[rune, 2017-03-29 10:40:35]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 10:41:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 12:14:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 12:14:29]

Dry run: This issue passed the CQ dry run.

[rune, 2017-03-30 08:17:29]

esprehn@ ptal?

[esprehn, 2017-03-30 09:30:33]

lgtm

https://codereview.chromium.org/2766163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/2766163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:179: // re-attachment.
In that case, we find update nextTextSibling with the first
Find is weird grammar?

[rune, 2017-03-30 09:49:48]

https://codereview.chromium.org/2766163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp (right):

https://codereview.chromium.org/2766163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/shadow/ShadowRoot.cpp:179: // re-attachment.
In that case, we find update nextTextSibling with the first
On 2017/03/30 09:30:33, esprehn wrote:
> Find is weird grammar?

Done.

[rune, 2017-03-30 09:49:54]

The CQ bit was checked by rune@opera.com

[rune, 2017-03-30 09:49:54]

The patchset sent to the CQ was uploaded after l-g-t-m from nainar@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2766163002/#ps40001
(title: "Documentation fix.")

[commit-bot: I haz the power, 2017-03-30 09:50:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 13:00:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490867394361360,
"parent_rev": "3d723ac3847809a99d6e46da60b297a0eb816c88", "commit_rev":
"06fa41652fb4ba0654a28453491c8347109d5620"}

[commit-bot: I haz the power, 2017-03-30 13:01:31]

Description was changed from

==========
Pass nextTextSibling to ::before layout rebuild.

Correct whitespace re-attachment relies on the next text node being
tracked in order to re-attach when a previous sibling element changes
display type. We did that just while traversing light tree siblings or
slotted siblings.

::before pseudo element display type may affect following whitespace
text node, so we need to pass on the next text sibling result from the
descendant traversal.

As we traverse siblings from right-to-left, we should also rebuild
::after before DOM siblings, and ::before after DOM siblings.

This fixes the case where an element, whose left-most child, or shadow
root child, is a whitespace text node, changes its ::before element's
display type from block to inline.

The pseudo element layout tree rebuild is renamed from reattach* to
rebuild* to match the semantics of the rest of the methods.

R=esprehn@chromium.org
BUG=648951
==========

to

==========
Pass nextTextSibling to ::before layout rebuild.

Correct whitespace re-attachment relies on the next text node being
tracked in order to re-attach when a previous sibling element changes
display type. We did that just while traversing light tree siblings or
slotted siblings.

::before pseudo element display type may affect following whitespace
text node, so we need to pass on the next text sibling result from the
descendant traversal.

As we traverse siblings from right-to-left, we should also rebuild
::after before DOM siblings, and ::before after DOM siblings.

This fixes the case where an element, whose left-most child, or shadow
root child, is a whitespace text node, changes its ::before element's
display type from block to inline.

The pseudo element layout tree rebuild is renamed from reattach* to
rebuild* to match the semantics of the rest of the methods.

R=esprehn@chromium.org
BUG=648951

Review-Url: https://codereview.chromium.org/2766163002
Cr-Commit-Position: refs/heads/master@{#460737}
Committed:
https://chromium.googlesource.com/chromium/src/+/06fa41652fb4ba0654a28453491c...
==========

[commit-bot: I haz the power, 2017-03-30 13:01:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/06fa41652fb4ba0654a28453491c...