Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
594093002:
Fix for mouse capture and release on ClearButtonElement (Closed)
|
Created:
6 years, 3 months ago by Paritosh Kumar Modified:
6 years, 2 months ago Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFix for mouse capture and release on ClearButtonElement
ClearButtonElement captures mouse events during mouse down,
and preventing 'mouseup' skips to release the mouse event capturing.
This affects mouse events on other element.
So, Removed event mousedown and mouseup handling, just changed to click.
R=habib.virji@samsung.com
BUG=376467
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184076
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 27 (6 generated)
paritosh.in@samsung.com changed reviewers: + habib.virji@samsung.com
PTAL.
https://codereview.chromium.org/594093002/diff/1/Source/core/html/shadow/Clea... File Source/core/html/shadow/ClearButtonElement.cpp (right): https://codereview.chromium.org/594093002/diff/1/Source/core/html/shadow/Clea... Source/core/html/shadow/ClearButtonElement.cpp:109: } else if (event->type() == EventTypeNames::mousemove) { As suggested by tkent. Please use click instead of mouse events. Also add tests for your changes.
Patchset #2 (id:20001) has been deleted
On 2014/09/23 11:09:06, Habib Virji wrote: > https://codereview.chromium.org/594093002/diff/1/Source/core/html/shadow/Clea... > File Source/core/html/shadow/ClearButtonElement.cpp (right): > > https://codereview.chromium.org/594093002/diff/1/Source/core/html/shadow/Clea... > Source/core/html/shadow/ClearButtonElement.cpp:109: } else if (event->type() == > EventTypeNames::mousemove) { > As suggested by tkent. Please use click instead of mouse events. > > Also add tests for your changes. Thanks. Updted as suggested by tkent. and added a related test case. PTAL.
https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html (right): https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:9: nit: No need of extra lines. https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:10: <button id="js-btn-test" type="button"></button> nit:ditto https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:12: <div id="js-test-output" value=""></div> nit:ditto https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:23: var btn = document.getElementById("js-btn-test"); nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:24: var output = document.getElementById("js-test-output"); nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:35: btn.addEventListener("click", function () { nit: Opening braces on next line nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:36: output.value = "Clicked"; nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:39: document.addEventListener("mouseup", function (evt) { nit: Opening braces on next line nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:50: nit: extra line https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:51: shouldBeEqualToString('output.value', "Clicked"); nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:56: nit:Extra lines https://codereview.chromium.org/594093002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/ClearButtonElement.cpp (left): https://codereview.chromium.org/594093002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/ClearButtonElement.cpp:91: frame->eventHandler().setCapturingMouseEventsNode(this); setCapturingMouseEventsNode and m_capturing are not set. Which will lead to issue in releaseCapture and detach. Would suggest to set m_capturing in your hover scenario and clean up setCapturingMouseEventsNode in detach and releaseCapture as they do not seem relevant.
https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html (right): https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:9: nit: No need of extra lines. https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:10: <button id="js-btn-test" type="button"></button> nit:ditto https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:12: <div id="js-test-output" value=""></div> nit:ditto https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:23: var btn = document.getElementById("js-btn-test"); nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:24: var output = document.getElementById("js-test-output"); nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:35: btn.addEventListener("click", function () { nit: Opening braces on next line nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:36: output.value = "Clicked"; nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:39: document.addEventListener("mouseup", function (evt) { nit: Opening braces on next line nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:50: nit: extra line https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:51: shouldBeEqualToString('output.value', "Clicked"); nit: used double quotes, use single quotes https://codereview.chromium.org/594093002/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearButton-click-and-preventDefault-then-mouseCapture_status.html:56: nit:Extra lines https://codereview.chromium.org/594093002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/ClearButtonElement.cpp (left): https://codereview.chromium.org/594093002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/ClearButtonElement.cpp:91: frame->eventHandler().setCapturingMouseEventsNode(this); setCapturingMouseEventsNode and m_capturing are not set. Which will lead to issue in releaseCapture and detach. Would suggest to set m_capturing in your hover scenario and clean up setCapturingMouseEventsNode in detach and releaseCapture as they do not seem relevant.
Thanks. Since I am on leave, not updating.
Apologize, I was on leave so not updating. Updated as you suggested. Pleace can have a look on this. Thanks.
The CQ bit was checked by paritosh.in@samsung.com
The CQ bit was unchecked by paritosh.in@samsung.com
PTAL.
https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... File Source/core/html/shadow/ClearButtonElement.cpp (left): https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/ClearButtonElement.cpp:89: if (renderer() && renderer()->visibleToHitTesting()) { Should you not still take visibileToHitTesting into criteria when changing m_capturing state? https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... File Source/core/html/shadow/ClearButtonElement.cpp (right): https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/ClearButtonElement.cpp:84: m_capturing = true; This looks bit unclear. You are changing m_capturing only in state hovered(). What happens after click when m_capturing should not be changed to false..
Thanks According to tkent's suggestion on http://crbug.com/376467, ClearButtonElement doesn't need to use mouse event capturing. So removing mouse capturing from ClearButtonElement. PTAL. Thanks..
Thanks According to tkent's suggestion on http://crbug.com/376467, ClearButtonElement doesn't need to use mouse event capturing. So removing mouse capturing from ClearButtonElement. PTAL. Thanks.. https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... File Source/core/html/shadow/ClearButtonElement.cpp (left): https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/ClearButtonElement.cpp:89: if (renderer() && renderer()->visibleToHitTesting()) { On 2014/10/16 08:39:53, Habib Virji wrote: > Should you not still take visibileToHitTesting into criteria when changing > m_capturing state? Thanks. Done.
Thanks According to tkent's suggestion on http://crbug.com/376467, ClearButtonElement doesn't need to use mouse event capturing. So removing mouse capturing from ClearButtonElement. PTAL. Thanks.. https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... File Source/core/html/shadow/ClearButtonElement.cpp (left): https://codereview.chromium.org/594093002/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/ClearButtonElement.cpp:89: if (renderer() && renderer()->visibleToHitTesting()) { On 2014/10/16 08:39:53, Habib Virji wrote: > Should you not still take visibileToHitTesting into criteria when changing > m_capturing state? Thanks. Done.
Changes look good. Remove WIP and please submit for review. Add keishi and tkent for this change. https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html (right): https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:10: <div id="js-test-output" value=""></div> You do not need this, you can declare variable in script and set it value. https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:22: var output = document.getElementById('js-test-output'); It can be local variable here. https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:35: output.value = 'Clicked'; Can be just output = 'Clicked'; https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:38: document.addEventListener('mouseup', function (evt) { nit: opening brackets in next line.
Thanks. https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html (right): https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:10: <div id="js-test-output" value=""></div> On 2014/10/20 13:16:43, Habib Virji wrote: > You do not need this, you can declare variable in script and set it value. Done. https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:22: var output = document.getElementById('js-test-output'); On 2014/10/20 13:16:43, Habib Virji wrote: > It can be local variable here. Done. https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:35: output.value = 'Clicked'; On 2014/10/20 13:16:43, Habib Virji wrote: > Can be just output = 'Clicked'; Done. https://codereview.chromium.org/594093002/diff/80001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:38: document.addEventListener('mouseup', function (evt) { On 2014/10/20 13:16:43, Habib Virji wrote: > nit: opening brackets in next line. Done.
paritosh.in@samsung.com changed reviewers: + keishi@chromium.org, tkent@chromium.org
PTAL.
PTAL.
https://codereview.chromium.org/594093002/diff/100001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html (right): https://codereview.chromium.org/594093002/diff/100001/LayoutTests/fast/forms/... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:42: mouseClickOn(input.offsetWidth - clearButtonOffset/2 - spinButtonOffset, center); The appearance of clear buttons can change in the future. You should do something like this. <script src="../resources/common.js"> <script> var clearButton = getElementByPseudoId(internals.oldestShadowRoot(input), "-webkit-clear-button"); clickElement(clearButton); </script>
Thanks Updated as suggested. PTAL. https://codereview.chromium.org/594093002/diff/100001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html (right): https://codereview.chromium.org/594093002/diff/100001/LayoutTests/fast/forms/... LayoutTests/fast/forms/date-multiple-fields/date-clearbutton-preventdefault-mousecapture-status.html:42: mouseClickOn(input.offsetWidth - clearButtonOffset/2 - spinButtonOffset, center); On 2014/10/21 10:25:45, keishi wrote: > The appearance of clear buttons can change in the future. You should do > something like this. > > <script src="../resources/common.js"> > <script> > var clearButton = getElementByPseudoId(internals.oldestShadowRoot(input), > "-webkit-clear-button"); > clickElement(clearButton); > </script> Done.
lgtm
The CQ bit was checked by paritosh.in@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/594093002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as 184076 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
