New issue
Advanced search Search tips

Issue 788379 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Anonymous flex item siblings are not merged

Project Member Reported by futhark@chromium.org, Nov 24 2017

Issue description

Text node children of a flexible box are wrapped in anonymous flex items. Consecutive text node siblings should be wrapped in the same item. This happens on initial attachment, but when we dynamically remove flex items between text nodes we don't merge anonymous flex items.

 
flex-space-2.html
215 bytes View Download

Comment 1 Deleted

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 30 2017

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

commit 41321b10500c1137240cd2610a26dc4bb8465988
Author: Rune Lillesveen <futhark@chromium.org>
Date: Thu Nov 30 10:25:55 2017

Merge anonymous flex item siblings.

When we detach a flex item, we might end up with anonymous flex item
in-flow siblings which should be merged into a single flex item because
their text children makes up a contiguous run of text.

The case where a flex item goes from in-flow to absolute positioned is
not fixed by this CL. That is the reason for the failing sub-test.

Bug:  788379 ,  657748 
Change-Id: Idf0bee2b490982b9bc987d31ceccf894a64147f8
Reviewed-on: https://chromium-review.googlesource.com/795724
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520501}
[modify] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-001.html
[add] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-002.html
[add] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-003.html
[add] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-004.html
[add] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-005.html
[add] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-006.html
[add] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-ref.html
[modify] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
[modify] https://crrev.com/41321b10500c1137240cd2610a26dc4bb8465988/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h

Cc: futhark@chromium.org
Owner: ----
Status: Available (was: Started)
Since there's still an issue with elements changing position from static to absolute (flexabs.html), I'll keep this issue open.

Comment 4 by r...@igalia.com, Nov 30 2017

Cc: svil...@igalia.com jfernan...@igalia.com
I guess this happens too for CSS Grid Layout.
The very same test using "display:grid" shows 2 rows (one for "two" and another for "words") instead of just one with "two words".

I guess we should apply a similar solution maybe moving MergeAnonymousFlexItems() to a common place. WDYT?
I see that the grid spec has the exact same concept of anonymous items around contiguous text runs. It probably makes sense to move the common code to LayoutBlock and keep the flex/grid specifics in their respective classes.

I won't do that at this point, so feel free to pick it up.

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 31 2018

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

commit 277af9187ad3574d0946f08de078e86971f54fee
Author: Rune Lillesveen <futhark@chromium.org>
Date: Wed Jan 31 22:22:09 2018

Do not skip out-of-flow boxes merging anonymous flex items.

A misread of the spec made us skip out-of-flow boxes merging anonymous
flex items. Only merge anonymous flex items which are truly box
siblings.

Adjusted the wpt tests accordingly. Fwiw, Firefox nightly now passes
all of wpt/css/css-flexbox/anonymous-*

Bug:  788379 ,  806151 
Change-Id: I124740e5d2150becdcc03ded26773a69607e0bbb
Reviewed-on: https://chromium-review.googlesource.com/895283
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533440}
[modify] https://crrev.com/277af9187ad3574d0946f08de078e86971f54fee/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/277af9187ad3574d0946f08de078e86971f54fee/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-004.html
[modify] https://crrev.com/277af9187ad3574d0946f08de078e86971f54fee/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-005.html
[modify] https://crrev.com/277af9187ad3574d0946f08de078e86971f54fee/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-006.html
[add] https://crrev.com/277af9187ad3574d0946f08de078e86971f54fee/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-split-ref.html
[modify] https://crrev.com/277af9187ad3574d0946f08de078e86971f54fee/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 1 2018

Labels: merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fc019b14d169e2675b86d801337d493aba8bd63

commit 4fc019b14d169e2675b86d801337d493aba8bd63
Author: Rune Lillesveen <futhark@chromium.org>
Date: Thu Feb 01 23:38:01 2018

Do not skip out-of-flow boxes merging anonymous flex items.

A misread of the spec made us skip out-of-flow boxes merging anonymous
flex items. Only merge anonymous flex items which are truly box
siblings.

Adjusted the wpt tests accordingly. Fwiw, Firefox nightly now passes
all of wpt/css/css-flexbox/anonymous-*

TBR=cbiesinger@chromium.org

Bug:  788379 ,  806151 
Change-Id: I124740e5d2150becdcc03ded26773a69607e0bbb
Reviewed-on: https://chromium-review.googlesource.com/895283
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#533440}(cherry picked from commit 277af9187ad3574d0946f08de078e86971f54fee)
Reviewed-on: https://chromium-review.googlesource.com/898413
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#249}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/4fc019b14d169e2675b86d801337d493aba8bd63/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/4fc019b14d169e2675b86d801337d493aba8bd63/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-004.html
[modify] https://crrev.com/4fc019b14d169e2675b86d801337d493aba8bd63/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-005.html
[modify] https://crrev.com/4fc019b14d169e2675b86d801337d493aba8bd63/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-006.html
[add] https://crrev.com/4fc019b14d169e2675b86d801337d493aba8bd63/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/anonymous-flex-item-split-ref.html
[modify] https://crrev.com/4fc019b14d169e2675b86d801337d493aba8bd63/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp

Status: Fixed (was: Available)

Sign in to add a comment