Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842483002:
Set relatedtarget for dragleave/enter events (Closed)
|
Created:
3 years, 8 months ago by Navid Zolghadr Modified:
3 years, 7 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet relatedtarget for dragleave/enter events
Chrome use to set the relatedTarget of boundary
drag events to null. This is not what the spec
says and developers expect. This CL fix this
by setting the relatedTarget to the element that
the pointer just left/entered (based on the event).
BUG=159534
Review-Url: https://codereview.chromium.org/2842483002
Cr-Commit-Position: refs/heads/master@{#471843}
Committed: https://chromium.googlesource.com/chromium/src/+/611466a4430557a67478d236ac57dc649b6eee99
Patch Set 1 #Patch Set 2 : Fix a typo #Patch Set 3 : Fix dragover problem #Patch Set 4 : Add test and link to the spec #Patch Set 5 : Fix test title #
Total comments: 5
Patch Set 6 : Add event sender input automation #Messages
Total messages: 45 (31 generated)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Description was changed from ========== Set relatedtarget for dragleave/enter/over events Chrome use to set the relatedTarget of boundary drag events to null. This is not what the spec says and developers expect. This CL fix this by setting the relatedTarget to the element that the pointer just left/entered (based on the event). BUG=159534 ========== to ========== Set relatedtarget for dragleave/enter events Chrome use to set the relatedTarget of boundary drag events to null. This is not what the spec says and developers expect. This CL fix this by setting the relatedTarget to the element that the pointer just left/entered (based on the event). BUG=159534 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org
On 2017/04/24 19:47:29, Navid Zolghadr wrote: Can you reference the spec in the CL? Tests?
On 2017/04/24 19:51:52, dtapuska wrote: > On 2017/04/24 19:47:29, Navid Zolghadr wrote: > > Can you reference the spec in the CL? > Tests? I wanted to ask about the best place to add the tests. Do you think manual wpt tests are better for these rather than our layout tests? Do you know where the related tests live in wpt repo?
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 nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal
https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html (right): https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html:20: //var drag_test = async_test("dragenter and dragleave are correctly fired."); Um this line is commented out.. How does this work? https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html:27: e.dataTransfer.setData("text", draggable.innerHTML); Is this necessary?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lanwei@chromium.org changed reviewers: + lanwei@chromium.org
https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt_automation/html/editing/dnd/events/relatedTarget-attribute-manual-automation.js (right): https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt_automation/html/editing/dnd/events/relatedTarget-attribute-manual-automation.js:4: return mouseDragInTargets(['#draggable', '#outerdiv', '#innerdiv', '#outerdiv']); I looked at the code and confirmed with Sadrul, basically it is difficult to make the drag and drop work for SyntheticPointerActions for all platforms. Each platform has its own specific code doing drag and drop, like linux and chromeOS, they use DesktopDragDropClientAuraX11::OnXdndDrop and ash::DragDropController, Windows uses DesktopDropTargetWin::OnDragEnter. They are taking low-level OS events, but SyntheticPointerActions is dispatched as ui::mouseevent from SyntheticGestureTargetAura for mouse type. Now, we have to use eventSender to generate drag and drop, it call EventSender::DoDragAfterMouseUp directly.
nzolghadr@chromium.org changed reviewers: + chongz@chromium.org
Adding chongz@ regarding the result of DnD investigation in GPU benchmarking extension. https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html (right): https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html:27: e.dataTransfer.setData("text", draggable.innerHTML); On 2017/04/25 19:46:06, dtapuska wrote: > Is this necessary? For some reason without this FF doesn't start the drag operation. So to get it passed on FF I had to add this.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. I added the event sender automation for drag and drop. Chong, you might be able to use the same or the modified version of it if that works for you for now until we get a better alternative for these events. https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html (right): https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html:20: //var drag_test = async_test("dragenter and dragleave are correctly fired."); On 2017/04/25 19:46:06, dtapuska wrote: > Um this line is commented out.. How does this work? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
On 2017/05/09 19:54:05, Navid Zolghadr wrote: > ptal. I added the event sender automation for drag and drop. > > > Chong, you might be able to use the same or the modified version of it if that > works for you for now until we get a better alternative for these events. > Ya I guess we have to rely on |eventSender()| for now... Thanks for keeping me in the loop! > https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html > (right): > > https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html:20: > //var drag_test = async_test("dragenter and dragleave are correctly fired."); > On 2017/04/25 19:46:06, dtapuska wrote: > > Um this line is commented out.. How does this work? > > Done.
On 2017/05/09 20:26:12, chongz wrote: > On 2017/05/09 19:54:05, Navid Zolghadr wrote: > > ptal. I added the event sender automation for drag and drop. > > > > > > Chong, you might be able to use the same or the modified version of it if that > > works for you for now until we get a better alternative for these events. > > > > Ya I guess we have to rely on |eventSender()| for now... Thanks for keeping me > in the loop! > > > > https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... > > File > > > third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html > > (right): > > > > > https://codereview.chromium.org/2842483002/diff/80001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/external/wpt/html/editing/dnd/events/relatedTarget-attribute-manual.html:20: > > //var drag_test = async_test("dragenter and dragleave are correctly fired."); > > On 2017/04/25 19:46:06, dtapuska wrote: > > > Um this line is commented out.. How does this work? > > > > Done. lgtm. The drag_target_.Get() can be replaced with just drag_target_ if you have to rebase this. If not it is fine to leave for another time.
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494867649293600,
"parent_rev": "6bfdb7745ef1171b8efa233d07f81ff6902368d1", "commit_rev":
"611466a4430557a67478d236ac57dc649b6eee99"}
Message was sent while issue was closed.
Description was changed from ========== Set relatedtarget for dragleave/enter events Chrome use to set the relatedTarget of boundary drag events to null. This is not what the spec says and developers expect. This CL fix this by setting the relatedTarget to the element that the pointer just left/entered (based on the event). BUG=159534 ========== to ========== Set relatedtarget for dragleave/enter events Chrome use to set the relatedTarget of boundary drag events to null. This is not what the spec says and developers expect. This CL fix this by setting the relatedTarget to the element that the pointer just left/entered (based on the event). BUG=159534 Review-Url: https://codereview.chromium.org/2842483002 Cr-Commit-Position: refs/heads/master@{#471843} Committed: https://chromium.googlesource.com/chromium/src/+/611466a4430557a67478d236ac57... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/611466a4430557a67478d236ac57... |
