Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2749313002:
Move slider implemention to use pointer capture (Closed)
|
Created:
3 years, 9 months ago by Navid Zolghadr Modified:
3 years, 9 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove slider implemention to use pointer capture
Previously slider input implementation was using
a hack in EventHandler to capture all the incoming
inputs. Now that pointer capture APIs are
introduced we can remove that hack.
BUG=701058
Review-Url: https://codereview.chromium.org/2749313002
Cr-Commit-Position: refs/heads/master@{#457846}
Committed: https://chromium.googlesource.com/chromium/src/+/f4f29e883a0d68017e912d930b0070c3d76e8506
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use only pointer capture #Patch Set 3 : Add a note #Messages
Total messages: 26 (16 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...
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
ptal. One thing I found weird was that the defaultEventHandler inside our widgets seems to be executed before the pointerdown handler of the js on the input element. Is that how shadow dom is supposed to work here?
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_...)
https://codereview.chromium.org/2749313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/2749313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:99: void SliderThumbElement::dragFrom(const PointerEvent* event) { Please rename |dragFrom| to something like |initiateDrag|, the "drag from point" is no longer the case. Another nit: const PointerEvent& instead? https://codereview.chromium.org/2749313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:249: requestPointerCapture(pointerEvent->pointerId()); So a chorded L-down would start dragging here, right? I think initiating through just a pointerdown with L-button is enough, old mouse code didn't anticipate the chorded button case. https://codereview.chromium.org/2749313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:252: if ((eventType == EventTypeNames::pointerup || May be just |if (!isLeftButtonActive) {} |? https://codereview.chromium.org/2749313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:264: HTMLDivElement::defaultEventHandler(event); Why not skip this? Is this "safe"? Passing pointer events through the default handlers could be bad. Again, I have seen handlers that break with pointer events.
https://codereview.chromium.org/2749313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp (right): https://codereview.chromium.org/2749313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:350: handleTouchEvent(toTouchEvent(event)); The slider-container takes care of the TEs, which should be nuked/replaced too.
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. Due to the backward comparability issue I faced here I changed the CL to minimally just get rid of that capturing hack rather than only using pointerevents in all the paths. I believe at this stage this is as close as it gets to using pointerevents in this widget.
On 2017/03/16 19:58:03, Navid Zolghadr wrote: > ptal. > > Due to the backward comparability issue I faced here I changed the CL to > minimally just get rid of that capturing hack rather than only using > pointerevents in all the paths. I believe at this stage this is as close as it > gets to using pointerevents in this widget. lgtm % can we add a comment here why we only use s_mouseId.. indicating that this control is only bound for mouse events?
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...
On 2017/03/16 20:00:03, dtapuska wrote: > On 2017/03/16 19:58:03, Navid Zolghadr wrote: > > ptal. > > > > Due to the backward comparability issue I faced here I changed the CL to > > minimally just get rid of that capturing hack rather than only using > > pointerevents in all the paths. I believe at this stage this is as close as it > > gets to using pointerevents in this widget. > > lgtm % can we add a comment here why we only use s_mouseId.. indicating that > this control is only bound for mouse events? Done. This element is not bound to mouse but the touch handling is occurring somewhere else and the touch events are implicitly captured to the element anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
nzolghadr@chromium.org changed reviewers: + bokan@chromium.org
rs lgtm based on Dave's review
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/2749313002/#ps40001 (title: "Add a note")
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": 40001, "attempt_start_ts": 1489777918813940,
"parent_rev": "2991f43a94bbdb2eb798a8f256b67b5b3865f129", "commit_rev":
"f4f29e883a0d68017e912d930b0070c3d76e8506"}
Message was sent while issue was closed.
Description was changed from ========== Move slider implemention to use pointer capture Previously slider input implementation was using a hack in EventHandler to capture all the incoming inputs. Now that pointer capture APIs are introduced we can remove that hack. BUG=701058 ========== to ========== Move slider implemention to use pointer capture Previously slider input implementation was using a hack in EventHandler to capture all the incoming inputs. Now that pointer capture APIs are introduced we can remove that hack. BUG=701058 Review-Url: https://codereview.chromium.org/2749313002 Cr-Commit-Position: refs/heads/master@{#457846} Committed: https://chromium.googlesource.com/chromium/src/+/f4f29e883a0d68017e912d930b00... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f4f29e883a0d68017e912d930b00... |
