[SPv2] Replay DragImages into PropertyTreeState of the enclosing stacking context

[SPv2] Replay DragImages into PropertyTreeState of the enclosing stacking context.

Previously we were replaying them into the root PropertyTreeState. This is
incorrect, because it would include any transform, clip or effect nodes that
are ancestors of node that we want a DragImage from.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
.

Review-Url: https://codereview.chromium.org/2784373004
Cr-Commit-Position: refs/heads/master@{#461261}
Committed: https://chromium.googlesource.com/chromium/src/+/dc1a0f2c42b5de2840c22e0a3e5c5edc57dd4715

[chrishtr, 2017-03-31 17:34:04]

Description was changed from

==========
none


none


none


none

BUG=
==========

to

==========
none


none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-03-31 17:35:30]

Description was changed from

==========
none


none


none


none

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPv2] Replay DragImages into PropertyTreeState of the enclosing stacking
context.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
.
==========

[chrishtr, 2017-03-31 17:35:59]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[chrishtr, 2017-03-31 17:36:10]

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

[commit-bot: I haz the power, 2017-03-31 17:36:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2017-03-31 17:37:24]

Description was changed from

==========
[SPv2] Replay DragImages into PropertyTreeState of the enclosing stacking
context.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
.
==========

to

==========
[SPv2] Replay DragImages into PropertyTreeState of the enclosing stacking
context.

Previously we were replaying them into the root PropertyTreeState. This is
incorrect, because it would include any transform, clip or effect nodes that
are ancestors of node that we want a DragImage from.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
.
==========

[wkorman, 2017-03-31 18:21:52]

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html:29: var
startX = image.offsetLeft + image.offsetWidth / 2;
Just validating the image, I expected this to mean basically:
- move to center of box (50px from left/top)
- click and move right and down 100px (so cursor now at (150, 150) from
starting)
- with transform of +50px in x applied, would = (200, 150)

but the expected image looks like it's been shifted 50px to the right, and 0px
down?

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/drag-image-transformed-parent.html
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/drag-image-transformed-parent.html:30: var
image = document.getElementById('draggable');
Wanted to validate similarly but expected image is not rendering, will wait to
hear your comment on previous.

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:74: // In SPv2
mode, replayes into the ancestor state given by |replayState|.
replayes -> replays

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h:54: // In
SPv2 mode, replayes into the ancestor state given by |replayState|.
replays

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h:54: // In
SPv2 mode, replayes into the ancestor state given by |replayState|.
Param has different name (and only in .cpp file) here.

[commit-bot: I haz the power, 2017-03-31 20:00:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 20:00:39]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-03-31 20:59:04]

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html:29: var
startX = image.offsetLeft + image.offsetWidth / 2;
On 2017/03/31 at 18:21:51, wkorman wrote:
> Just validating the image, I expected this to mean basically:
> - move to center of box (50px from left/top)
> - click and move right and down 100px (so cursor now at (150, 150) from
starting)
> - with transform of +50px in x applied, would = (200, 150)
> 
> but the expected image looks like it's been shifted 50px to the right, and 0px
down?

This is because I put a transform: translateX(50px) on the child.
It's expected for it to be shifted by 50px.

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/drag-image-transformed-parent.html
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/drag-image-transformed-parent.html:30: var
image = document.getElementById('draggable');
On 2017/03/31 at 18:21:51, wkorman wrote:
> Wanted to validate similarly but expected image is not rendering, will wait to
hear your comment on previous.

Good catch, sorry I missed it. It worked correctly with manual local checking
via mouse drag, but the image capture failed. the reason is that the
positioning of the mouse event must be adjusted for the translatex(50).
New patch set does that.

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h (right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.h:74: // In SPv2
mode, replayes into the ancestor state given by |replayState|.
On 2017/03/31 at 18:21:51, wkorman wrote:
> replayes -> replays

Done.

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintRecordBuilder.h:54: // In
SPv2 mode, replayes into the ancestor state given by |replayState|.
On 2017/03/31 at 18:21:51, wkorman wrote:
> replays

Fixed

[chrishtr, 2017-03-31 20:59:14]

The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 21:00:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wkorman, 2017-03-31 21:08:18]

lgtm

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html:29: var
startX = image.offsetLeft + image.offsetWidth / 2;
On 2017/03/31 20:59:03, chrishtr wrote:
> On 2017/03/31 at 18:21:51, wkorman wrote:
> > Just validating the image, I expected this to mean basically:
> > - move to center of box (50px from left/top)
> > - click and move right and down 100px (so cursor now at (150, 150) from
> starting)
> > - with transform of +50px in x applied, would = (200, 150)
> > 
> > but the expected image looks like it's been shifted 50px to the right, and
0px
> down?
> 
> This is because I put a transform: translateX(50px) on the child.
> It's expected for it to be shifted by 50px.

Yes, but I incorporated that into my third bullet, but I still compute expected
end position of the box as (200, 150). I haven't personally dumped drag images
previously. I would expect (200, 150) would be the center of the dragged image?
Which would put the top left at (100, 50), but in the image the green is at (50,
0).

But I am sure I am just misinterpreting/computing somehow, please clarify.

https://codereview.chromium.org/2784373004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2784373004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:221: LOG(ERROR) << "layer: "
<< layer->layoutObject().debugName();
remove

[chrishtr, 2017-03-31 21:17:59]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-03-31 21:18:00]

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html
(right):

