Fix invalidation during painting of scrollbars

Fix invalidation during painting of scrollbars

Previously LayerScrollbarArea::positionOverflowControls() is called
from ScrollableAreaPainter::paintOverflowControls() just to apply
the paintOffset. This is incorrect because painting should not affect
layout and paintOffset should be applied to painting only.

- Remove the offsetFromRoot parameter of positionOverflowControls();
- Apply paintOffset to GraphicsContext only;
- Fix initial positionOverflowControls() when overflow:scroll but no
  overflow;
- Fix under-invalidation when scrollbar position/size changes.

BUG=457415, 456028
TEST=existing tests

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191027

[Xianzhu, 2015-02-24 21:41:43]

wangxianzhu@chromium.org changed reviewers:
+ jchaffraix@chromium.org, pdr@chromium.org

[chrishtr, 2015-02-24 22:38:41]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2015-02-24 22:38:42]

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:111: invalidate();
Is this because widgets don't obey the standard way of doing paint invalidation?

[Xianzhu, 2015-02-24 23:01:57]

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:111: invalidate();
On 2015/02/24 22:38:42, chrishtr wrote:
> Is this because widgets don't obey the standard way of doing paint
invalidation?

Yes. They are not LayoutObjects, so the normal
previousPaintInvalidationRect/needsFullPaintInvalidation mechanisms don't apply
for them.

[chrishtr, 2015-02-24 23:13:25]

On 2015/02/24 at 23:01:57, wangxianzhu wrote:
>
https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
> File Source/platform/scroll/Scrollbar.cpp (right):
> 
>
https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
> Source/platform/scroll/Scrollbar.cpp:111: invalidate();
> On 2015/02/24 22:38:42, chrishtr wrote:
> > Is this because widgets don't obey the standard way of doing paint
invalidation?
> 
> Yes. They are not LayoutObjects, so the normal
previousPaintInvalidationRect/needsFullPaintInvalidation mechanisms don't apply
for them.

Have you investigating fixing that? e.g. invalidate them when the LayoutObject
that owns the widget invalidates.

[Xianzhu, 2015-02-24 23:17:42]

On 2015/02/24 23:13:25, chrishtr wrote:
> On 2015/02/24 at 23:01:57, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
> > File Source/platform/scroll/Scrollbar.cpp (right):
> > 
> >
>
https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
> > Source/platform/scroll/Scrollbar.cpp:111: invalidate();
> > On 2015/02/24 22:38:42, chrishtr wrote:
> > > Is this because widgets don't obey the standard way of doing paint
> invalidation?
> > 
> > Yes. They are not LayoutObjects, so the normal
> previousPaintInvalidationRect/needsFullPaintInvalidation mechanisms don't
apply
> for them.
> 
> Have you investigating fixing that? e.g. invalidate them when the LayoutObject
> that owns the widget invalidates.

We are already doing this which still depends on proper invalidation status set
in ScrollableArea. If the added invalidate() is called during layout, it will
set scrollbar damage rect in ScrollableArea. Then during paint invalidation,
RenderBox (or FrameView) will see the damage and do proper real invalidation.
Without the added calls, RenderBox won't know if the scrollbar needs
invalidation.

[Julien - ping for review, 2015-02-25 01:43:36]

I have one question but the change looks good AFAICT.

I also think that we should do the invalidation through the LayoutScrollbars.
However they are pretty crazy and this change is fixing an important regression
so I don't think we should gate it on the orthogonal fixing of LayoutScrollbar's
invalidations.

https://codereview.chromium.org/949303002/diff/40001/Source/core/layout/Layer...
File Source/core/layout/LayerScrollableArea.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/core/layout/Layer...
Source/core/layout/LayerScrollableArea.cpp:710: positionOverflowControls();
If we have no overflow it seems wasteful to call positionOverflowControls. Why
do we need to do it?

