Hit 'b' to toggle the background color after scrolling down a bit. Parts of the background remain unchanged.
Created attachment 207401 [details] test reduction
This is an issue with missing repaint rects. When the style change happens, we send repaints on the content -excluding the margins and on the entire RenderView() (see below). Making the entire canvas dirty is meant to fix the background repaint issue on the body/html. However, when the body's margin makes its parent (html) larger than the viewport, repainting only the canvas is not sufficient. Something like this could address the problem. -> diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp index d93dbf0..d8f17d5 100644 --- a/Source/WebCore/rendering/RenderBox.cpp +++ b/Source/WebCore/rendering/RenderBox.cpp @@ -205,10 +205,12 @@ void RenderBox::styleWillChange(StyleDifference diff, const RenderStyle* newStyl RenderStyle* oldStyle = style(); if (oldStyle) { - // The background of the root element or the body element could propagate up to - // the canvas. Just dirty the entire canvas when our style changes substantially. + // The background of the root or the body element could propagate up to + // the canvas. Just dirty the parents including the entire canvas when our style changes substantially. if (diff >= StyleDifferenceRepaint && node() && (node()->hasTagName(htmlTag) || node()->hasTagName(bodyTag))) { + if (parent() && parent() != view()) + parent()->repaint(); view()->repaint(); #if USE(ACCELERATED_COMPOSITING)
Does this happen without tiled drawing enabled. I think view()->repaint(); is broken with tiled drawing; it only repaints the visible part of the view, but we need to dirty all tiles.
(In reply to comment #3) > Does this happen without tiled drawing enabled. > > I think view()->repaint(); is broken with tiled drawing; it only repaints the visible part of the view, but we need to dirty all tiles. I'll check it out...However, the RenderView is a lot smaller than its RenderBlock child RenderView (0, 0, 1260, 717) RenderBlock (0, 0, 1260, 1200) (notice the impact of the body's margin on the width) RenderBody (300, 0, 660, 900) so it might be that the tiling code just makes this problem visible.
(In reply to comment #3) > Does this happen without tiled drawing enabled. > > I think view()->repaint(); is broken with tiled drawing; it only repaints the visible part of the view, but we need to dirty all tiles. It's broken the same way in the non-tiled version too as far as the repainting is the concerned after the style change. However, since non-tiled scrolling involves repaints as you scroll (no pre-painted buffers), the out-of-the-viewport part gets repainted and that fixes the background color. If you scroll to the bottom first and reload the page, you see the broken background color (and gets cleared up right after the document is scrolled)
(In reply to comment #3) > Does this happen without tiled drawing enabled. > > I think view()->repaint(); is broken with tiled drawing; it only repaints the visible part of the view, but we need to dirty all tiles. (recap: 1, Both body and html background get propagated up to the viewport. Say, if body has background-color attribute set, while <html> doesn't, the color is applied not only on the body but on both the html and the viewport. 3. With the current 'view()->repaint()' setup, it works just fine as long as the document is smaller than the viewport. When the document extends the viewport, the out-of-the-viewport area of the existing tiles end up with wrong(old) background color(no repaint). 4. As the document position is changed and new tiles are getting created, they are painted with the correct color. That's why the linked page gets the wrong background color only on a 'small' portion of the document. 5. With tiles off, it works fine as we always repaint when the document position is changed. (though with the exception of the renderview's rect is applied mistakenly when the doc pos != 0) ) We could either 1. simply dirty all the tiles or 2, be a bit more verbose by doing something like this: repaint(); ->(repaint this) if (isBody()) parent()->repaint(); ->(repaint <html>, in case html does not have background set) view()->repaint(); ->(repaint the viewport, in case the document is smaller than the viewport) but that's pretty much equivalent of making all the tiles dirty and it just creates noise. What I am not sure about is whether view()->repaint() should be extended to cover all the tiles too (so it would always dirty them all) or come up with a dedicated function for this specific usecase.
Created attachment 207529 [details] Patch
Comment on attachment 207529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207529&action=review > Source/WebCore/rendering/RenderBox.cpp:211 > + view()->layer()->repaintIncludingDescendants(); I think this is too heavyweight; it will repaint all child compositing layers, which is not necessary.
(In reply to comment #6) > (In reply to comment #3) > > Does this happen without tiled drawing enabled. > > > > I think view()->repaint(); is broken with tiled drawing; it only repaints the visible part of the view, but we need to dirty all tiles. > > (recap: > 1, Both body and html background get propagated up to the viewport. Say, if body has background-color attribute set, while <html> doesn't, the color is applied not only on the body but on both the html and the viewport. > 3. With the current 'view()->repaint()' setup, it works just fine as long as the document is smaller than the viewport. When the document extends the viewport, the out-of-the-viewport area of the existing tiles end up with wrong(old) background color(no repaint). > 4. As the document position is changed and new tiles are getting created, they are painted with the correct color. That's why the linked page gets the wrong background color only on a 'small' portion of the document. > 5. With tiles off, it works fine as we always repaint when the document position is changed. (though with the exception of the renderview's rect is applied mistakenly when the doc pos != 0) ) > > We could either > 1. simply dirty all the tiles > or > 2, be a bit more verbose by doing something like this: > repaint(); ->(repaint this) > if (isBody()) > parent()->repaint(); ->(repaint <html>, in case html does not have background set) > view()->repaint(); ->(repaint the viewport, in case the document is smaller than the viewport) > > but that's pretty much equivalent of making all the tiles dirty and it just creates noise. > What I am not sure about is whether view()->repaint() should be extended to cover all the tiles too (so it would always dirty them all) or come up with a dedicated function for this specific usecase. I think we should invent a new function to repaint all the tiles, possibly on FrameView, and transition callers of view()->repaint() onto it. This would be similar to the "paintsEntireContents" code path. When tiled/layer-backed, it would know to invalidate the entire tiled area. We'll have to be careful to distinguish between repaints which only need to affect the root background, and those which are "repaint everything including all compositing layers".
Created attachment 207846 [details] Patch
I am more than happy to change 'repaintFullContent' to something more meaningful.
Comment on attachment 207846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207846&action=review > Source/WebCore/page/FrameView.cpp:2023 > +void FrameView::repaintFullContent() > +{ > + RenderView* renderView = this->renderView(); > + if (!renderView) > + return; > + > +#if USE(ACCELERATED_COMPOSITING) > + RenderLayer* layer = renderView->layer(); > + if (layer->isComposited()) { > + layer->setBackingNeedsRepaint(); > + return; > + } > +#endif > + renderView->repaint(); > +} Since this just gets the RenderView and does stuff to it, I think it should be on RenderView, which would simplify the call sites. So I think this should just be moved to RenderView and called something like repaintRootContents() or something. > Source/WebCore/rendering/RenderBox.cpp:211 > + if (diff >= StyleDifferenceRepaint && (isRoot() || isBody())) { I'm not sure that isRoot() and node()->hasTagName(htmlTag) are equivalent in all cases. E.g. what about <html> inside SVG foreign object?
Created attachment 208069 [details] Patch
(In reply to comment #12) > (From update of attachment 207846 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207846&action=review > > > Source/WebCore/page/FrameView.cpp:2023 > > +void FrameView::repaintFullContent() > > +{ > > + RenderView* renderView = this->renderView(); > > + if (!renderView) > > + return; > > + > > +#if USE(ACCELERATED_COMPOSITING) > > + RenderLayer* layer = renderView->layer(); > > + if (layer->isComposited()) { > > + layer->setBackingNeedsRepaint(); > > + return; > > + } > > +#endif > > + renderView->repaint(); > > +} > > Since this just gets the RenderView and does stuff to it, I think it should be on RenderView, which would simplify the call sites. So I think this should just be moved to RenderView and called something like repaintRootContents() or something. > > > Source/WebCore/rendering/RenderBox.cpp:211 > > + if (diff >= StyleDifferenceRepaint && (isRoot() || isBody())) { > > I'm not sure that isRoot() and node()->hasTagName(htmlTag) are equivalent in all cases. E.g. what about <html> inside SVG foreign object? isRoot() is a bit misleading name, It actually calls document()->documentElement() == m_node. Even when <html> is embedded to a svg foreign object, we are still inside the <html> document so document()->documentElement() returns the <html> as opposed to what root might suggest (the <svg> document).
Comment on attachment 208069 [details] Patch Rejecting attachment 208069 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 208069, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/1342867
Created attachment 208100 [details] Patch for landing
Comment on attachment 208100 [details] Patch for landing Clearing flags on attachment: 208100 Committed r153701: <http://trac.webkit.org/changeset/153701>
All reviewed patches have been landed. Closing bug.