Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2340533003:
Fix issue with non-touchpad scrolling event over resizer: should not cause an area resize. (Closed)
DescriptionFix issue with non-touchpad scrolling event over resizer: should not cause an area resize.
BUG=632930
Committed: https://crrev.com/28d25ce85ea355af02e766225fb6f8d79a5ed374
Cr-Commit-Position: refs/heads/master@{#419217}
Patch Set 1 : Initial Commit #
Total comments: 8
Patch Set 2 : Codereview update #
Messages
Total messages: 27 (19 generated)
The CQ bit was checked by lunalu@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by lunalu@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...
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by lunalu@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lunalu@chromium.org changed reviewers: + bokan@chromium.org, iclelland@chromium.org
Hi, could you please review my CL? Thanks
https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html (right): https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:2: <meta charset="utf-8"> No need for this meta https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:5: <body> No need for body tags either. See https://www.chromium.org/blink/coding-style/layout-test-style-guidelines https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:22: test(function() { I'd actually make test() wrap each |resize| call below. i.e. test(function() { resize("textarea", 20, "touchscreen") }, "Touchscreen drag resizes textarea"); ... https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:31: resize("textarea", 0, "touchpad"); Unless I'm missing something, if deltaY is 0, doesn't that mean we wouldn't resize textarea/DIV even without the fix? I think you want separate "scrollAmount"/"expectedSizeChange" params.
Patchset #2 (id:80001) has been deleted
Done. Please take another look. Thanks https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html (right): https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:2: <meta charset="utf-8"> On 2016/09/16 15:45:35, bokan wrote: > No need for this meta Done. https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:5: <body> On 2016/09/16 15:45:35, bokan wrote: > No need for body tags either. See > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines Done. https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:22: test(function() { On 2016/09/16 15:45:35, bokan wrote: > I'd actually make test() wrap each |resize| call below. i.e. > > test(function() { resize("textarea", 20, "touchscreen") }, "Touchscreen drag > resizes textarea"); > ... Done. https://codereview.chromium.org/2340533003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-over-resizer.html:31: resize("textarea", 0, "touchpad"); On 2016/09/16 15:45:35, bokan wrote: > Unless I'm missing something, if deltaY is 0, doesn't that mean we wouldn't > resize textarea/DIV even without the fix? I think you want separate > "scrollAmount"/"expectedSizeChange" params. Done.
Changes look good. If you haven't already, please try running the test case without your fix applied and make sure it fails. (It should fail without your fix, pass with your fix). Otherwise, lgtm!
On 2016/09/16 16:16:35, bokan wrote: > Changes look good. If you haven't already, please try running the test case > without your fix applied and make sure it fails. (It should fail without your > fix, pass with your fix). > > Otherwise, lgtm! Thanks for reviewing my CL. I just run the test without my patch and it failed as expected: PASS Touchscreen drag resizes textarea FAIL Touchpad scroll should not resize textarea assert_equals: expected 0 but got 20 PASS Touchscreen drag resizes resizable div FAIL Touchpad scroll should not resize resizable div assert_equals: expected 0 but got 20
The CQ bit was checked by lunalu@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 lunalu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix issue with non-touchpad scrolling event over resizer: should not cause an area resize. BUG=632930 ========== to ========== Fix issue with non-touchpad scrolling event over resizer: should not cause an area resize. BUG=632930 Committed: https://crrev.com/28d25ce85ea355af02e766225fb6f8d79a5ed374 Cr-Commit-Position: refs/heads/master@{#419217} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/28d25ce85ea355af02e766225fb6f8d79a5ed374 Cr-Commit-Position: refs/heads/master@{#419217} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
