Don't change selection if some texts were already selected....

Don't change selection if some texts were already selected.

This patch changes the behavior of right click for a textarea.
Before this patch, right click changes the selection like:

"chr**ome" => "*chrome*"
"c*hro*me" => "*chrome*"
"chr*ome  *" => "*chrome*  "
"* chrome *" => " *chrome* "
"*:chrome:*" => ":*chrome*:"
"*chrome browser*" => "*chrome browser*" (no change)

where characters enclosed by asterisks are selected.

After this patch, only the first change happens
(i.e., no texts were selected).

BUG=7297

[hamaji, 2009-04-22 23:42:05]

Hi,

I'd like ask you to review this patch, which changes the behavior of left click
for textarea. Though I believe this is a better behavior, I'm not 100% sure if
we should stop changing selection when some texts are already selected. If there
are something unclear, please ask me.

Thanks,

[sidchat (Google), 2009-04-23 00:15:29]

I understand what your patch does, but the description is a bit misleading to
me. Note that the patch affects right click behavior, not left click behavior,
as you have described in the patch, as far as I know. However, that is a minor
issue.

The major issue I have is that I am not sure we should behave the way the patch
is designed to make Chrome behave. It seems that, if you select a portion of a
word, and right click on it, it will treat *that* portion of the word as the
misspelled word, and give suggestions for *that* portion, which is a bit weird I
think, especially because the suggestions that come up will be odd. For example,
consider the word "Running". Now, if you select "Runn" portion of the word, and
right click on it, and replace it by a suggestion, it replaces it - which is a
bit weird.

I think before we land this patch, we have to get feedback from UI experts
whether this behavior is acceptable.

On 2009/04/22 23:42:05, hamaji wrote:
> Hi,
> 
> I'd like ask you to review this patch, which changes the behavior of left
click
> for textarea. Though I believe this is a better behavior, I'm not 100% sure if
> we should stop changing selection when some texts are already selected. If
there
> are something unclear, please ask me.
> 
> Thanks,

[hamaji, 2009-04-23 00:30:49]

Thanks for the comments!

On 2009/04/23 00:15:29, sidchat (Google) wrote:
> I understand what your patch does, but the description is a bit misleading to
> me. Note that the patch affects right click behavior, not left click behavior,
> as you have described in the patch, as far as I know. However, that is a minor
> issue.

Oops, embarrassing mistake... I fixed the description. I cannot fix my email
above, though :(

> 
> The major issue I have is that I am not sure we should behave the way the
patch
> is designed to make Chrome behave. It seems that, if you select a portion of a
> word, and right click on it, it will treat *that* portion of the word as the
> misspelled word, and give suggestions for *that* portion, which is a bit weird
I
> think, especially because the suggestions that come up will be odd. For
example,
> consider the word "Running". Now, if you select "Runn" portion of the word,
and
> right click on it, and replace it by a suggestion, it replaces it - which is a
> bit weird.
> 
> I think before we land this patch, we have to get feedback from UI experts
> whether this behavior is acceptable.

Ah, I see. I guess showing no spell correction for "Runn" case would be the
best? but I'm totally not sure. I agree that we need inputs from experts. So,
how can I contact UI experts?

Thanks,

> 
> On 2009/04/22 23:42:05, hamaji wrote:
> > Hi,
> > 
> > I'd like ask you to review this patch, which changes the behavior of left
> click
> > for textarea. Though I believe this is a better behavior, I'm not 100% sure
if
> > we should stop changing selection when some texts are already selected. If
> there
> > are something unclear, please ask me.
> > 
> > Thanks,

[M-A Ruel, 2009-04-23 00:39:37]

I don't know particularly this code so I have no comment. I just don't know why
you sent this review to me (and to two persons for a seemingly trivial change
like this). Am I on the svn blame list?

rubberstamp lgtm

http://codereview.chromium.org/92047/diff/2001/3001
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/92047/diff/2001/3001#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Update it to 2009

[hamaji, 2009-04-23 17:22:26]

Yes, I've seen you by svn blame. Thanks for your review.

http://codereview.chromium.org/92047/diff/2001/3001
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/92047/diff/2001/3001#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/04/23 00:39:37, M-A wrote:
> Update it to 2009

Done.

[sidchat (Google), 2009-04-23 17:39:27]

> Ah, I see. I guess showing no spell correction for "Runn" case would > be the
best? 
Well, technically yes, but that means we now have to bring in more complications
- determine whether a word is partially selected or selected as a whole - and
then trigger a suggestion based on that.

But it is also true that selection of partial words is also done for cut/paste
etc, for which this patch is food.

Alright, so let us submit this patch for now. As such this will be rare case,
and even when suggestions for partial words do come up, it wont be critical.

So, LGTM from my side!

> but I'm totally not sure. I agree that we need inputs from experts. > So,
> how can I contact UI experts?
> 
> Thanks,
> 
> > 
> > On 2009/04/22 23:42:05, hamaji wrote:
> > > Hi,
> > > 
> > > I'd like ask you to review this patch, which changes the behavior of left
> > click
> > > for textarea. Though I believe this is a better behavior, I'm not 100%
sure
> if
> > > we should stop changing selection when some texts are already selected. If
> > there
> > > are something unclear, please ask me.
> > > 
> > > Thanks,

[hamaji, 2009-04-23 17:55:11]

On 2009/04/23 17:39:27, sidchat (Google) wrote:
> > Ah, I see. I guess showing no spell correction for "Runn" case would > be
the
> best? 
> Well, technically yes, but that means we now have to bring in more
complications
> - determine whether a word is partially selected or selected as a whole - and
> then trigger a suggestion based on that.
> 
> But it is also true that selection of partial words is also done for cut/paste
> etc, for which this patch is food.
> 
> Alright, so let us submit this patch for now. As such this will be rare case,
> and even when suggestions for partial words do come up, it wont be critical.
> 
> So, LGTM from my side!

Ah, thanks. Could you check this in? I don't have chromium account yet.

By the way, I've just found that this fixes another weird behavior: in the
"Runn" case, right click changes the selection to "Running", but the context
menu shows "Search Google for 'Runn'".

Thanks,

> 
> > but I'm totally not sure. I agree that we need inputs from experts. > So,
> > how can I contact UI experts?
> > 
> > Thanks,
> > 
> > > 
> > > On 2009/04/22 23:42:05, hamaji wrote:
> > > > Hi,
> > > > 
> > > > I'd like ask you to review this patch, which changes the behavior of
left
> > > click
> > > > for textarea. Though I believe this is a better behavior, I'm not 100%
> sure
> > if
> > > > we should stop changing selection when some texts are already selected.
If
> > > there
> > > > are something unclear, please ask me.
> > > > 
> > > > Thanks,