Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(16)

Issue 144713007: Spinner elements not to dispatch change event on hover (Closed)

Created:
6 years, 10 months ago by Habib Virji
Modified:
6 years, 10 months ago
Reviewers:
tkent, keishi
CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Spinner elements not to dispatch change event on hover Change event get triggered on hover, due to releaseCapture called from defaultEventHandler. Also m_capture is not reset if mouse is not moved from spinElement, resulting in no change event being called. Dispatch change event from inside mouseUp event, to avoid above issues. R=tkent BUG=155747 TEST=Updated number-spinbutton-change event, to test when mouseMove is called no dispatchEvent is triggered and also if values are increased for second time, both input and change event are triggered.

Patch Set 1 #

Total comments: 1

Patch Set 2 : SpinButton mouse event trigger #

Patch Set 3 : A variable to keep track of mouse event #

Patch Set 4 : Test scripts update #

Total comments: 1

Patch Set 5 : Set change event details before dispatching event #

Total comments: 2

Patch Set 6 : SpinButton change event trigger #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -5 lines) Patch
M LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger.html View 1 2 3 1 chunk +12 lines, -0 lines 0 comments Download
M LayoutTests/fast/forms/number/number-spinbutton-changeevent-trigger-expected.txt View 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/html/forms/TextFieldInputType.cpp View 1 2 3 4 5 2 chunks +1 line, -2 lines 0 comments Download
M Source/core/html/shadow/SpinButtonElement.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/shadow/SpinButtonElement.cpp View 1 2 3 4 5 3 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
Habib Virji
The patch proposes to move dispatchChangeEvent inside mouseUp event due to releaseCapture called from multiple ...
6 years, 10 months ago (2014-01-28 17:48:45 UTC) #1
tkent
https://codereview.chromium.org/144713007/diff/1/Source/core/html/shadow/SpinButtonElement.cpp File Source/core/html/shadow/SpinButtonElement.cpp (left): https://codereview.chromium.org/144713007/diff/1/Source/core/html/shadow/SpinButtonElement.cpp#oldcode202 Source/core/html/shadow/SpinButtonElement.cpp:202: m_spinButtonOwner->spinButtonDidReleaseMouseCapture(); Calling spinButtonDidReleaseMouseCapture in releaseCapture is correct though we ...
6 years, 10 months ago (2014-01-29 00:16:35 UTC) #2
Habib Virji
Updated code to move back dispatch event inside releaseCapture. - Reason for checking indeterminate was ...
6 years, 10 months ago (2014-01-29 13:51:17 UTC) #3
tkent
On 2014/01/29 13:51:17, habib.virji wrote: > Updated code to move back dispatch event inside releaseCapture. ...
6 years, 10 months ago (2014-01-30 05:36:05 UTC) #4
Habib Virji
On 2014/01/30 05:36:05, tkent wrote: > On 2014/01/29 13:51:17, habib.virji wrote: > > Updated code ...
6 years, 10 months ago (2014-01-30 09:47:59 UTC) #5
Habib Virji
Based on the above comment, I have tried to look in m_textAsOfLastFormControlChangeEvent state. It appears ...
6 years, 10 months ago (2014-02-03 13:45:51 UTC) #6
tkent
https://codereview.chromium.org/144713007/diff/60002/Source/core/html/shadow/SpinButtonElement.cpp File Source/core/html/shadow/SpinButtonElement.cpp (right): https://codereview.chromium.org/144713007/diff/60002/Source/core/html/shadow/SpinButtonElement.cpp#newcode137 Source/core/html/shadow/SpinButtonElement.cpp:137: m_mouseEvent = true; This approach doesn't work for hovering ...
6 years, 10 months ago (2014-02-04 04:57:33 UTC) #7
tkent
On 2014/02/03 13:45:51, habib.virji wrote: > It appears m_valueIfDirty and m_textAsOfLastFormControlChangeEvent are never > equal ...
6 years, 10 months ago (2014-02-04 04:59:07 UTC) #8
Habib Virji
On 2014/02/04 04:59:07, tkent wrote: > On 2014/02/03 13:45:51, habib.virji wrote: > > It appears ...
6 years, 10 months ago (2014-02-04 11:07:18 UTC) #9
Habib Virji
MouseUp to be more precise is useful when detach is called, especially in case of ...
6 years, 10 months ago (2014-02-04 16:38:21 UTC) #10
tkent
The patch still don't fix the bug. 1. A user press the mouse button inside ...
6 years, 10 months ago (2014-02-06 07:32:12 UTC) #11
Habib Virji
Tried replicating, but I see change event being dispatch only when I release mouseUp button. ...
6 years, 10 months ago (2014-02-06 17:38:18 UTC) #12
tkent
https://codereview.chromium.org/144713007/diff/130001/Source/core/html/HTMLTextFormControlElement.cpp File Source/core/html/HTMLTextFormControlElement.cpp (right): https://codereview.chromium.org/144713007/diff/130001/Source/core/html/HTMLTextFormControlElement.cpp#newcode188 Source/core/html/HTMLTextFormControlElement.cpp:188: setTextAsOfLastFormControlChangeEvent(value()); This change has no effect. We need to ...
6 years, 10 months ago (2014-02-07 08:58:17 UTC) #13
Habib Virji
On 2014/02/07 08:58:17, tkent wrote: > https://codereview.chromium.org/144713007/diff/130001/Source/core/html/HTMLTextFormControlElement.cpp > File Source/core/html/HTMLTextFormControlElement.cpp (right): > > https://codereview.chromium.org/144713007/diff/130001/Source/core/html/HTMLTextFormControlElement.cpp#newcode188 > ...
6 years, 10 months ago (2014-02-07 16:28:49 UTC) #14
Habib Virji
1. TextFieldInputType.cpp: Previously value for m_textAsOfLastFormControlChangeEvent was set when input is not in focus. Value ...
6 years, 10 months ago (2014-02-07 16:33:36 UTC) #15
Habib Virji
On 2014/02/07 16:33:36, habib.virji wrote: > 1. TextFieldInputType.cpp: Previously value for > m_textAsOfLastFormControlChangeEvent was set ...
6 years, 10 months ago (2014-02-11 14:08:26 UTC) #16
tkent
Patch Set 6 still doesn't fix the following scenario. Why don't you test it? > ...
6 years, 10 months ago (2014-02-12 07:30:46 UTC) #17
tkent
> I don't expect you can fix this issue soon. I'll revert r165300 and r165455. ...
6 years, 10 months ago (2014-02-12 08:03:29 UTC) #18
Habib Virji
6 years, 10 months ago (2014-02-12 20:17:35 UTC) #19
Message was sent while issue was closed.
On 2014/02/12 07:30:46, tkent wrote:
> Patch Set 6 still doesn't fix the following scenario.  Why don't you test it?
> > 1. A user press the mouse button inside of an <input type=number>, but not
on
> the spin button
> > 2. He moves the pointer on the spin button
> > 3. He releases the mouse button.
> 
> I don't expect you can fix this issue soon.  I'll revert r165300 and r165455.

Apology, I did try always in content shell and chrome in Linux platform before
submitting  and it worked for me.

Powered by Google App Engine
This is Rietveld 408576698