Bug 110933 - REGRESSION (r143643): Buttons containing floats render differently
Summary: REGRESSION (r143643): Buttons containing floats render differently
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Christian Biesinger
URL:
Keywords:
: 111241 111263 (view as bug list)
Depends on: 109994
Blocks:
  Show dependency tree / graph
 
Reported: 2013-02-26 18:31 PST by Christian Biesinger
Modified: 2013-03-04 18:48 PST (History)
9 users (show)

See Also:

Attachments
testcase (736 bytes, text/html)
2013-02-26 18:32 PST, Christian Biesinger 		no flags Details
Patch (4.97 KB, patch)
2013-02-27 14:08 PST, Christian Biesinger 		no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2013-02-27 14:24 PST, Christian Biesinger 		no flags Details | Formatted Diff | Diff
Testcase for floats inside a flexitem (1.13 KB, text/html)
2013-03-04 12:11 PST, Christian Biesinger 		no flags Details
Patch (8.17 KB, patch)
2013-03-04 14:05 PST, Christian Biesinger 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.