Bug 33364 - [Chromium] Should not select a word on right-click in editable text
Summary: [Chromium] Should not select a word on right-click in editable text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows XP
: P2 Normal
Assignee: Nobody
URL: http://crbug.com/8841
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-07 22:12 PST by Kent Tamura
Modified: 2010-02-02 00:25 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.25 KB, patch)
2010-01-07 22:18 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.2) (6.37 KB, patch)
2010-01-14 02:28 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.3) (6.36 KB, patch)
2010-01-14 02:32 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Proposed patch (rev.4) (6.23 KB, patch)
2010-01-15 00:55 PST, Kent Tamura
fishd: review+
tkent: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2010-01-07 22:12:54 PST
[Chromium] Should not select a word on right-click
Comment 1 Kent Tamura 2010-01-07 22:18:12 PST
Created attachment 46112 [details]
Patch
Comment 2 WebKit Review Bot 2010-01-07 22:20:19 PST
style-queue ran check-webkit-style on attachment 46112 [details] without any errors.
Comment 3 Eric Seidel (no email) 2010-01-08 12:40:32 PST
I could have sworn we already had code around this?  I could have sworn several other bugs relating to this have already gone by.

A quick search I found bug 20363 and bug 23351 which look related, but are not the ones I was thinking of.
Comment 4 Tony Chang 2010-01-10 00:32:37 PST
(In reply to comment #3)
> I could have sworn we already had code around this?  I could have sworn several
> other bugs relating to this have already gone by.
> 
> A quick search I found bug 20363 and bug 23351 which look related, but are not
> the ones I was thinking of.

Eric is thinking of bug 15279.  This patch is for editable text areas.

Does this still allow you to correct misspelled words if they are not selected?
Comment 5 Kent Tamura 2010-01-11 04:22:33 PST
(In reply to comment #4)
> (In reply to comment #3)
> > I could have sworn we already had code around this?  I could have sworn several
> > other bugs relating to this have already gone by.
> > 
> > A quick search I found bug 20363 and bug 23351 which look related, but are not
> > the ones I was thinking of.
> 
> Eric is thinking of bug 15279.  This patch is for editable text areas.

Yes, this patch is for editable.
Bug#20363 was a fix for a curious behavior for non-editable, Bug#23351 is a follow-up of Bug#15279, and Bug#15279 disabled right-click-selection for non-editable. Chromium still have right-click-selection for editable.

> Does this still allow you to correct misspelled words if they are not selected?

No. With this patch, the context menu shows spell corrections only if there is already selected text.  I'm not sure we can replace a non-selected word by the spell checker. I'll check it later.
Comment 6 Tony Chang 2010-01-11 17:16:01 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Does this still allow you to correct misspelled words if they are not selected?
> 
> No. With this patch, the context menu shows spell corrections only if there is
> already selected text.  I'm not sure we can replace a non-selected word by the
> spell checker. I'll check it later.

That's what I was afraid of.  I think this will make it very hard to correct misspelled words.  Ideally, I think we want to not select the word but still provide spell corrections.
Comment 7 Kent Tamura 2010-01-11 22:27:07 PST
I confirmed the current spell correction code assumed a misspelled word was selected.

ExecuteItemCommand() in render_view_context_menu.cc:
    case IDC_SPELLCHECK_SUGGESTION_0:
    case IDC_SPELLCHECK_SUGGESTION_1:
    case IDC_SPELLCHECK_SUGGESTION_2:
    case IDC_SPELLCHECK_SUGGESTION_3:
    case IDC_SPELLCHECK_SUGGESTION_4:
      source_tab_contents_->render_view_host()->Replace(
          params_.dictionary_suggestions[id - IDC_SPELLCHECK_SUGGESTION_0]);
      break;

RenderView::OnReplace() is simply:
  webview()->focusedFrame()->replaceSelection(text);


However it seems not hard to change it so that replacing non-selected misspelled word.
1. selectMisspelledWord() in ContextMenuClientImpl.cpp returns a word without changing selection if there is no selection.
2. RenderView::OnReplace() replaces a word around the cursor with the specified string if there is no existing selection. (If ViewMsg_Replace() is used by other code, we should introduce new ViewMsg for this.)
Comment 8 Kent Tamura 2010-01-14 02:28:12 PST
Created attachment 46551 [details]
Proposed patch (rev.2)
Comment 9 Kent Tamura 2010-01-14 02:30:33 PST
(In reply to comment #8)
> Created an attachment (id=46551) [details]
> Proposed patch (rev.2)

Note: If this patch is landed, spell-checking without selection won't work.  We'll need a simple change for Chromium and fortunately I'm going to be a WebKit sheriff next week.
Comment 10 WebKit Review Bot 2010-01-14 02:30:42 PST
Attachment 46551 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebFrameImpl.cpp:952:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1
Comment 11 Kent Tamura 2010-01-14 02:32:23 PST
Created attachment 46552 [details]
Proposed patch (rev.3)
Comment 12 Darin Fisher (:fishd, Google) 2010-01-14 21:51:41 PST
Comment on attachment 46552 [details]
Proposed patch (rev.3)


> +++ b/WebKit/chromium/public/WebFrame.h
> @@ -327,6 +327,9 @@ public:
>  
>      // Replaces the selection with the given text.
>      virtual void replaceSelection(const WebString& text) = 0;
> +    // Replaces the selection with the given text or replaces a word around the

^^^ please insert a new line above the comment so that there is a new line
between functions.


> +    virtual void replaceWord(const WebString& text) = 0;

I think you should give this function a more descriptive name.  I'm also
concerned that it has the side-effect of altering the selection.  Perhaps
for the WebKit API we should really break this up.  We already have a
hasSelection() method, so it seems like you could just add a method
that causes selection to be set at the word around the caret.  Like this:

  virtual void selectWordAroundCaret() = 0;


> +++ b/WebKit/chromium/src/WebFrameImpl.cpp
...
> +void WebFrameImpl::replaceWord(const WebString& text)
> +{
> +    SelectionController* controller = frame()->selection();
> +    // Should have a caret or a ranged selection because the corrected word was
> +    // based on them. See selectMisspelledWord() in ContextMenuClientImpl.cpp.
> +    ASSERT(!controller->isNone());

It seems like there could be a race here.  Do we allow events to be processed
between the time selectMisspelledWord is called and the time that replaceWord
is called?
Comment 13 Kent Tamura 2010-01-15 00:55:58 PST
Created attachment 46650 [details]
Proposed patch (rev.4)
Comment 14 Kent Tamura 2010-01-15 00:57:59 PST
> > +    virtual void replaceWord(const WebString& text) = 0;
> 
> I think you should give this function a more descriptive name.  I'm also
> concerned that it has the side-effect of altering the selection.  Perhaps
> for the WebKit API we should really break this up.  We already have a
> hasSelection() method, so it seems like you could just add a method
> that causes selection to be set at the word around the caret.  Like this:
> 
>   virtual void selectWordAroundCaret() = 0;

ok, I renamed so.

> > +    // Should have a caret or a ranged selection because the corrected word was
> > +    // based on them. See selectMisspelledWord() in ContextMenuClientImpl.cpp.
> > +    ASSERT(!controller->isNone());
> 
> It seems like there could be a race here.  Do we allow events to be processed
> between the time selectMisspelledWord is called and the time that replaceWord
> is called?

I confirmed this assertion was invalid.  JavaScript code can change the selection during a context menu is opening.  Removed the assertion.
Comment 15 Kent Tamura 2010-02-02 00:25:46 PST
Committed r54212: <http://trac.webkit.org/changeset/54212>