Bug 75782

Summary: Implement flex-align
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: Layout and RenderingAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, hyatt, koivisto, macpherson, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62048    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Comment 1 Ojan Vafai 2012-01-12 15:42:46 PST
Created attachment 122322 [details]
Patch
Comment 2 Tony Chang 2012-01-12 15:56:53 PST
Comment on attachment 122322 [details]
Patch

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

r- because of missing test cases.

> Source/WebCore/rendering/RenderBox.cpp:1803
> +        EFlexAlign align = style()->flexItemAlign();
> +        if (align == AlignAuto)
> +            align = parent()->style()->flexAlign();
> +        if (align != AlignStretch)
> +            return true;

Nit: I would probably just do "if (itemalign == stretch || (itemalign == auto && parent->itemalign == stretch))".  *shrug*

> Source/WebCore/rendering/RenderFlexibleBox.cpp:621
> +static EFlexAlign flexAlign(RenderBox* flexItem)

Nit: Maybe name it flexAlignForChild?

> LayoutTests/css3/flexbox/flex-align.html:124
> +</div>
> +

It would be nice to have a test with flex-align set and flex-item-align not set.

> LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt:147
> --webkit-flex-item-align: stretch;
> +-webkit-flex-align: stretch;

There are port specific results for these tests too, right?
Comment 3 Ojan Vafai 2012-01-12 19:03:51 PST
Created attachment 122359 [details]
Patch
Comment 4 Tony Chang 2012-01-12 19:11:16 PST
Please make sure to update all the other platform results before landing.
Comment 5 WebKit Review Bot 2012-01-12 20:04:15 PST
Comment on attachment 122359 [details]
Patch

Attachment 122359 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11227415

New failing tests:
svg/css/getComputedStyle-basic.xhtml
css3/flexbox/flex-align.html
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Comment 6 Tony Chang 2012-01-13 10:03:27 PST
(In reply to comment #5)
> (From update of attachment 122359 [details])
> Attachment 122359 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11227415
> 
> New failing tests:
> svg/css/getComputedStyle-basic.xhtml
> css3/flexbox/flex-align.html
> fast/css/getComputedStyle/computed-style.html
> fast/css/getComputedStyle/computed-style-without-renderer.html

css3/flexbox/flex-align.html failing is a bit suspicious.
Comment 7 Ojan Vafai 2012-01-13 12:43:45 PST
Committed r104972: <http://trac.webkit.org/changeset/104972>
Comment 8 Ojan Vafai 2012-01-13 14:00:43 PST
Reverted r104972 for reason:

Broke some tests

Committed r104985: <http://trac.webkit.org/changeset/104985>
Comment 9 Ojan Vafai 2012-01-13 16:11:56 PST
Antti, Hyatt: Would one of you mind taking a quick look at the CSSComputedStyleDeclaration changes in this patch and letting me know if I'm doing something obviously wrong? It's just two lines (the CSSPropertyWebkitFlexItemAlign changes).

Hmm. The following tests crashed on chromium win and linux (they're not run on chromium-mac):
http/tests/inspector/extensions-ignore-cache.html
inspector/styles/styles-new-API.html
inspector/styles/parse-utf8-bom.html
inspector/styles/media-queries.html

It's strange they didn't crash on the chromium EWS.

Here's the stack:
	WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue() [0xb8e289]
	WebCore::CSSComputedStyleDeclaration::getPropertyCSSValue() [0xb9807e]
	WebCore::CSSComputedStyleDeclaration::getPropertyValue() [0xb88402]
	WebCore::CSSStyleDeclaration::getPropertyValue() [0xbd011f]
	WebCore::InspectorStyle::populateAllProperties() [0xeb01aa]
	WebCore::InspectorStyle::buildArrayForComputedStyle() [0xeb09a3]
	WebCore::InspectorCSSAgent::getComputedStyleForNode() [0xea60fa]
	WebCore::InspectorBackendDispatcher::CSS_getComputedStyleForNode() [0x11f6c91]
	WebCore::InspectorBackendDispatcher::dispatch() [0x1200e9a]
	WebKit::WebDevToolsAgentImpl::dispatchOnInspectorBackend() [0x4a0c8f]
Comment 10 Ojan Vafai 2012-01-13 18:58:41 PST
Created attachment 122529 [details]
Patch
Comment 11 Ojan Vafai 2012-01-13 19:13:51 PST
Committed r105015: <http://trac.webkit.org/changeset/105015>

Took out the special CSSComputedStyleDeclaration behavior since it's almost certainly what caused the crash. Will fix it in bug 76326.