Bug 75782 - Implement flex-align
Summary: Implement flex-align
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: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency treegraph
 
Reported: 2012-01-07 12:55 PST by Ojan Vafai
Modified: 2012-01-24 11:28 PST (History)
6 users (show)

See Also:


Attachments
Patch (33.96 KB, patch)
2012-01-12 15:42 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (37.13 KB, patch)
2012-01-12 19:03 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (45.40 KB, patch)
2012-01-13 18:58 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.