Summary: | CSS Flexbox: dynamically applied align-items doesn't affect item alignment | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Masataka Yakura <myakura.web> | ||||||
Component: | Layout and Rendering | Assignee: | Tony Chang <tony> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cbiesinger, eric, esprehn+autocc, ojan.autocc, ojan, tony, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 62048 | ||||||||
Attachments: |
|
Description
Masataka Yakura
2013-02-20 06:40:23 PST
Definitely a bug. Not sure where though. We do check webkit-align-items in RenderStyle::diff. (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/ I see the bug. I'm working on a fix. Created attachment 190336 [details]
Patch
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. Created attachment 190339 [details]
Patch for landing
Comment on attachment 190339 [details] Patch for landing Clearing flags on attachment: 190339 Committed r144104: <http://trac.webkit.org/changeset/144104> All reviewed patches have been landed. Closing bug. |