-webkit-flex-flow does not work with inherit/initial values
Created attachment 155439 [details] Patch
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.
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?
(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.
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.
Created attachment 155692 [details] Patch
Thanks for the comments, please take another look. I moved the handling for both flex and flex-flow to StyleBuilder.
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.
Created attachment 155697 [details] Patch
Comment on attachment 155697 [details] Patch Clearing flags on attachment: 155697 Committed r124297: <http://trac.webkit.org/changeset/124297>
All reviewed patches have been landed. Closing bug.