Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1982943002:
Drag images should paint descendants that are painting siblings of the dragged element (Closed)
|
Created:
4 years, 7 months ago by trchen Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDrag images should paint descendants that are painting siblings of the dragged element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. In this CL we start
painting from the nearest stacking context instead.
BUG=605119
Committed: https://crrev.com/40333ebebd01babf43339e66ee470679a9922244
Cr-Commit-Position: refs/heads/master@{#395729}
Patch Set 1 #
Total comments: 1
Patch Set 2 : start paint at parent stacking context & add test #
Total comments: 4
Patch Set 3 : update comments & rebase #
Messages
Total messages: 39 (17 generated)
Description was changed from
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. We should collect the DOM
descendants that are painting siblings of current element a paint them in
appropriate order.
BUG=605119
==========
to
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. We should collect the DOM
descendants that are painting siblings of current element and paint them in
appropriate order.
BUG=605119
==========
trchen@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
It is gross but I'm afraid it has to be this gross. :( https://codereview.chromium.org/1982943002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1982943002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalFrame.cpp:653: PaintLayerPainter(*layer).paintLayer(dragImageBuilder.context(), paintingInfo, flags | PaintLayerAppliedTransform); I felt we should also address pdr's comment on line 621. This line here will paint other contents that belongs to the same (psuedo) stacking context that are not necessarily descendants of the target element. ObjectPainter::paintAllPhasesAtomically can be used but calculating paint offset is a pain.
How about simply painting from the nearest enclosing stacking context? And actually clipped to the bounds of the object being clipped.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982943002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982943002/20001
On 2016/05/17 22:21:47, chrishtr wrote: > How about simply painting from the nearest enclosing stacking context? And > actually clipped > to the bounds of the object being clipped. Yea, that's less gross but is subject to more over-paint. I guess we can do whatever makes sense and is convenient to us.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982943002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982943002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2016/05/18 at 01:10:36, trchen wrote: > On 2016/05/17 22:21:47, chrishtr wrote: > > How about simply painting from the nearest enclosing stacking context? And > > actually clipped > > to the bounds of the object being clipped. > > Yea, that's less gross but is subject to more over-paint. I guess we can do whatever makes sense and is convenient to us. Let's do that. If it fails for some other reason, then perhaps we should just revert these changes to remove the paintingRoot field in PaintInfo.
https://codereview.chromium.org/1982943002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1982943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:620: // Paint starting at the nearest stacking context, clipped to the object itself. I wonder if we should explicitly clip now, to avoid over-painting of ancestors.
Please update the CL description also. https://codereview.chromium.org/1982943002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1982943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:621: // TODO(pdr): This will also paint the content behind the object if the object contains I'd change this to a note instead of a TODO. There is now no intention to "fix" it.
ping
Description was changed from
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. We should collect the DOM
descendants that are painting siblings of current element and paint them in
appropriate order.
BUG=605119
==========
to
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. In this CL we start
painting from the nearest stacking context instead.
BUG=605119
==========
& CL description updated. https://codereview.chromium.org/1982943002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1982943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:620: // Paint starting at the nearest stacking context, clipped to the object itself. On 2016/05/19 17:45:16, chrishtr wrote: > I wonder if we should explicitly clip now, to avoid over-painting of ancestors. As discussed offline, it is already clipped. https://codereview.chromium.org/1982943002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:621: // TODO(pdr): This will also paint the content behind the object if the object contains On 2016/05/19 17:46:52, chrishtr wrote: > I'd change this to a note instead of a TODO. There is now no intention to "fix" > it. Done.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982943002/60001
lgtm
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982943002/60001
On 2016/05/23 at 19:39:52, chrishtr wrote: > lgtm This seems reasonable to me too.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982943002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982943002/60001
Message was sent while issue was closed.
Description was changed from
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. In this CL we start
painting from the nearest stacking context instead.
BUG=605119
==========
to
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. In this CL we start
painting from the nearest stacking context instead.
BUG=605119
==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. In this CL we start
painting from the nearest stacking context instead.
BUG=605119
==========
to
==========
Drag images should paint descendants that are painting siblings of the dragged
element
DOM descendant of an element is not necessarily painting descendant of it.
For example:
<div id="A" style="position:relative; z-index:0;">
<div id="B" style="position:relative;">
<div id="C" style="position:relative;">
Although C is a child of B, C actually gets painted by A because B is not
a stacking context. For drag image purposes, when B is dragged, the users
expect to see the whole DOM subtree being dragged. In this CL we start
painting from the nearest stacking context instead.
BUG=605119
Committed: https://crrev.com/40333ebebd01babf43339e66ee470679a9922244
Cr-Commit-Position: refs/heads/master@{#395729}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/40333ebebd01babf43339e66ee470679a9922244 Cr-Commit-Position: refs/heads/master@{#395729} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
