Bug 92709

Summary:

-webkit-flex-flow does not work with inherit/initial values

Product:

WebKit

Reporter:

dstockwell

Component:

New Bugs

Assignee:

dstockwell

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, macpherson, menard, ojan, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

dstockwell

Reported 2012-07-30 22:19:44 PDT

-webkit-flex-flow does not work with inherit/initial values

Attachments
Patch (3.95 KB, patch) 2012-07-30 22:23 PDT, dstockwell	no flags	Details Formatted Diff Diff
Patch (7.35 KB, patch) 2012-07-31 17:56 PDT, dstockwell	no flags	Details Formatted Diff Diff
Patch (7.35 KB, patch) 2012-07-31 18:23 PDT, dstockwell	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

dstockwell

Comment 1 2012-07-30 22:23:36 PDT

Created attachment 155439 [details] Patch

Tony Chang

Comment 2 2012-07-31 10:05:00 PDT

Comment on attachment 155439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155439&action=review Thanks for the patch! Just some small nits. > Source/WebCore/ChangeLog:10 > + Test: fast/css/webkit-flex-flow-initial.html Can you put the test in css3/flexbox? > LayoutTests/fast/css/webkit-flex-flow-initial.html:1 > +<style> <!DOCTYPE html> and <html> tag go here. > LayoutTests/fast/css/webkit-flex-flow-initial.html:15 > +</style> > +<span id="test1"></span> Please include some text describing what this test is testing. > LayoutTests/fast/css/webkit-flex-flow-initial.html:21 > +result.innerText = value1 === value2 ? 'PASS' : 'FAIL ' + value1 + ' !== ' + value2; Can you add a check to see that either value1 or value2 are column? You may want to use the js-test-pre.js file to get some convenience functions for testing.

Ojan Vafai

Comment 3 2012-07-31 11:29:04 PDT

Comment on attachment 155439 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155439&action=review > Source/WebCore/css/StyleResolver.cpp:4012 > + case CSSPropertyWebkitFlexFlow: > + if (isInherit) { > + m_style->setFlexDirection(m_parentStyle->flexDirection()); > + m_style->setFlexWrap(m_parentStyle->flexWrap()); > + } else if (isInitial) { > + m_style->setFlexDirection(RenderStyle::initialFlexDirection()); > + m_style->setFlexWrap(RenderStyle::initialFlexWrap()); > + } > + return; How do other shorthands (e.g. border) deal with this? I don't think we should need to handle flexflow differently than other shorthands, right?

Tony Chang

Comment 4 2012-07-31 11:51:13 PDT

(In reply to comment #3) > (From update of attachment 155439 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155439&action=review > > > Source/WebCore/css/StyleResolver.cpp:4012 > > + case CSSPropertyWebkitFlexFlow: > > + if (isInherit) { > > + m_style->setFlexDirection(m_parentStyle->flexDirection()); > > + m_style->setFlexWrap(m_parentStyle->flexWrap()); > > + } else if (isInitial) { > > + m_style->setFlexDirection(RenderStyle::initialFlexDirection()); > > + m_style->setFlexWrap(RenderStyle::initialFlexWrap()); > > + } > > + return; > > How do other shorthands (e.g. border) deal with this? I don't think we should need to handle flexflow differently than other shorthands, right? They do it manually too: http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L3628 We could use the HANDLE_INHERIT_AND_INITIAL macros. I didn't for CSSPropertyWebkitFlex because it's a little less efficient when there are multiple properties, but it would probably be OK in this case.

Ojan Vafai

Comment 5 2012-07-31 12:12:07 PDT

Looks like most properties do this by using ApplyPropertyExpanding in StyleBuilder.cpp (e.g. http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleBuilder.cpp#L1836). I think we just need to add -webkit-flex-flow and -webkit-flex here appropriately.

dstockwell

Comment 6 2012-07-31 17:56:29 PDT

Created attachment 155692 [details] Patch

dstockwell

Comment 7 2012-07-31 17:58:46 PDT

Thanks for the comments, please take another look. I moved the handling for both flex and flex-flow to StyleBuilder.

Ojan Vafai

Comment 8 2012-07-31 18:20:37 PDT

Comment on attachment 155692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155692&action=review Looks great. Thanks! Just need to fix the typo in the test and then feel free to commit. > LayoutTests/css3/flexbox/flex-flow-initial.html:1 > +<DOCTYPE! html> The ! comes before the DOCTYPE.

dstockwell

Comment 9 2012-07-31 18:23:00 PDT

Created attachment 155697 [details] Patch

WebKit Review Bot

Comment 10 2012-07-31 21:48:39 PDT

Comment on attachment 155697 [details] Patch Clearing flags on attachment: 155697 Committed r124297: <http://trac.webkit.org/changeset/124297>

WebKit Review Bot

Comment 11 2012-07-31 21:48:43 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.