https://codereview.chromium.org/949303002/diff/40001/Source/core/paint/Scroll...
File Source/core/paint/ScrollableAreaPainter.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/core/paint/Scroll...
Source/core/paint/ScrollableAreaPainter.cpp:127: TransformRecorder
translateRecorder(*context, m_renderLayerScrollableArea.displayItemClient(),
AffineTransform::translation(adjustedPaintOffset.x(), adjustedPaintOffset.y()));
Ideally we would want this scope to extend to the function. This could probably
be easily done if you stopped passing adjustedPaintOffset to paintScrollCorner
and paintResizer.

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:110: if (frameRect != this->frameRect()) {
We prefer early return whenever possible.

[Xianzhu, 2015-02-25 01:57:09]

https://codereview.chromium.org/949303002/diff/40001/Source/core/layout/Layer...
File Source/core/layout/LayerScrollableArea.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/core/layout/Layer...
Source/core/layout/LayerScrollableArea.cpp:710: positionOverflowControls();
On 2015/02/25 01:43:36, Julien Chaffraix - PST wrote:
> If we have no overflow it seems wasteful to call positionOverflowControls. Why
> do we need to do it?

Even if we have no overflow, we can still have scrollbars (e.g. overflow:
scroll). Discovered this when running layout tests. positionOverflowControls()
will be fast if there is no scrollbars.

https://codereview.chromium.org/949303002/diff/40001/Source/core/paint/Scroll...
File Source/core/paint/ScrollableAreaPainter.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/core/paint/Scroll...
Source/core/paint/ScrollableAreaPainter.cpp:127: TransformRecorder
translateRecorder(*context, m_renderLayerScrollableArea.displayItemClient(),
AffineTransform::translation(adjustedPaintOffset.x(), adjustedPaintOffset.y()));
On 2015/02/25 01:43:36, Julien Chaffraix - PST wrote:
> Ideally we would want this scope to extend to the function. This could
probably
> be easily done if you stopped passing adjustedPaintOffset to paintScrollCorner
> and paintResizer.

Agreed. Tried this but found that paintScrollCorner and paintResizer are also
called from elsewhere with explicit paintOffset, and ScrollbarPainter also needs
cleaning up about paintOffset. I would like to do this later.

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/949303002/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:110: if (frameRect != this->frameRect()) {
On 2015/02/25 01:43:36, Julien Chaffraix - PST wrote:
> We prefer early return whenever possible.

Done.

[chrishtr, 2015-02-25 18:02:01]

lgtm

[Xianzhu, 2015-02-25 22:57:06]

Based on trybot results, the CL caused 1-pixel size mismatches of some
scrollbars on Mac. Investigating. Perhaps related to rounding differences
between applying offset to scrollbar frameRect and applying offset with
transformation.

[Xianzhu, 2015-02-26 19:39:53]

Found that the 1-pixel mismatch is caused by that the size of
RenderBox::pixelSnappedBorderBox() may change after layout of itself.

The steps are:
1. The box layouts itself. Its frameRect may be at (0,0), with correctly
calculated size;
2. LayerScrollableArea::updateAfterLayout() is called, and scrollbars are
positioned;
3. The container (e.g. RenderBlockFlow) positions the box. If the new position
is not at whole pixel, the size of pixelSnappedBorderBox may change and *should*
affect scrollbar positioning.

I only think of calling LayerScrollableArea::updateAfterLayout() when box's
location changes. Julien, does this method look good? What would you suggest?

[Xianzhu, 2015-02-26 20:32:40]

I think eventually we should disallow any pixel snapping during layout
(especially any layout depending on pixel snapped result), but this seems a big
work to do.

The latest patch set contains a workaround just for scrollbars:
- adds LayoutBox::setLocationAndUpdateOverflowControlsIfNeeded() to call
LayerScrollableArea::updateAfterLayout() when the size of
pixelSnappedBorderBoxRect() changes;
- changes some caller of LayoutBox::setLocation() to use the new function.

I can't ensure if the change is complete, but the changed callers seem the most
common cases.

[chrishtr, 2015-02-26 20:35:45]

chrishtr@chromium.org changed reviewers:
+ leviw@chromium.org

[chrishtr, 2015-02-26 20:35:46]

+levi for feedback on pixel snapping issues.

[Julien - ping for review, 2015-02-26 21:48:53]

On 2015/02/26 at 19:39:53, wangxianzhu wrote:
> Found that the 1-pixel mismatch is caused by that the size of
RenderBox::pixelSnappedBorderBox() may change after layout of itself.
> 
> The steps are:
> 1. The box layouts itself. Its frameRect may be at (0,0), with correctly
calculated size;
> 2. LayerScrollableArea::updateAfterLayout() is called, and scrollbars are
positioned;
> 3. The container (e.g. RenderBlockFlow) positions the box. If the new position
is not at whole pixel, the size of pixelSnappedBorderBox may change and *should*
affect scrollbar positioning.
> 
> I only think of calling LayerScrollableArea::updateAfterLayout() when box's
location changes. Julien, does this method look good? What would you suggest?

Is that guaranteed to be called once per layout? I am concerned about calling
the function several times per layout as it's pretty involved.

[Xianzhu, 2015-02-26 21:56:24]

On 2015/02/26 21:48:53, Julien Chaffraix - PST wrote:
> On 2015/02/26 at 19:39:53, wangxianzhu wrote:
> > Found that the 1-pixel mismatch is caused by that the size of
> RenderBox::pixelSnappedBorderBox() may change after layout of itself.
> > 
> > The steps are:
> > 1. The box layouts itself. Its frameRect may be at (0,0), with correctly
> calculated size;
> > 2. LayerScrollableArea::updateAfterLayout() is called, and scrollbars are
> positioned;
> > 3. The container (e.g. RenderBlockFlow) positions the box. If the new
position
> is not at whole pixel, the size of pixelSnappedBorderBox may change and
*should*
> affect scrollbar positioning.
> > 
> > I only think of calling LayerScrollableArea::updateAfterLayout() when box's
> location changes. Julien, does this method look good? What would you suggest?
> 
> Is that guaranteed to be called once per layout? I am concerned about calling
> the function several times per layout as it's pretty involved.

In the latest patch set,
RenderBox::setLocationAndUpdateOverflowControlsIfNeeded() is called from two
places which change RenderBox's location out of RenderBox's own layout().

[Xianzhu, 2015-02-27 19:00:10]

Any other comments on the latest change to workaround pixel snapping?

[chrishtr, 2015-02-27 19:20:33]

lgtm

[Xianzhu, 2015-02-27 19:25:38]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-02-27 19:25:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/949303002/#ps100001
(title: "NeedsRebaseline")

[commit-bot: I haz the power, 2015-02-27 19:26:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/949303002/100001

[commit-bot: I haz the power, 2015-02-27 20:56:06]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191027