Bug 49027
| Summary: | video controls RenderReplaced::layout() calls setNeedsLayout(false) while its children still need layout | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | James Robinson <jamesr> |
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | dglazkov, eric.carlson, hyatt, koivisto, mitz, simon.fraser |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | OS X 10.5 | ||
| Bug Depends on: | 53020 | ||
| Bug Blocks: | 49019 | ||
James Robinson
The ASSERT()s in https://bugs.webkit.org/show_bug.cgi?id=49019 trigger on the following HTML:
<!DOCTYPE html>
<video controls></video>
as well as most of the media/ tests. The render tree looks something like this:
RenderView 0x3dc3cd8 #document 0x3dfed20
RenderBlock 0x3e3c968 HTML 0x3e3bf40
RenderBody 0x3e37968 BODY 0x3e3cd90
RenderVideo 0x3e4aa88 VIDEO 0x3e37a30
* RenderBlock (relative positioned) 0x3e4ae48 DIV 0x3e38950
RenderFlexibleBox (positioned) 0x3e48308 DIV 0x3e4b050
RenderButton 0x3e48808 INPUT 0x3e4c610
RenderFlexibleBox (positioned) 0x3e48ea8 DIV 0x3e4cb90
RenderBlock (positioned) 0x3e49438 DIV 0x3e4cc30
RenderText 0x3db0ed8 #text 0x3db0e50 "00:00"
RenderSlider 0x3e499a8 INPUT 0x3e4ccd0
RenderBlock 0x3db0a08 DIV 0x3db01f0
RenderButton 0x3dafb88 INPUT 0x3e476d0
The RenderBlock root of the controls is marked as m_needsLayout=true, m_normalChildNeedsLayout=true, m_posChildNeedsLayout=true at the end of RenderReplaced::layout() on its parent RenderVideo.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Simon Fraser (smfr)
Oooh, maybe RenderTreeAsText should optionally mark renderers that still need layout. If we did that in DRT, it might catch lots of bugs.
James Robinson
(In reply to comment #1)
> Oooh, maybe RenderTreeAsText should optionally mark renderers that still need layout. If we did that in DRT, it might catch lots of bugs.
I like it. I also think that DRT should ASSERT that if any renderers are marked as needing layout at the end of the test that the appropriate layout timer is set and the layout root (if set) is an ancestor of all dirty objects.
James Robinson
RenderReplaced::layout() isn't written to handle children and in fact RenderReplaced overrides canHaveChildren() to return false, however RenderMedia goes ahead and inserts a shadow RenderObject tree underneath itself.
Antti Koivisto
(In reply to comment #3)
> RenderReplaced::layout() isn't written to handle children and in fact RenderReplaced overrides canHaveChildren() to return false, however RenderMedia goes ahead and inserts a shadow RenderObject tree underneath itself.
RenderMedia also overrides layout() which is supposed to handle this.
Dimitri Glazkov (Google)
I think I can fix this while I am in here.
Dimitri Glazkov (Google)
(In reply to comment #5)
> I think I can fix this while I am in here.
One fix write RenderMedia::layout as full replacement for RenderReplaced::layout.
Dimitri Glazkov (Google)
(In reply to comment #5)
> I think I can fix this while I am in here.
I am no longer "in here" :(