Flash of broken page when exiting full screen at jerryseinfeld.com
Created attachment 95724 [details] Patch
<rdar://problem/9522985>
Created attachment 95732 [details] Patch Added radar URL; moved some code between the two patches to allow them to apply cleanly.
Comment on attachment 95724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95724&action=review > Source/WebCore/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=61897 There should be a brief summary of the fix here. > Source/WebCore/dom/Document.cpp:4892 > + bool shouldCreatePlaceholder = m_fullScreenElement->renderer()->isBox(); Why does being a box require a placeholder? Needs an explanatory comment. > Source/WebCore/dom/Document.cpp:4894 > + IntRect frameRect = renderer->isBox() ? toRenderBox(renderer)->frameRect() : IntRect(); Why test renderer->isBox() rather than shouldCreatePlaceholder? Seems like an if () clause here would be easier to read. > Source/WebCore/dom/Document.cpp:4976 > + renderer->createPlaceholder(RenderStyle::clone(placeholder->style()), placeholder->frameRect()); So the style has been cloned twice? > Source/WebCore/rendering/RenderFullScreen.cpp:102 > + if (!m_placeholder) { > + m_placeholder = new (document()->renderArena()) RenderBlock(document()); > + m_placeholder->setStyle(style); > + m_placeholder->setIsAnonymous(false); > + if (parent()) { > + parent()->addChild(m_placeholder, this); > + remove(); > + m_placeholder->addChild(this); > + } > + } else > + m_placeholder->setStyle(style); > + m_placeholder->setFrameRect(frameRect); It's weird and a bit scarey that you're manipulating the render tree outside of layout, and expecting that the frameRect that you set will not be changed by layout.
Comment on attachment 95724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95724&action=review >> Source/WebCore/ChangeLog:6 >> + https://bugs.webkit.org/show_bug.cgi?id=61897 > > There should be a brief summary of the fix here. Sure. >> Source/WebCore/dom/Document.cpp:4892 >> + bool shouldCreatePlaceholder = m_fullScreenElement->renderer()->isBox(); > > Why does being a box require a placeholder? Needs an explanatory comment. Only a RenderBox and its subclasses has a frameRect(). I'll add a comment. >> Source/WebCore/dom/Document.cpp:4894 >> + IntRect frameRect = renderer->isBox() ? toRenderBox(renderer)->frameRect() : IntRect(); > > Why test renderer->isBox() rather than shouldCreatePlaceholder? Seems like an if () clause here would be easier to read. This could be shouldCreatePlaceholder. I'll change it. >> Source/WebCore/dom/Document.cpp:4976 >> + renderer->createPlaceholder(RenderStyle::clone(placeholder->style()), placeholder->frameRect()); > > So the style has been cloned twice? Or many more times, depending on how many times this function is called while within full-screen mode. I guess that, since RenderStyles are RefCounted, we don't have to clone them, but I didn't like the idea that two renderers might point to the same style, no matter how briefly. >> Source/WebCore/rendering/RenderFullScreen.cpp:102 >> + m_placeholder->setFrameRect(frameRect); > > It's weird and a bit scarey that you're manipulating the render tree outside of layout, and expecting that the frameRect that you set will not be changed by layout. It very well might be changed by layout. If the style says, e.g., "width: 50%" I would absolutely expect that the frameRect will change during layout. But it should change in a similar way to the full-screen element. And actually this function is called from within layout, in the case of wrapWithRenderFullScreen() above. I could modify webkitWillEnterFullScreenForElement() so that the placeholder is added during layout instead of directly after, but that would require adding two new ivars to Document.
I lived on this patch for a little while and did lot of video watching. Seems good!
Comment on attachment 95732 [details] Patch Unflagging - Jer will be revising to address comments.
Created attachment 95928 [details] Patch Addressed Simon's comments by ensuring the creation of the placeholder renderer occurs entirely within layout.
Comment on attachment 95928 [details] Patch Whoops, somehow webkit-patch picked up two patches. Obseleting.
Created attachment 95930 [details] Patch Addressed Simon's comments by ensuring the creation of the placeholder renderer occurs entirely within layout.
Comment on attachment 95930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95930&action=review > Source/WebCore/dom/Document.cpp:4788 > + // Only create a placeholder block for a RenderBox, as only a box will have a frameRect. I'd like this comment to say what you say in the changelog about the fullscreen element being taken out of normal flow. Otherwise the role of the placeholder is unclear. > Source/WebCore/rendering/RenderFullScreen.cpp:107 > + m_placeholder->setFrameRect(frameRect); Is this necessary? Won't layout happen and set the frame?
Comment on attachment 95930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95930&action=review >> Source/WebCore/dom/Document.cpp:4788 >> + // Only create a placeholder block for a RenderBox, as only a box will have a frameRect. > > I'd like this comment to say what you say in the changelog about the fullscreen element being taken out of normal flow. Otherwise the role of the placeholder is unclear. Sure thing. >> Source/WebCore/rendering/RenderFullScreen.cpp:107 >> + m_placeholder->setFrameRect(frameRect); > > Is this necessary? Won't layout happen and set the frame? Not for the case of an element whose width and height are dependent on their contents. Since the placeholder doesn't have any contents in the normal flow, it's width and height will remain zero. (In the case that the style's width or height are auto.)
(In reply to comment #12) > (From update of attachment 95930 [details]) > >> Source/WebCore/rendering/RenderFullScreen.cpp:107 > >> + m_placeholder->setFrameRect(frameRect); > > > > Is this necessary? Won't layout happen and set the frame? > > Not for the case of an element whose width and height are dependent on their contents. Since the placeholder doesn't have any contents in the normal flow, it's width and height will remain zero. (In the case that the style's width or height are auto.) Oh, I see what you're saying; we reset the style's width and height if they're auto, so these values should get replaced during layout. Let me try that out.
(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 95930 [details] [details]) > > >> Source/WebCore/rendering/RenderFullScreen.cpp:107 > > >> + m_placeholder->setFrameRect(frameRect); > > > > > > Is this necessary? Won't layout happen and set the frame? > > > > Not for the case of an element whose width and height are dependent on their contents. Since the placeholder doesn't have any contents in the normal flow, it's width and height will remain zero. (In the case that the style's width or height are auto.) > > Oh, I see what you're saying; we reset the style's width and height if they're auto, so these values should get replaced during layout. Let me try that out. Yes, it looks like this is unnecessary. I'll remove the frameRect stuff from the patch.
Committed r88034: <http://trac.webkit.org/changeset/88034>