RESOLVED FIXED 92709
-webkit-flex-flow does not work with inherit/initial values 
https://bugs.webkit.org/show_bug.cgi?id=92709
Summary -webkit-flex-flow does not work with inherit/initial values
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
DetailsFormatted DiffDiff
Patch (7.35 KB, patch)
2012-07-31 17:56 PDT, dstockwell 		no flags
DetailsFormatted DiffDiff
Patch (7.35 KB, patch)
2012-07-31 18:23 PDT, dstockwell 		no flags
DetailsFormatted DiffDiff
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.