WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
119033
Background doesn't fully repaint on this page
https://bugs.webkit.org/show_bug.cgi?id=119033
Summary
Background doesn't fully repaint on this page
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.
Top of Page
Format For Printing
XML
Clone This Bug