Bug 110341 – CSS Flexbox: dynamically applied align-items doesn't affect item alignment

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Masataka Yakura]

Steps to reproduce:
1. go to http://jsfiddle.net/gegJt/embedded/result/
2. hit the button below the words "Item 1" and "Item 2"
3. check if the layout changes

Expected result:
Words "Item 1" and "Item 2" are horizontally aligned (looking "Item 1Item 2")

Actual result:
Words weren't aligned unless resizing window.

[Ojan Vafai]

Definitely a bug. Not sure where though. We do check webkit-align-items in RenderStyle::diff.

[Masataka Yakura]

(In reply to comment #1)
> Definitely a bug. Not sure where though. We do check webkit-align-items in RenderStyle::diff.

Perhaps somthing on text layout? It works fine on a modified testcase that set width and height explicitly.
http://jsfiddle.net/suW5F/embedded/result/

[Tony Chang]

I see the bug.  I'm working on a fix.

[Tony Chang]

Created attachment 190336 [details]
Patch

[Ojan Vafai]

Comment on attachment 190336 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190336&action=review

> Source/WebCore/ChangeLog:10
> +        The problem was with flex items that were previously stretch but didn't change width,
> +        we would not lay them out again. Since they were not layed out, we use the old stretch
> +        value. Fix this by marking flex items that were stretch as needing layout.

This grammar is a bit broken. I see what you're saying, but might be worth cleaning up.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:316
> +    if (oldStyle && oldStyle->alignItems() == AlignStretch && diff == StyleDifferenceLayout) {
> +        for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {
> +            EAlignItems previousAlignment = resolveAlignment(oldStyle, child->style());
> +            if (previousAlignment == AlignStretch && previousAlignment != resolveAlignment(style(), child->style()))
> +                child->setChildNeedsLayout(true, MarkOnlyThis);
> +        }
> +    }

I think this is correct, but it's tricky enough that it could use a brief "why" comment, e.g. why we only care about the alignment going from stretch to something else.

[Tony Chang]

Created attachment 190339 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 190339 [details]
Patch for landing

Clearing flags on attachment: 190339

Committed r144104: <http://trac.webkit.org/changeset/144104>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.