Summary: | ASSERTION FAILURE: !child->needsLayout() on paypal.com | ||
---|---|---|---|
Product: | WebKit | Reporter: | Matt Lilek <dev+webkit> |
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | bdakin, ddkilzer, mitz |
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression |
Version: | 420+ | ||
Hardware: | Mac | ||
OS: | OS X 10.4 | ||
URL: | http://paypal.com | ||
Attachments: |
Description
Matt Lilek
2007-01-22 21:32:41 PST
Created attachment 12617 [details]
Crashlog
If you don't have any items in recent activity, a detail page from My Account -> History triggers this too. Created attachment 12624 [details]
Test case (needs reduction)
Test case saved as "Web Page, Complete" from Firefox with transaction-specific info replaced with our friend, Lorem Ipsum. Still needs a reduction, but now you don't need a PayPal account to reproduce it.
Created attachment 12626 [details]
Reduction (will trip the ASSERT)
I talked with Maciej a bit about this bug. We are asserting because child is a RenderInline and RenderInline::layout() does nothing. (It is worth noting that this does not crash without the assertion, not does it cause any layout glitches as far as I can tell.) After talking with Maciej, it seems like we are incorrectly constructing the RenderTree. It seems like there should be an anonymous block wrapping the RenderInline so that it would get updated appropriately if there was anything to update. If this isn't the case, then it seems like the assertion is just wrong. Firefox has a UA stylesheet rule that sets such forms to display:none. The children of the form element are pushed out anyway, so it shouldn't matter. I tried to override createRenderer() to return 0 if the form element has one of the forbidden parents (table, tbody, tfoot, thead, tr) but it broke some of the fast/dom tests where the form is created dynamically inside the table. Maybe other rules apply there. How about having RenderInline::layout() just call setNeedsLayout(false)? The ASSERT in RenderContainer is new, but it is nice to have. 1) Which tests break? 2) Would the UA rule work for us too? (In reply to comment #9) > 1) Which tests break? Sorry, they were not fast/dom but rather fast/table/form-in-tbody-before-misnested-text-crash.xhtml fast/table/form-in-row-before-misnested-text-crash.xhtml fast/table/form-in-table-before-misnested-text-crash.xhtml These are all XHTML, and I remember that the Firefox UA rule uses a "-moz-is-html" pseudoclass, so maybe I should have done something similar in my createRenderer() override. > 2) Would the UA rule work for us too? Here's the Firefox rule from its html.css: tr > form:-moz-is-html, tbody > form:-moz-is-html, thead > form:-moz-is-html, tfoot > form:-moz-is-html, table > form:-moz-is-html { /* Important: don't show these forms in HTML */ display: none !important; } I don't know if there's a WebKit equivalent of -moz-is-html. Created attachment 12759 [details]
Option A: Don't create renderers for forms in tables, table sections and table rows in HTML
Wait, there's another patch!
Created attachment 12760 [details]
Option B: Alternatively, change the assertion to ignore inlines. They're harmless!
Also fixes the problem.
Comment on attachment 12759 [details]
Option A: Don't create renderers for forms in tables, table sections and table rows in HTML
I think this change will break any use of a counter CSS rule on one of these "form elements inside tables", because counters are based on the render tree hierarchy.
I'm not sure if that's an important issue, though.
Comment on attachment 12760 [details]
Option B: Alternatively, change the assertion to ignore inlines. They're harmless!
Hyatt told me this change is a good idea. So r=me.
Landed in r19270. |