Fix fixed-background offset issue

Fix fixed-background offset issue

Fixed-background under scrolled container has been broken when we
enabled ScrollRecorder. Because the drawing coordinates are in the
scrolled content, while fixed-background should be drawn as if it
is in the viewport, we should add the accumulated scroll offset to
the target rect of the fixed-background.

BUG=528226
TEST=paint/overflow/fixed-background-scroll.html
TEST=paint/overflow/fixed-background-scroll-window.html
TEST=paint/overflow/fixed-background-scroll-in-frame.html

Committed: https://crrev.com/efecb10be7c17ac2632cd86b890915d441e2da31
git-svn-id: svn://svn.chromium.org/blink/trunk@202014 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[Xianzhu, 2015-09-09 01:23:52]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, trchen@chromium.org

[chrishtr, 2015-09-09 17:05:41]

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
Source/core/paint/BackgroundImageGeometry.cpp:235: IntPoint
absoluteContainerOffset =
roundedIntPoint(paintContainer->localToAbsolute(FloatPoint()));
localToAbsolute includes scroll offsets...why isn't it happening here?

[Xianzhu, 2015-09-09 17:09:09]

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
Source/core/paint/BackgroundImageGeometry.cpp:235: IntPoint
absoluteContainerOffset =
roundedIntPoint(paintContainer->localToAbsolute(FloatPoint()));
On 2015/09/09 17:05:41, chrishtr wrote:
> localToAbsolute includes scroll offsets...why isn't it happening here?

It includes scroll offsets from root to the paint container, not including those
from the paint container to the current painting object.

[chrishtr, 2015-09-09 17:29:16]

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
Source/core/paint/BackgroundImageGeometry.cpp:235: IntPoint
absoluteContainerOffset =
roundedIntPoint(paintContainer->localToAbsolute(FloatPoint()));
On 2015/09/09 at 17:09:09, Xianzhu wrote:
> On 2015/09/09 17:05:41, chrishtr wrote:
> > localToAbsolute includes scroll offsets...why isn't it happening here?
> 
> It includes scroll offsets from root to the paint container, not including
those from the paint container to the current painting object.

Oh right.

How about replacing your code with mapLocalToContainer from the local object to
the paintContainer? Then this code can at least be localized to fixed-position
backgrounds.

[Xianzhu, 2015-09-09 20:40:03]

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
Source/core/paint/BackgroundImageGeometry.cpp:235: IntPoint
absoluteContainerOffset =
roundedIntPoint(paintContainer->localToAbsolute(FloatPoint()));
On 2015/09/09 17:29:16, chrishtr wrote:
> On 2015/09/09 at 17:09:09, Xianzhu wrote:
> > On 2015/09/09 17:05:41, chrishtr wrote:
> > > localToAbsolute includes scroll offsets...why isn't it happening here?
> > 
> > It includes scroll offsets from root to the paint container, not including
> those from the paint container to the current painting object.
> 
> Oh right.
> 
> How about replacing your code with mapLocalToContainer from the local object
to
> the paintContainer? Then this code can at least be localized to fixed-position
> backgrounds.

This seems not to work because we only need to compensate scroll offsets for
which we issued translations, but not other offsets in paint offset.

[chrishtr, 2015-09-09 20:48:15]

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
Source/core/paint/BackgroundImageGeometry.cpp:235: IntPoint
absoluteContainerOffset =
roundedIntPoint(paintContainer->localToAbsolute(FloatPoint()));
On 2015/09/09 at 20:40:03, Xianzhu wrote:
> On 2015/09/09 17:29:16, chrishtr wrote:
> > On 2015/09/09 at 17:09:09, Xianzhu wrote:
> > > On 2015/09/09 17:05:41, chrishtr wrote:
> > > > localToAbsolute includes scroll offsets...why isn't it happening here?
> > > 
> > > It includes scroll offsets from root to the paint container, not including
> > those from the paint container to the current painting object.
> > 
> > Oh right.
> > 
> > How about replacing your code with mapLocalToContainer from the local object
to
> > the paintContainer? Then this code can at least be localized to
fixed-position
> > backgrounds.
> 
> This seems not to work because we only need to compensate scroll offsets for
which we issued translations, but not other offsets in paint offset.

Maybe just make a one-off method that accumulates just scroll offsets then? SP
phase 2 should
allow us to fix this more cleanly by specifying the root transform space, once
trchen's
work is done. In the meantime I think this should be as targeted of a fix as
possible.

