Bug 87068

Summary: Replaced items in a flexbox should be coerced to display:block
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: Layout and RenderingAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, jackalmage, macpherson, menard, tabatkins, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88963    
Bug Blocks: 62048    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Ojan Vafai
Reported 2012-05-21 18:45:45 PDT
As per the new spec. It actually hardcodes tagnames that should be coerced to display:block when they are in a flexbox. It also says "An atomic inline-level child of a flex container" should be a flex-item. I'm not really sure what that refers to since it's a different item than the replaced items and the spec refers to button as an atomic inline-level child. Tab, can you clarify?
Attachments
Patch (11.17 KB, patch)
2012-06-12 13:26 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (11.23 KB, patch)
2012-06-12 13:27 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (11.46 KB, patch)
2012-06-12 13:33 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch for landing (11.77 KB, patch)
2012-06-12 13:58 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Tab Atkins Jr.
Comment 1 2012-05-22 13:20:46 PDT
"atomic inline" refers to things that are inline-level, but lay out as a single thing: inline-block, inline-table, inline replaced, etc. All the hardcoded tag names should have already been flexbox items, since they all fall under the "replaced element" umbrella - the new entry with hardcoded names is just meant to cover the odd case where an element can be replaced or not depending on whether a link resolves.
Tony Chang
Comment 2 2012-06-12 13:26:38 PDT
Created attachment 147141 [details] Patch
Tony Chang
Comment 3 2012-06-12 13:27:19 PDT
Created attachment 147142 [details] Patch
Tony Chang
Comment 4 2012-06-12 13:27:43 PDT
Comment on attachment 147142 [details] Patch Added a link to the spec in the ChangeLog.
Gyuyoung Kim
Comment 5 2012-06-12 13:31:49 PDT
Comment on attachment 147142 [details] Patch Attachment 147142 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12944649
Tony Chang
Comment 6 2012-06-12 13:33:58 PDT
Created attachment 147143 [details] Patch
Ojan Vafai
Comment 7 2012-06-12 13:45:47 PDT
Comment on attachment 147143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147143&action=review > Source/WebCore/css/StyleResolver.cpp:1923 > +static bool shouldBecomeBlockWhenParentIsFlexbox(const Element* element) This should probably include a link to the spec. > LayoutTests/css3/flexbox/resources/flexbox.js:69 > + var expectedDisplay = node.getAttribute && node.getAttribute("data-display"); Nit: should the attribute name be data-expected-display?
Tony Chang
Comment 8 2012-06-12 13:58:50 PDT
Created attachment 147152 [details] Patch for landing
Tony Chang
Comment 9 2012-06-12 13:59:11 PDT
Comment on attachment 147143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147143&action=review >> Source/WebCore/css/StyleResolver.cpp:1923 >> +static bool shouldBecomeBlockWhenParentIsFlexbox(const Element* element) > > This should probably include a link to the spec. I didn't want to include a link to the working draft in code. Once it's finalized or CR stage, it would probably be OK. >> LayoutTests/css3/flexbox/resources/flexbox.js:69 >> + var expectedDisplay = node.getAttribute && node.getAttribute("data-display"); > > Nit: should the attribute name be data-expected-display? Renamed.
WebKit Review Bot
Comment 10 2012-06-12 15:39:18 PDT
Comment on attachment 147152 [details] Patch for landing Clearing flags on attachment: 147152 Committed r120132: <http://trac.webkit.org/changeset/120132>
WebKit Review Bot
Comment 11 2012-06-12 15:39:23 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.