Bug 110933

Summary:

REGRESSION (r143643): Buttons containing floats render differently

Product:

WebKit

Reporter:

Christian Biesinger <cbiesinger>

Component:

Layout and Rendering

Assignee:

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:

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

Christian Biesinger

Reported 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.

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 attachment proposed patch, testcase, etc.

Christian Biesinger

Comment 1 2013-02-26 18:32:38 PST

Created attachment 190409 [details] testcase Firefox renders like old-webkit

Simon Fraser (smfr)

Comment 2 2013-02-27 11:22:50 PST

What revision did this regress in? (i.e. qualify the "now" in the title).

Christian Biesinger

Comment 3 2013-02-27 11:23:56 PST

It was bug 109994 aka r143643, on 2013-02-21 12:58:04

Christian Biesinger

Comment 4 2013-02-27 13:53:18 PST

Note, this also affects twitter.com (the "New Tweet" button in the topright corner)

Christian Biesinger

Comment 5 2013-02-27 14:08:35 PST

Created attachment 190601 [details] Patch

Christian Biesinger

Comment 6 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...

Christian Biesinger

Comment 7 2013-02-27 14:24:37 PST

Created attachment 190602 [details] Patch

Zan Dobersek

Comment 8 2013-03-02 00:25:15 PST

*** Bug 111241 has been marked as a duplicate of this bug. ***

Christian Biesinger

Comment 9 2013-03-04 12:11:46 PST

Created attachment 191286 [details] Testcase for floats inside a flexitem

Christian Biesinger

Comment 10 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.

Ojan Vafai

Comment 11 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.

Christian Biesinger

Comment 12 2013-03-04 14:05:31 PST

Created attachment 191306 [details] Patch

Christian Biesinger

Comment 13 2013-03-04 14:11:19 PST

*** Bug 111263 has been marked as a duplicate of this bug. ***

WebKit Review Bot

Comment 14 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>

WebKit Review Bot

Comment 15 2013-03-04 18:48:50 PST

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.