https://codereview.chromium.org/2784373004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/drag-image-transformed-child.html:29: var
startX = image.offsetLeft + image.offsetWidth / 2;
On 2017/03/31 at 21:08:18, wkorman wrote:
> On 2017/03/31 20:59:03, chrishtr wrote:
> > On 2017/03/31 at 18:21:51, wkorman wrote:
> > > Just validating the image, I expected this to mean basically:
> > > - move to center of box (50px from left/top)
> > > - click and move right and down 100px (so cursor now at (150, 150) from
> > starting)
> > > - with transform of +50px in x applied, would = (200, 150)
> > > 
> > > but the expected image looks like it's been shifted 50px to the right, and
0px
> > down?
> > 
> > This is because I put a transform: translateX(50px) on the child.
> > It's expected for it to be shifted by 50px.
> 
> Yes, but I incorporated that into my third bullet, but I still compute
expected end position of the box as (200, 150). I haven't personally dumped drag
images previously. I would expect (200, 150) would be the center of the dragged
image? Which would put the top left at (100, 50), but in the image the green is
at (50, 0).
> 
> But I am sure I am just misinterpreting/computing somehow, please clarify.

Oh I see, sorry. The drag image is independent of how much you drag it.
The image is sized according to the (stacking context ancestor of) the element
which received the dragging request, positioned at the 0,0 of that ancestor,
and painted down from there. This is why the green square is shifted 50px to the
right, because we're painting the containing stacking context (which is the
HTML/BODY element) and the green square is offset 50px from it.

https://codereview.chromium.org/2784373004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2784373004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:221: LOG(ERROR) << "layer: "
<< layer->layoutObject().debugName();
On 2017/03/31 at 21:08:18, wkorman wrote:
> remove

Fixed.

[chrishtr, 2017-03-31 21:18:01]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2784373004/#ps40001
(title: "none")

[commit-bot: I haz the power, 2017-03-31 21:18:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[trchen, 2017-03-31 21:22:32]

Well there is really no standards about how drag images should be drawn. Just
wondering what is the behavior users expect?

For example:
<div style="transform:rotate(30deg)">
  <div id="enclosing_stacking_context"style="position:relative; z-index:0">
    <div>Drag me!</div>
  </div>
</div>

Should the drag image to be straight or rotated? I feel like I'm more used to
rotated image for this case.
----
On the other hand,
<div style="width:100px; height:100px; overflow:scroll;">
  <div style="width:200px; height:200px;">
    Drag me!
    Lorem ipsum
    ...
    With a lot over overflow.
  </div>
</div>

I believe it would be better user experience if we show the whole selection
without getting clipped.
----
Anyway, code-wise I don't see anything wrong.

[chrishtr, 2017-03-31 21:27:15]

On 2017/03/31 at 21:22:32, trchen wrote:
> Well there is really no standards about how drag images should be drawn. Just
wondering what is the behavior users expect?
> 
> For example:
> <div style="transform:rotate(30deg)">
>   <div id="enclosing_stacking_context"style="position:relative; z-index:0">
>     <div>Drag me!</div>
>   </div>
> </div>
> 
> Should the drag image to be straight or rotated? I feel like I'm more used to
rotated image for this case.

SPv1 draws it non-rotated (just checked). Best to match.

> ----
> On the other hand,
> <div style="width:100px; height:100px; overflow:scroll;">
>   <div style="width:200px; height:200px;">
>     Drag me!
>     Lorem ipsum
>     ...
>     With a lot over overflow.
>   </div>
> </div>
> 
> I believe it would be better user experience if we show the whole selection
without getting clipped.
> ----
> Anyway, code-wise I don't see anything wrong.

[trchen, 2017-03-31 21:28:04]

On 2017/03/31 21:27:15, chrishtr wrote:
> On 2017/03/31 at 21:22:32, trchen wrote:
> > Well there is really no standards about how drag images should be drawn.
Just
> wondering what is the behavior users expect?
> > 
> > For example:
> > <div style="transform:rotate(30deg)">
> >   <div id="enclosing_stacking_context"style="position:relative; z-index:0">
> >     <div>Drag me!</div>
> >   </div>
> > </div>
> > 
> > Should the drag image to be straight or rotated? I feel like I'm more used
to
> rotated image for this case.
> 
> SPv1 draws it non-rotated (just checked). Best to match.

Ah, probably pdr's change a few months back. lgtm then.

[commit-bot: I haz the power, 2017-03-31 22:51:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490995079926690,
"parent_rev": "eefd3fc93296ee6b3ef9c5055d024573d8c7f63d", "commit_rev":
"dc1a0f2c42b5de2840c22e0a3e5c5edc57dd4715"}

[commit-bot: I haz the power, 2017-03-31 22:52:45]

Description was changed from

==========
[SPv2] Replay DragImages into PropertyTreeState of the enclosing stacking
context.

Previously we were replaying them into the root PropertyTreeState. This is
incorrect, because it would include any transform, clip or effect nodes that
are ancestors of node that we want a DragImage from.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
.
==========

to

==========
[SPv2] Replay DragImages into PropertyTreeState of the enclosing stacking
context.

Previously we were replaying them into the root PropertyTreeState. This is
incorrect, because it would include any transform, clip or effect nodes that
are ancestors of node that we want a DragImage from.

BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
.

Review-Url: https://codereview.chromium.org/2784373004
Cr-Commit-Position: refs/heads/master@{#461261}
Committed:
https://chromium.googlesource.com/chromium/src/+/dc1a0f2c42b5de2840c22e0a3e5c...
==========

[commit-bot: I haz the power, 2017-03-31 22:52:57]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/dc1a0f2c42b5de2840c22e0a3e5c...