Bug 119033

Summary:

Background doesn't fully repaint on this page

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

Layout and Rendering

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, commit-queue, esprehn+autocc, glenn, kondapallykalyan, simon.fraser, zalan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://people.mozilla.org/~jdaggett/tests/social-waterfall.html

Attachments:

Description	Flags
test reduction	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Simon Fraser (smfr)

Reported 2013-07-23 21:55:08 PDT

Hit 'b' to toggle the background color after scrolling down a bit. Parts of the background remain unchanged.

Attachments
test reduction (2.04 KB, text/html) 2013-07-24 09:39 PDT, alan	no flags	Details
Patch (11.24 KB, patch) 2013-07-26 08:34 PDT, alan	no flags	Details Formatted Diff Diff
Patch (14.88 KB, patch) 2013-07-31 06:05 PDT, alan	no flags	Details Formatted Diff Diff
Patch (15.37 KB, patch) 2013-08-03 12:34 PDT, alan	no flags	Details Formatted Diff Diff
Patch for landing (15.37 KB, patch) 2013-08-04 13:41 PDT, alan	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

alan

Comment 1 2013-07-24 09:39:29 PDT

Created attachment 207401 [details] test reduction

alan

Comment 2 2013-07-24 09:54:24 PDT

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)

Simon Fraser (smfr)

Comment 3 2013-07-24 10:24:27 PDT

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.

alan

Comment 4 2013-07-24 10:35:41 PDT

(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.

alan

Comment 5 2013-07-24 12:04:29 PDT

(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)

alan

Comment 6 2013-07-25 09:19:36 PDT

(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.

alan

Comment 7 2013-07-26 08:34:56 PDT

Created attachment 207529 [details] Patch

Simon Fraser (smfr)

Comment 8 2013-07-26 08:45:46 PDT

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.

Simon Fraser (smfr)

Comment 9 2013-07-26 08:49:22 PDT

(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".

alan

Comment 10 2013-07-31 06:05:22 PDT

Created attachment 207846 [details] Patch

alan

Comment 11 2013-07-31 06:06:41 PDT

I am more than happy to change 'repaintFullContent' to something more meaningful.

Simon Fraser (smfr)

Comment 12 2013-08-02 14:50:54 PDT

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?

alan

Comment 13 2013-08-03 12:34:02 PDT

Created attachment 208069 [details] Patch

alan

Comment 14 2013-08-03 12:38:52 PDT

(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).

WebKit Commit Bot

Comment 15 2013-08-04 11:02:44 PDT

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

alan

Comment 16 2013-08-04 13:41:05 PDT

Created attachment 208100 [details] Patch for landing

WebKit Commit Bot

Comment 17 2013-08-04 14:27:50 PDT

Comment on attachment 208100 [details] Patch for landing Clearing flags on attachment: 208100 Committed r153701: <http://trac.webkit.org/changeset/153701>

WebKit Commit Bot

Comment 18 2013-08-04 14:27:53 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.