Fix the problem to keep the selection when clicking the substring out of range

Fix the problem to keep the selection when clicking the substring out of range

The problem is that it's not consistent to keep the selection when clicking the
substiring in/out of range. We need the additional condition missing
from http://crrev.com/507533002

BUG=32865, 438102
R=yosin@chromium.org
R=rbyers@chromium.org
TEST=fast/forms/setrangetext-out-of-range.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186550

[Miyoung Shin(g), 2014-12-02 06:00:06]

yosin@, rbyers@, PTAL

[Rick Byers, 2014-12-02 14:49:39]

I'm not enough of an expert on blink editing / selection to approve this
(there's obviously some subtle interaction possible here), but I'll rubber-stamp
as an OWNER if one of the editing experts approves it.

yosin@ are you a good reviewer for this?

https://codereview.chromium.org/764313004/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/764313004/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:563: } else {
style nit: combine into one line 'else if {'

[yosin_UTC9, 2014-12-03 01:07:00]

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
File LayoutTests/fast/forms/setrangetext-out-of-range.html (right):

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
LayoutTests/fast/forms/setrangetext-out-of-range.html:5: window.onload =
function() {
nit: Please add |jsTestIsAsync = true| and call |TestRunner.notifyDone()| at end
of test. TestRunner finalized test by "DOMContentLoaded" which is dispatched
"load" event.

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
LayoutTests/fast/forms/setrangetext-out-of-range.html:16:
eventSender.mouseMoveTo(tx, ty);
nit: Please use four spaces indentation.

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
LayoutTests/fast/forms/setrangetext-out-of-range.html:36: <input type="text"
value="valueeeeeeeeeeeeeeeeeeeeeeeeeeee" id="textfield"></input>
nit: It is better to use different characters rather than repeated "e" for ease
of identifying where value is truncated.

https://codereview.chromium.org/764313004/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/764313004/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:564: if (m_selectionInitiationState !=
ExtendedSelection)
It seems condition is opposite or should we set |m_selectionInitializationState|
to |ExtendedSelection|?

[Miyoung Shin(g), 2014-12-03 03:39:10]

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
File LayoutTests/fast/forms/setrangetext-out-of-range.html (right):

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
LayoutTests/fast/forms/setrangetext-out-of-range.html:5: window.onload =
function() {
On 2014/12/03 01:07:00, Yosi_UTC9 wrote:
> nit: Please add |jsTestIsAsync = true| and call |TestRunner.notifyDone()| at
end
> of test. TestRunner finalized test by "DOMContentLoaded" which is dispatched
> "load" event.

Ok, I will update to use |jsTestIsAsync = true| and |finishJSTest()|.

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
LayoutTests/fast/forms/setrangetext-out-of-range.html:16:
eventSender.mouseMoveTo(tx, ty);
On 2014/12/03 01:06:59, Yosi_UTC9 wrote:
> nit: Please use four spaces indentation.

OK. I will update

https://codereview.chromium.org/764313004/diff/1/LayoutTests/fast/forms/setra...
LayoutTests/fast/forms/setrangetext-out-of-range.html:36: <input type="text"
value="valueeeeeeeeeeeeeeeeeeeeeeeeeeee" id="textfield"></input>
On 2014/12/03 01:07:00, Yosi_UTC9 wrote:
> nit: It is better to use different characters rather than repeated "e" for
ease
> of identifying where value is truncated.

OK. I will update

https://codereview.chromium.org/764313004/diff/1/Source/core/page/EventHandle...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/764313004/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:563: } else {
On 2014/12/02 14:49:39, Rick Byers wrote:
> style nit: combine into one line 'else if {'

Ok. I will change

https://codereview.chromium.org/764313004/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:564: if (m_selectionInitiationState !=
ExtendedSelection)
On 2014/12/03 01:07:00, Yosi_UTC9 wrote:
> It seems condition is opposite or should we set
|m_selectionInitializationState|
> to |ExtendedSelection|?

This condition is to check if there is to update the some range of selection
during event handling. If |m_selectionInitializationState| is
|ExtendedSelection|, it means that the operation of the selection is happened by
JS. And we should keep the selection by JS.

[Miyoung Shin(g), 2014-12-04 06:53:28]

@yosin,
I've uploaded the patch, PTAL

[yosin_UTC9, 2014-12-04 08:19:03]

lgtm

[Rick Byers, 2014-12-04 16:30:56]

On 2014/12/04 08:19:03, Yosi_UTC9 wrote:
> lgtm

RubberStamp LGTM

[Miyoung Shin(g), 2014-12-04 22:06:08]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2014-12-04 22:06:58]

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

[commit-bot: I haz the power, 2014-12-04 23:41:51]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186550

[Donn Denman, 2014-12-09 00:59:37]

donnd@chromium.org changed reviewers:
+ donnd@chromium.org

[Donn Denman, 2014-12-09 00:59:37]

This change is causing a problem for for Contextual Search (a feature for Chrome
on Android).  See crbug.com/440086.  It looks like only one part of this change
is causing the problem for our feature.  What's the best way to resolve this
incompatibility?

https://codereview.chromium.org/764313004/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/764313004/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:563: } else if (m_selectionInitiationState !=
ExtendedSelection) {
This line causes a problem for Contextual Search (a feature for Chrome on
Android).

[Rick Byers, 2014-12-09 17:53:20]

On 2014/12/09 00:59:37, Donn Denman wrote:
> This change is causing a problem for for Contextual Search (a feature for
Chrome
> on Android).  See crbug.com/440086.  It looks like only one part of this
change
> is causing the problem for our feature.  What's the best way to resolve this
> incompatibility?
> 
>
https://codereview.chromium.org/764313004/diff/20001/Source/core/page/EventHa...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/764313004/diff/20001/Source/core/page/EventHa...
> Source/core/page/EventHandler.cpp:563: } else if (m_selectionInitiationState
!=
> ExtendedSelection) {
> This line causes a problem for Contextual Search (a feature for Chrome on
> Android).

If there's not a trivial fix, then perhaps we should revert this CL for now?

[Donn Denman, 2014-12-09 18:07:12]

On 2014/12/09 17:53:20, Rick Byers wrote:
> On 2014/12/09 00:59:37, Donn Denman wrote:
> > This change is causing a problem for for Contextual Search (a feature for
> Chrome
> > on Android).  See crbug.com/440086.  It looks like only one part of this
> change
> > is causing the problem for our feature.  What's the best way to resolve this
> > incompatibility?
> > 
> >
>
https://codereview.chromium.org/764313004/diff/20001/Source/core/page/EventHa...
> > File Source/core/page/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/764313004/diff/20001/Source/core/page/EventHa...
> > Source/core/page/EventHandler.cpp:563: } else if (m_selectionInitiationState
> !=
> > ExtendedSelection) {
> > This line causes a problem for Contextual Search (a feature for Chrome on
> > Android).
> 
> If there's not a trivial fix, then perhaps we should revert this CL for now?

Based on communication in the CS bug (crbug.com/440086) it looks like Miyoung is
trying to reproduce this and fix the incompatibility, so I'm willing to wait a
day or two before reverting.