Bug 54573 - Root element should establish a new block formatting context
Summary: Root element should establish a new block formatting context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://software.hixie.ch/utilities/js...
Keywords:
Depends on:
Blocks: 59624
  Show dependency treegraph
 
Reported: 2011-02-16 11:10 PST by Simon Fraser (smfr)
Modified: 2011-04-27 12:12 PDT (History)
10 users (show)

See Also:


Attachments
Patch (19.25 KB, patch)
2011-02-24 15:26 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (59.04 KB, patch)
2011-04-21 14:38 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-02-16 11:10:01 PST
The root element should establish a new block formatting context. See http://wiki.csswg.org/spec/css2.1#issue-209
Comment 1 Dave Hyatt 2011-02-16 11:11:13 PST
I think it effectively already does in every case that matters except for expanding to enclose overhanging floats.  We just need to patch that case.
Comment 2 Levi Weintraub 2011-02-24 15:26:25 PST
Created attachment 83730 [details]
Patch
Comment 3 Levi Weintraub 2011-02-24 15:28:24 PST
Comment on attachment 83730 [details]
Patch

Removing the r?. If this is accurate (it renders correctly), there are a lot of associated expected result changes I'll have to include as there are many cases where the root's size doesn't include overhanging floats.
Comment 4 Levi Weintraub 2011-04-21 14:38:48 PDT
Created attachment 90605 [details]
Patch
Comment 5 Levi Weintraub 2011-04-22 14:01:43 PDT
(In reply to comment #4)
> Created an attachment (id=90605) [details]
> Patch

This is an easy fix. Can I get a review before the layout test updates atrophy?
Comment 6 Eric Seidel (no email) 2011-04-26 17:13:07 PDT
Comment on attachment 90605 [details]
Patch

OK.
Comment 7 WebKit Commit Bot 2011-04-26 21:33:28 PDT
Comment on attachment 90605 [details]
Patch

Clearing flags on attachment: 90605

Committed r85011: <http://trac.webkit.org/changeset/85011>
Comment 8 WebKit Commit Bot 2011-04-26 21:33:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2011-04-27 01:01:54 PDT
http://trac.webkit.org/changeset/85029 might have broken Chromium Win Release
Comment 10 Jessie Berlin 2011-04-27 07:12:54 PDT
Note: you only updated the Mac results, ignoring other platform results such as Windows and causing ~10 failures:

http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/12162
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r85011%20(12163)/results.html

I am debating rolling out this patch as a result, since it broke so many tests.
Comment 11 Jessie Berlin 2011-04-27 08:13:19 PDT
I updated the Windows results in http://trac.webkit.org/changeset/85052 instead of rolling out the patch, but it would be better in the future to stick around after you commit if you think there might be any rebaselining to be done on other platforms.
Comment 12 Levi Weintraub 2011-04-27 08:40:29 PDT
(In reply to comment #11)
> I updated the Windows results in http://trac.webkit.org/changeset/85052 instead of rolling out the patch, but it would be better in the future to stick around after you commit if you think there might be any rebaselining to be done on other platforms.

As I stated on https://bugs.webkit.org/show_bug.cgi?id=54573 this was landed in my absence via the commit-queue. I hadn't listed it as cq? so I expected to be the one landing, so I could then be around to prevent this backlash.

Thanks for updating the expected results. I'd have been there to help if I'd known :)
Comment 13 Levi Weintraub 2011-04-27 08:40:50 PDT
Erm, in https://bugs.webkit.org/show_bug.cgi?id=59581 rather.
Comment 14 Eric Seidel (no email) 2011-04-27 08:51:53 PDT
We're working on teaching the EWSes to run tests (currently one cr-linux-ews does!) which may help prevent this sort of trouble in the future.

Committers are ultimately responsible for their patches of course.  But the commit-queue tries to make things as safe as it knows how.  Right now, that means it should never break mac. :)

Thanks for the fix Jessie.