Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401503002:
Align drag threshold across content types. (Closed)
DescriptionAlign drag threshold across content types.
We inherited WebKit's drag and drop implementation, which has a
threshold of 40px for links, 5px for images, and 3px for text. While
these are very thoughtful defaults, the right behavior for an
application is to use drag thresholds dictated by the platform.
This CL unifies all the drag and drop thesholds, paving the way for
having them read from the underlying platform, which is explored in
https://crbug.com/653490
WebKit also used the term "drag hysteresis" instead of threshold. This
CL switches to threshold to match the documentation of the platforms
that we run on.
BUG=20486
Committed: https://crrev.com/fe307646932b8cd2948b88a02f380685341334f3
Cr-Commit-Position: refs/heads/master@{#426447}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Better patch based on feedback, added layout test. #
Total comments: 2
Patch Set 3 : Converted layout test to testharness.js. #
Total comments: 4
Patch Set 4 : Addressed nits. #
Messages
Total messages: 37 (25 generated)
The CQ bit was checked by pwnall@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: This issue passed the CQ dry run.
Drive-by (don't make me the reviewer of record) How hard is is it to actually get the correct values from the OS? We already use system metrics in various other places in Blink, so presumably this is not an unknown problem. https://codereview.chromium.org/2401503002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2401503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/MouseEventManager.cpp:70: // drag, and because dragging links is rare. This comment should be updated or removed. https://codereview.chromium.org/2401503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/MouseEventManager.cpp:71: const int kLinkDragHysteresis = 5; Perhaps we should just use the text/image values below (depending on whether the link is text/image, or whatever) instead of having a custom link value.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Given that the values are so close together now, I wonder if it even makes sense to maintain them separately anymore.
On 2016/10/06 09:14:43, dcheng wrote: > Given that the values are so close together now, I wonder if it even makes sense > to maintain them separately anymore. (I was sort of hoping that would be an outcome of my other comments.)
The CQ bit was checked by pwnall@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: This issue passed the CQ dry run.
Description was changed from ========== Change the link drag hysteresis from 40px to 5px. The 40px constant was inherited from WebKit. We use a 5px hysteresis for images, and a 3px hysteresis for text and everything else. Firefox also uses a small value and seems to be doing just fine. BUG=20486 ========== to ========== Align drag threshold across content types. We inherited WebKit's drag and drop implementation, which has a threshold of 40px for links, 5px for images, and 3px for text. While these are very thoughtful defaults, the right behavior for an application is to use drag thresholds dictated by the platform. This CL unifies all the drag and drop thesholds, paving the way for having them read from the underlying platform, which is explored in https://crbug.com/653490 BUG=20486 ==========
Description was changed from ========== Align drag threshold across content types. We inherited WebKit's drag and drop implementation, which has a threshold of 40px for links, 5px for images, and 3px for text. While these are very thoughtful defaults, the right behavior for an application is to use drag thresholds dictated by the platform. This CL unifies all the drag and drop thesholds, paving the way for having them read from the underlying platform, which is explored in https://crbug.com/653490 BUG=20486 ========== to ========== Align drag threshold across content types. We inherited WebKit's drag and drop implementation, which has a threshold of 40px for links, 5px for images, and 3px for text. While these are very thoughtful defaults, the right behavior for an application is to use drag thresholds dictated by the platform. This CL unifies all the drag and drop thesholds, paving the way for having them read from the underlying platform, which is explored in https://crbug.com/653490 WebKit also used the term "drag hysteresis" instead of threshold. This CL switches to threshold to match the documentation of the platforms that we run on. BUG=20486 ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by pwnall@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...
On 2016/10/06 09:05:36, Peter Kasting wrote: > How hard is is it to actually get the correct values from the OS? We already > use system metrics in various other places in Blink, so presumably this is not > an unknown problem. Getting the values from the OS is not hard, but it's not trivial either. I documented some approaches in http://crbug.com/653490 which is mentioned in a TODO in this patch.
dcheng@: Thank you for volunteering to review this CL! PTAL? https://chromiumcodereview.appspot.com/2401503002/diff/1/third_party/WebKit/S... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://chromiumcodereview.appspot.com/2401503002/diff/1/third_party/WebKit/S... third_party/WebKit/Source/core/input/MouseEventManager.cpp:70: // drag, and because dragging links is rare. On 2016/10/06 09:05:36, Peter Kasting (OOO Oct. 6-9) wrote: > This comment should be updated or removed. Done. https://chromiumcodereview.appspot.com/2401503002/diff/1/third_party/WebKit/S... third_party/WebKit/Source/core/input/MouseEventManager.cpp:71: const int kLinkDragHysteresis = 5; On 2016/10/06 09:05:36, Peter Kasting wrote: > Perhaps we should just use the text/image values below (depending on whether the > link is text/image, or whatever) instead of having a custom link value. I opted to prepare the path for obeying the platform preferences for drag and drop thresholds. I created separate X and Y constants because Windows has separate X and Y thresholds.
https://chromiumcodereview.appspot.com/2401503002/diff/60001/third_party/WebK... File third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html (right): https://chromiumcodereview.appspot.com/2401503002/diff/60001/third_party/WebK... third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html:103: </script> Thanks for the good test! Would you be willing to try to make this a testharness.js based test? One example of a previous port from js-test.js to testharness.js: https://codereview.chromium.org/2264953003
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 pwnall@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/2401503002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html (right): https://codereview.chromium.org/2401503002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html:103: </script> On 2016/10/20 02:40:47, dcheng wrote: > Thanks for the good test! Would you be willing to try to make this a > testharness.js based test? One example of a previous port from js-test.js to > testharness.js: https://codereview.chromium.org/2264953003 Done. Thank you for the suggestion! I didn't think of testharness, becase I associated that with WPT, but it's nice to reuse its infrastructure!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html (right): https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html:27: function runTest(t, _, target, deltaX, deltaY, expectedDrag) { Might be slightly clearer to just name _ "description", even though it's unused here. https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:939: int thresholdY = kDragThresholdY; Nit: maybe just use the constants directly in the comparisons for now? When we add the code to dynamically grab values from the system, then it'll make more sense to have locals again.
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2401503002/#ps100001 (title: "Addressed nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you very much for the quick turnaround! https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html (right): https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/drag-and-drop-thresholds.html:27: function runTest(t, _, target, deltaX, deltaY, expectedDrag) { On 2016/10/20 09:11:55, dcheng wrote: > Might be slightly clearer to just name _ "description", even though it's unused > here. Done. https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2401503002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:939: int thresholdY = kDragThresholdY; On 2016/10/20 09:11:55, dcheng wrote: > Nit: maybe just use the constants directly in the comparisons for now? When we > add the code to dynamically grab values from the system, then it'll make more > sense to have locals again. Done.
Message was sent while issue was closed.
Description was changed from ========== Align drag threshold across content types. We inherited WebKit's drag and drop implementation, which has a threshold of 40px for links, 5px for images, and 3px for text. While these are very thoughtful defaults, the right behavior for an application is to use drag thresholds dictated by the platform. This CL unifies all the drag and drop thesholds, paving the way for having them read from the underlying platform, which is explored in https://crbug.com/653490 WebKit also used the term "drag hysteresis" instead of threshold. This CL switches to threshold to match the documentation of the platforms that we run on. BUG=20486 ========== to ========== Align drag threshold across content types. We inherited WebKit's drag and drop implementation, which has a threshold of 40px for links, 5px for images, and 3px for text. While these are very thoughtful defaults, the right behavior for an application is to use drag thresholds dictated by the platform. This CL unifies all the drag and drop thesholds, paving the way for having them read from the underlying platform, which is explored in https://crbug.com/653490 WebKit also used the term "drag hysteresis" instead of threshold. This CL switches to threshold to match the documentation of the platforms that we run on. BUG=20486 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Align drag threshold across content types. We inherited WebKit's drag and drop implementation, which has a threshold of 40px for links, 5px for images, and 3px for text. While these are very thoughtful defaults, the right behavior for an application is to use drag thresholds dictated by the platform. This CL unifies all the drag and drop thesholds, paving the way for having them read from the underlying platform, which is explored in https://crbug.com/653490 WebKit also used the term "drag hysteresis" instead of threshold. This CL switches to threshold to match the documentation of the platforms that we run on. BUG=20486 ========== to ========== Align drag threshold across content types. We inherited WebKit's drag and drop implementation, which has a threshold of 40px for links, 5px for images, and 3px for text. While these are very thoughtful defaults, the right behavior for an application is to use drag thresholds dictated by the platform. This CL unifies all the drag and drop thesholds, paving the way for having them read from the underlying platform, which is explored in https://crbug.com/653490 WebKit also used the term "drag hysteresis" instead of threshold. This CL switches to threshold to match the documentation of the platforms that we run on. BUG=20486 Committed: https://crrev.com/fe307646932b8cd2948b88a02f380685341334f3 Cr-Commit-Position: refs/heads/master@{#426447} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fe307646932b8cd2948b88a02f380685341334f3 Cr-Commit-Position: refs/heads/master@{#426447} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
