New issue
Advanced search Search tips

Issue 863040 link

Starred by 1 user
Status: Fixed
Owner:
mstensho@chromium.org
Closed: Jul 31
Cc:
yosin@chromium.org
futhark@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[LayoutNG] Failure to force legacy layout on the entire BFC

Project Member Reported by mstensho@chromium.org, Jul 12 
The code that attempts to force legacy layout for an entire BFC when a contenteditable element is encountered doesn't always work.

If we have something in the BFC that doesn't generate a layout box, but later on gets a child with a box, we'll fail to use legacy layout on that child. The reason is that we don't update the ForceLegacyLayout on the ComputedStyle for box-less nodes.

See attachment. It uses a display:contents element (which doesn't generate a box, but may have children that do). Later on, we add a regular block child. This one gets an NG object, while all the other objects in the BFC have legacy objects.

A similar problem occurs in shadow DOM. Slot elements don't generate layout boxes by default, but its children may. https://chromium-review.googlesource.com/c/chromium/src/+/1127027 adds DCHECKs that fail if we switch engines within a BFC, and then external/wpt/html/editing/focus/tabindex-focus-flag.html fails: https://test-results.appspot.com/data/layout_results/linux_layout_tests_layout_ng/7841/layout-test-results/results.html
tc.html
558 bytes View Download
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 31 

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

commit 5e33dcbec0fb31c4bbd5a8535045cd6bc7c7bd8a
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Tue Jul 31 09:30:45 2018

[LayoutNG] Force legacy layout on the entire subtree when editable.

When we decide to switch over to legacy layout for a block formatting
context (and the entire subtree) because there's something editable, we
used to only set ForceLegacyLayout on those nodes that create layout
objects, but that isn't enough. There may be descendants of box-less
nodes that may create layout objects later on, and, since
ForceLegacyLayout is an inherited "property", we need to set it on all
potential parent nodes. Otherwise we risk inserting NG objects that
participate in a legacy block formatting context, which isn't allowed
[1].

This matters when we have display:contents (which is very common
situation for shadow DOM slot elements, for instance).

Checking with the layout parent's style whether to force legacy layout
or not fixes the problem.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1127027
enforces this.

Bug:  863040 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I6133c20493017b4648b153f2c7558c76589c3d89
Reviewed-on: https://chromium-review.googlesource.com/1136445
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579360}
[modify] https://crrev.com/5e33dcbec0fb31c4bbd5a8535045cd6bc7c7bd8a/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[add] https://crrev.com/5e33dcbec0fb31c4bbd5a8535045cd6bc7c7bd8a/third_party/WebKit/LayoutTests/fast/block/margin-collapse/display-contents-contenteditable.html
[modify] https://crrev.com/5e33dcbec0fb31c4bbd5a8535045cd6bc7c7bd8a/third_party/blink/renderer/core/css/resolver/style_adjuster.cc

Comment 2 by mstensho@chromium.org, Jul 31

Owner: mstensho@chromium.org
Status: Fixed (was: Available)
►

Sign in to add a comment