Bug 110933

Summary: REGRESSION (r143643): Buttons containing floats render differently
Product: WebKit Reporter: Christian Biesinger <cbiesinger>
Component: Layout and RenderingAssignee: Christian Biesinger <cbiesinger>
Status: RESOLVED FIXED    
Severity: Normal CC: c.petersen87, eric, esprehn+autocc, esprehn, ojan.autocc, ojan, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109994    
Bug Blocks:    
Attachments:
Description Flags
testcase
none
Patch
none
Patch
none
Testcase for floats inside a flexitem
none
Patch none

Description Christian Biesinger 2013-02-26 18:31:45 PST 
See attached testcase. It looks like converting buttons to new-flexbox made it behave differently with respect to floats.
Comment 1 Christian Biesinger 2013-02-26 18:32:38 PST 
Created attachment 190409 [details]
testcase

Firefox renders like old-webkit
Comment 2 Simon Fraser (smfr) 2013-02-27 11:22:50 PST 
What revision did this regress in? (i.e. qualify the "now" in the title).
Comment 3 Christian Biesinger 2013-02-27 11:23:56 PST 
It was bug 109994 aka r143643, on 2013-02-21 12:58:04
Comment 4 Christian Biesinger 2013-02-27 13:53:18 PST 
Note, this also affects twitter.com (the "New Tweet" button in the topright corner)
Comment 5 Christian Biesinger 2013-02-27 14:08:35 PST 
Created attachment 190601 [details]
Patch
Comment 6 Christian Biesinger 2013-02-27 14:21:12 PST 
This patch does fix the problem, but I'd appreciate opinions on whether this is the right fix. I could instead add isRenderButton() to the if.

Come to think of it, I should also add a test to css3/flexbox that doesn't use a button. New patch coming...
Comment 7 Christian Biesinger 2013-02-27 14:24:37 PST 
Created attachment 190602 [details]
Patch
Comment 8 Zan Dobersek 2013-03-02 00:25:15 PST 
*** Bug 111241 has been marked as a duplicate of this bug. ***
Comment 9 Christian Biesinger 2013-03-04 12:11:46 PST 
Created attachment 191286 [details]
Testcase for floats inside a flexitem
Comment 10 Christian Biesinger 2013-03-04 13:10:51 PST 
OK, including new-flexbox in this if is correct because flex boxes establish a formatting context which means they should expand to include floats.
Comment 11 Ojan Vafai 2013-03-04 13:39:49 PST 
Comment on attachment 190602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190602&action=review

> Source/WebCore/ChangeLog:13
> +        Also include new flexbox as a renderer that needs to enclose
> +        overhanging floats.

Might be worth an extra sentence here to mention that the spec says that flexboxes establish a block formatting context.
Comment 12 Christian Biesinger 2013-03-04 14:05:31 PST 
Created attachment 191306 [details]
Patch
Comment 13 Christian Biesinger 2013-03-04 14:11:19 PST 
*** Bug 111263 has been marked as a duplicate of this bug. ***
Comment 14 WebKit Review Bot 2013-03-04 18:48:46 PST 
Comment on attachment 191306 [details]
Patch

Clearing flags on attachment: 191306

Committed r144706: <http://trac.webkit.org/changeset/144706>
Comment 15 WebKit Review Bot 2013-03-04 18:48:50 PST 
All reviewed patches have been landed.  Closing bug.