Bug 110341 - CSS Flexbox: dynamically applied align-items doesn't affect item alignment
Summary: CSS Flexbox: dynamically applied align-items doesn't affect item alignment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency treegraph
 
Reported: 2013-02-20 06:40 PST by Masataka Yakura
Modified: 2013-02-26 14:00 PST (History)
7 users (show)

See Also:


Attachments
Patch (8.65 KB, patch)
2013-02-26 11:51 PST, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (8.88 KB, patch)
2013-02-26 12:24 PST, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Masataka Yakura 2013-02-20 06:40:23 PST
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.
Comment 1 Ojan Vafai 2013-02-25 18:33:15 PST
Definitely a bug. Not sure where though. We do check webkit-align-items in RenderStyle::diff.
Comment 2 Masataka Yakura 2013-02-26 04:53:18 PST
(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/
Comment 3 Tony Chang 2013-02-26 10:59:02 PST
I see the bug.  I'm working on a fix.
Comment 4 Tony Chang 2013-02-26 11:51:28 PST
Created attachment 190336 [details]
Patch
Comment 5 Ojan Vafai 2013-02-26 12:03:19 PST
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.
Comment 6 Tony Chang 2013-02-26 12:24:58 PST
Created attachment 190339 [details]
Patch for landing
Comment 7 WebKit Review Bot 2013-02-26 14:00:29 PST
Comment on attachment 190339 [details]
Patch for landing

Clearing flags on attachment: 190339

Committed r144104: <http://trac.webkit.org/changeset/144104>
Comment 8 WebKit Review Bot 2013-02-26 14:00:33 PST
All reviewed patches have been landed.  Closing bug.