Bug 33364 – [Chromium] Should not select a word on right-click in editable text

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kent Tamura]

[Chromium] Should not select a word on right-click

[Kent Tamura]

Created attachment 46112 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46112 [details] without any errors.

[Eric Seidel (no email)]

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.

[Tony Chang]

(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?

[Kent Tamura]

(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.

[Tony Chang]

(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.

[Kent Tamura]

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

[Kent Tamura]

Created attachment 46551 [details]
Proposed patch (rev.2)

[Kent Tamura]

(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.

[WebKit Review Bot]

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

[Kent Tamura]

Created attachment 46552 [details]
Proposed patch (rev.3)

[Darin Fisher (:fishd, Google)]

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?

[Kent Tamura]

Created attachment 46650 [details]
Proposed patch (rev.4)

[Kent Tamura]

> > +    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.

[Kent Tamura]

Committed r54212: <http://trac.webkit.org/changeset/54212>