[Xianzhu, 2015-09-09 22:04:06]

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/1/Source/core/paint/Backgroun...
Source/core/paint/BackgroundImageGeometry.cpp:235: IntPoint
absoluteContainerOffset =
roundedIntPoint(paintContainer->localToAbsolute(FloatPoint()));
On 2015/09/09 20:48:15, chrishtr wrote:
> On 2015/09/09 at 20:40:03, Xianzhu wrote:
> > On 2015/09/09 17:29:16, chrishtr wrote:
> > > On 2015/09/09 at 17:09:09, Xianzhu wrote:
> > > > On 2015/09/09 17:05:41, chrishtr wrote:
> > > > > localToAbsolute includes scroll offsets...why isn't it happening here?
> > > > 
> > > > It includes scroll offsets from root to the paint container, not
including
> > > those from the paint container to the current painting object.
> > > 
> > > Oh right.
> > > 
> > > How about replacing your code with mapLocalToContainer from the local
object
> to
> > > the paintContainer? Then this code can at least be localized to
> fixed-position
> > > backgrounds.
> > 
> > This seems not to work because we only need to compensate scroll offsets for
> which we issued translations, but not other offsets in paint offset.
> 
> Maybe just make a one-off method that accumulates just scroll offsets then? SP
> phase 2 should
> allow us to fix this more cleanly by specifying the root transform space, once
> trchen's
> work is done. In the meantime I think this should be as targeted of a fix as
> possible.

Good idea! Done.

[chrishtr, 2015-09-09 22:07:05]

https://codereview.chromium.org/1332643002/diff/20001/Source/core/paint/Backg...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/20001/Source/core/paint/Backg...
Source/core/paint/BackgroundImageGeometry.cpp:246:
viewportRect.moveBy(accumulatedScrollOffset(obj, paintContainer));
Add a TODO(trchen) to fix this for SP phase 2, and note in the appropriate bug.

https://codereview.chromium.org/1332643002/diff/20001/Source/core/paint/Backg...
Source/core/paint/BackgroundImageGeometry.cpp:249: if (paintContainer &&
paintContainer->view() == obj.view()) {
Why this change?

[Xianzhu, 2015-09-09 22:40:47]

https://codereview.chromium.org/1332643002/diff/20001/Source/core/paint/Backg...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/20001/Source/core/paint/Backg...
Source/core/paint/BackgroundImageGeometry.cpp:246:
viewportRect.moveBy(accumulatedScrollOffset(obj, paintContainer));
On 2015/09/09 22:07:05, chrishtr wrote:
> Add a TODO(trchen) to fix this for SP phase 2, and note in the appropriate
bug.

Done.

https://codereview.chromium.org/1332643002/diff/20001/Source/core/paint/Backg...
Source/core/paint/BackgroundImageGeometry.cpp:249: if (paintContainer &&
paintContainer->view() == obj.view()) {
On 2015/09/09 22:07:05, chrishtr wrote:
> Why this change?

Thought paintContainer could be in ancestor view of the current view. Debugged
and found that paintContainer will be the view even if the view is not
composited.

Removed the change.

[chrishtr, 2015-09-09 22:46:33]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-09-09 22:46:33]

lgtm

https://codereview.chromium.org/1332643002/diff/40001/Source/core/paint/Backg...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/40001/Source/core/paint/Backg...
Source/core/paint/BackgroundImageGeometry.cpp:246: // TODO(trchen): Fix this for
SP phase 2. crbug.com/528226.
Wrong bug? I was thinking a tracking bug for this work in phase 2.

[commit-bot: I haz the power, 2015-09-09 22:46:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332643002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332643002/40001

[Xianzhu, 2015-09-09 23:08:20]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2015-09-09 23:39:57]

https://codereview.chromium.org/1332643002/diff/40001/Source/core/paint/Backg...
File Source/core/paint/BackgroundImageGeometry.cpp (right):

https://codereview.chromium.org/1332643002/diff/40001/Source/core/paint/Backg...
Source/core/paint/BackgroundImageGeometry.cpp:246: // TODO(trchen): Fix this for
SP phase 2. crbug.com/528226.
On 2015/09/09 22:46:33, chrishtr wrote:
> Wrong bug? I was thinking a tracking bug for this work in phase 2.

Wanted to reuse the bug for phase 2.

Filed crbug.com/529963 and updated the above line.

[Xianzhu, 2015-09-09 23:40:01]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-09-09 23:40:01]

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/1332643002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-09-09 23:40:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332643002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332643002/60001

[commit-bot: I haz the power, 2015-09-10 01:15:13]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202014

[commit-bot: I haz the power, 2015-09-23 12:05:51]

Patchset 4 (id:??) landed as
https://crrev.com/efecb10be7c17ac2632cd86b890915d441e2da31