Drop down list closes on touch scroll/pinch zoom

Drop down list closes on touch scroll/pinch zoom

BUG=569092
Test=LayoutTests/fast/forms/select-popup/popup-menu-mouse-operations.html

Committed: https://crrev.com/3b47955d5b187f1897b781459d8c33b677d3a6de
Cr-Commit-Position: refs/heads/master@{#434330}

[sahel, 2016-11-21 18:22:51]

Patchset #1 (id:1) has been deleted

[sahel, 2016-11-21 18:32:05]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 18:33:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 20:04:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 20:04:01]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-11-21 20:08:43]

sahel@chromium.org changed reviewers:
+ bokan@chromium.org, jam@chromium.org

[sahel, 2016-11-21 20:08:43]

bokan@chromium.org: Please review changes in 
third_party/WebKit/Source/web/*

jam@chromium.org: Please review changes in 
third_party/WebKit/LayoutTests/fast/forms/select-popup/*

[bokan, 2016-11-21 20:14:35]

bokan@chromium.org changed reviewers:
- jam@chromium.org

[bokan, 2016-11-21 20:14:36]

-jam@ since anyone can stamp LayoutTests (and he's OOO)

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html
(right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html:158:
eventSender.gestureTapDown(300, 300);
We seem to be doing both a touch move and a gesture tap, what's the intention
here? If we want to make sure both close the popup, we should have separate
tests for each.

Also, the description says we close on pinch-zoom or gesture scroll. We don't
generate either here.

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:907: // Touch pinch zoom and
scroll must hide the popup.
Is this comment meant to be a TODO? If so add TODO(sahel): and link to a bug.

[sahel, 2016-11-21 20:41:44]

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html
(right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html:158:
eventSender.gestureTapDown(300, 300);
On 2016/11/21 20:14:36, bokan wrote:
> We seem to be doing both a touch move and a gesture tap, what's the intention
> here? If we want to make sure both close the popup, we should have separate
> tests for each.
> 
> Also, the description says we close on pinch-zoom or gesture scroll. We don't
> generate either here.

I can get rid of the touch move, here.
I used GestureTapDown instead of a GS/GP event, because in both WebViewImpl.cpp
and WebPagePopupImpl.cpp, handleGestureEvent is called with GestureTapDown in
case of a touch scroll/pinch zoom. handleGestureEvent gets called before
generating GS or GP events from touch info.

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:907: // Touch pinch zoom and
scroll must hide the popup.
On 2016/11/21 20:14:36, bokan wrote:
> Is this comment meant to be a TODO? If so add TODO(sahel): and link to a bug.

No, it's not a todo, it's the reason for adding this line of code. In case of a
touch scroll or pinch zoom, this function is called with GestureTapDown rather
than a GSB/GSU/GSE or GPB/GPU/GPE.
I can either get rid of the comment, or explain more, if it's not clear.

[bokan, 2016-11-21 21:09:17]

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:907: // Touch pinch zoom and
scroll must hide the popup.
On 2016/11/21 20:41:44, sahel wrote:
> On 2016/11/21 20:14:36, bokan wrote:
> > Is this comment meant to be a TODO? If so add TODO(sahel): and link to a
bug.
> 
> No, it's not a todo, it's the reason for adding this line of code. In case of
a
> touch scroll or pinch zoom, this function is called with GestureTapDown rather
> than a GSB/GSU/GSE or GPB/GPU/GPE.
> I can either get rid of the comment, or explain more, if it's not clear.

Where does the translation happen from GesturePinch/GestureScroll ->
GestureTapDown?

[sahel, 2016-11-22 14:31:06]

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:907: // Touch pinch zoom and
scroll must hide the popup.
On 2016/11/21 21:09:17, bokan wrote:
> On 2016/11/21 20:41:44, sahel wrote:
> > On 2016/11/21 20:14:36, bokan wrote:
> > > Is this comment meant to be a TODO? If so add TODO(sahel): and link to a
> bug.
> > 
> > No, it's not a todo, it's the reason for adding this line of code. In case
of
> a
> > touch scroll or pinch zoom, this function is called with GestureTapDown
rather
> > than a GSB/GSU/GSE or GPB/GPU/GPE.
> > I can either get rid of the comment, or explain more, if it's not clear.
> 
> Where does the translation happen from GesturePinch/GestureScroll ->
> GestureTapDown?

I believe there isn't any translations. In gesture_detector.cc, on ACTION_DOWN
the OnDown() function is called which generates the GTD. Then, in case of
ACTION_MOVE in gesture_detector.cc/ scale_gesture_detector.cc,
OnScroll()/OnScale() function is called to generate (GSB)GSU/(GPB)GPU.

[bokan, 2016-11-22 16:00:08]

lgtm % comments

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html
(right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html:158:
eventSender.gestureTapDown(300, 300);
On 2016/11/21 20:41:44, sahel wrote:
> On 2016/11/21 20:14:36, bokan wrote:
> > We seem to be doing both a touch move and a gesture tap, what's the
intention
> > here? If we want to make sure both close the popup, we should have separate
> > tests for each.
> > 
> > Also, the description says we close on pinch-zoom or gesture scroll. We
don't
> > generate either here.
> 
> I can get rid of the touch move, here.
> I used GestureTapDown instead of a GS/GP event, because in both
WebViewImpl.cpp
> and WebPagePopupImpl.cpp, handleGestureEvent is called with GestureTapDown in
> case of a touch scroll/pinch zoom. handleGestureEvent gets called before
> generating GS or GP events from touch info.

Ok, just get rid of everything but the GestureTapDown then.

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:907: // Touch pinch zoom and
scroll must hide the popup.
On 2016/11/22 14:31:06, sahel wrote:
> On 2016/11/21 21:09:17, bokan wrote:
> > On 2016/11/21 20:41:44, sahel wrote:
> > > On 2016/11/21 20:14:36, bokan wrote:
> > > > Is this comment meant to be a TODO? If so add TODO(sahel): and link to a
> > bug.
> > > 
> > > No, it's not a todo, it's the reason for adding this line of code. In case
> of
> > a
> > > touch scroll or pinch zoom, this function is called with GestureTapDown
> rather
> > > than a GSB/GSU/GSE or GPB/GPU/GPE.
> > > I can either get rid of the comment, or explain more, if it's not clear.
> > 
> > Where does the translation happen from GesturePinch/GestureScroll ->
> > GestureTapDown?
> 
> I believe there isn't any translations. In gesture_detector.cc, on ACTION_DOWN
> the OnDown() function is called which generates the GTD. Then, in case of
> ACTION_MOVE in gesture_detector.cc/ scale_gesture_detector.cc,
> OnScroll()/OnScale() function is called to generate (GSB)GSU/(GPB)GPU.

Ah, ok thanks. I didn't know that. (Please add a comment since it's not obvious
that's what happens).

[sahel, 2016-11-22 20:35:08]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 20:35:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 23:33:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:33:19]

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_...)

[sahel, 2016-11-23 21:45:17]

The CQ bit was checked by sahel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 21:46:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 23:47:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 23:47:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[sahel, 2016-11-24 15:29:25]

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html
(right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html:158:
eventSender.gestureTapDown(300, 300);
On 2016/11/22 16:00:08, bokan wrote:
> On 2016/11/21 20:41:44, sahel wrote:
> > On 2016/11/21 20:14:36, bokan wrote:
> > > We seem to be doing both a touch move and a gesture tap, what's the
> intention
> > > here? If we want to make sure both close the popup, we should have
separate
> > > tests for each.
> > > 
> > > Also, the description says we close on pinch-zoom or gesture scroll. We
> don't
> > > generate either here.
> > 
> > I can get rid of the touch move, here.
> > I used GestureTapDown instead of a GS/GP event, because in both
> WebViewImpl.cpp
> > and WebPagePopupImpl.cpp, handleGestureEvent is called with GestureTapDown
in
> > case of a touch scroll/pinch zoom. handleGestureEvent gets called before
> > generating GS or GP events from touch info.
> 
> Ok, just get rid of everything but the GestureTapDown then.

Done.

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2517253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:907: // Touch pinch zoom and
scroll must hide the popup.
On 2016/11/22 16:00:08, bokan wrote:
> On 2016/11/22 14:31:06, sahel wrote:
> > On 2016/11/21 21:09:17, bokan wrote:
> > > On 2016/11/21 20:41:44, sahel wrote:
> > > > On 2016/11/21 20:14:36, bokan wrote:
> > > > > Is this comment meant to be a TODO? If so add TODO(sahel): and link to
a
> > > bug.
> > > > 
> > > > No, it's not a todo, it's the reason for adding this line of code. In
case
> > of
> > > a
> > > > touch scroll or pinch zoom, this function is called with GestureTapDown
> > rather
> > > > than a GSB/GSU/GSE or GPB/GPU/GPE.
> > > > I can either get rid of the comment, or explain more, if it's not clear.
> > > 
> > > Where does the translation happen from GesturePinch/GestureScroll ->
> > > GestureTapDown?
> > 
> > I believe there isn't any translations. In gesture_detector.cc, on
ACTION_DOWN
> > the OnDown() function is called which generates the GTD. Then, in case of
> > ACTION_MOVE in gesture_detector.cc/ scale_gesture_detector.cc,
> > OnScroll()/OnScale() function is called to generate (GSB)GSU/(GPB)GPU.
> 
> Ah, ok thanks. I didn't know that. (Please add a comment since it's not
obvious
> that's what happens).

Done.

[sahel, 2016-11-24 15:29:33]

The CQ bit was checked by sahel@chromium.org

[sahel, 2016-11-24 15:29:34]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2517253002/#ps40001
(title: "review comments addressed.")

[commit-bot: I haz the power, 2016-11-24 15:29:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 15:33:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480001373738540,
"parent_rev": "09e14aecb7faa3c4f849be8630d9da981c040a7e", "commit_rev":
"e39bdbcb05d8e4678911f0622833e87773b74a1a"}

[commit-bot: I haz the power, 2016-11-24 15:34:53]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-11-24 15:38:10]

Description was changed from

==========
Drop down list closes on touch scroll/pinch zoom

BUG=569092
Test=LayoutTests/fast/forms/select-popup/popup-menu-mouse-operations.html
==========

to

==========
Drop down list closes on touch scroll/pinch zoom

BUG=569092
Test=LayoutTests/fast/forms/select-popup/popup-menu-mouse-operations.html

Committed: https://crrev.com/3b47955d5b187f1897b781459d8c33b677d3a6de
Cr-Commit-Position: refs/heads/master@{#434330}
==========

[commit-bot: I haz the power, 2016-11-24 15:38:11]

Patchset 2 (id:??) landed as
https://crrev.com/3b47955d5b187f1897b781459d8c33b677d3a6de
Cr-Commit-Position: refs/heads/master@{#434330}

[sahel, 2016-12-07 17:18:40]

Patchset #3 (id:60001) has been deleted