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 treegraph
 
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

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.