Issue 705733002: Make sure that selection is cleared after tapping.

Issue 705733002: Make sure that selection is cleared after tapping. (Closed)

Created:
6 years, 1 month ago by Miyoung Shin(g)

Modified:
6 years, 1 month ago

Reviewers:
Rick Byers

CC:
blink-reviews, blink-reviews-events_chromium.org, dglazkov+blink, eae+blinkwatch

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Project:
blink

Visibility:
Public.

More Reviews

Description

Make sure that selection is cleared after tapping. The problem is that Selection is not getting cleared after tapping of gesture. This was affected by https://codereview.chromium.org/507533002, because initialization for m_selectionInitiationState was skipped in case of gesture. This patch makes m_selectionInitiationState be initialized after dispatching mousedown event for gesture tapping. BUG=429967 R=rbyers@chromium.org TEST=LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185410

Patch Set 1 #

Total comments: 6

Patch Set 2 : #

Total comments: 1

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+34 lines, -1 line)			Patch
A	LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html	View	1	1 chunk	+19 lines, -0 lines	1 comment	Download
A	LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range-expected.txt	View	1	1 chunk	+11 lines, -0 lines	0 comments	Download
M	Source/core/page/EventHandler.cpp	View	1	1 chunk	+4 lines, -1 line	0 comments	Download

Messages

Total messages: 14 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Miyoung Shin(g)

rbyers@ PTAL. I missed the initialization of m_selectionInitiationState for gesture's tapping.

6 years, 1 month ago (2014-11-05 09:29:19 UTC) #1

Rick Byers

I'm very sorry for the long delay. Between travel for TPAC and BlinkOn I fell ...

6 years, 1 month ago (2014-11-13 20:09:01 UTC) #3

I'm very sorry for the long delay.  Between travel for TPAC and BlinkOn I fell
completely behind in e-mail / code reviews.

The change seems reasonable, but I've got some suggestions for improving the
test.

https://codereview.chromium.org/705733002/diff/1/LayoutTests/fast/events/touc...
File
LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html
(right):

https://codereview.chromium.org/705733002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:3:
<body>
nit: omit <body> (it's unnecessary and you have no matching </body> anyway).

https://codereview.chromium.org/705733002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:5:
<p id="description"></p>
nit: you can also omit this <p> and the blow console <div> - js-test.js will add
them for you.

https://codereview.chromium.org/705733002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:11:
var reselt;
note this is mis-spelled.

https://codereview.chromium.org/705733002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:23:
eventSender.mouseUp();
Could this test be simpler by setting the selection explicitly in JavaScript
instead of simulating mouse events?  This is OK too, but it might be slightly
easier for you to maintain.

Also please test that the selection rangeCount is actually >0 BEFORE sending the
tap.  Otherwise a simple test (or product) bug that resulted in a total lack of
selection would look like a perfectly valid 'SUCCESS':-(

https://codereview.chromium.org/705733002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:25:
eventSender.gestureTap(startX + dragme.offsetWidth / 2, startY +
dragme.offsetHeight / 2);
we're trying to start to enforce (with ASSERTs) that events come in valid
streams (eg. see http://crbug.com/345372).  Tap is only valid after a
GestureTapDown and GestureShowPress, so please send those first (see other tests
in this directory as an example).

https://codereview.chromium.org/705733002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:27:
result = window.getSelection().rangeCount > 0 ? 'FAILED' : 'SUCCESS';
no need for the temporary 'result'.  Just do a
shouldBeGreaterThan(window.getSelection()...).

Rick Byers

Also please update the issue subject and first line of the description to be a ...

6 years, 1 month ago (2014-11-13 20:10:23 UTC) #4

Miyoung Shin(g)

rbyer@, thank you for your kindly review. I've uploaded the patch to improve the test ...

6 years, 1 month ago (2014-11-14 10:01:28 UTC) #6

Rick Byers

LGTM https://codereview.chromium.org/705733002/diff/40001/LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/705733002/diff/40001/LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html#newcode17 LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:17: shouldBeZero('window.getSelection().rangeCount'); Nice simple test now, thank you for ...

6 years, 1 month ago (2014-11-14 21:48:41 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/705733002/40001

6 years, 1 month ago (2014-11-14 21:50:31 UTC) #9

commit-bot: I haz the power

Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/30972)

6 years, 1 month ago (2014-11-14 23:12:39 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/705733002/40001

6 years, 1 month ago (2014-11-16 14:07:27 UTC) #13

commit-bot: I haz the power

6 years, 1 month ago (2014-11-16 15:15:03 UTC) #14

Message was sent while issue was closed.

Committed patchset #2 (id:40001) as 185410

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages