Bug 103520 – On Linux, should be able to get spelling suggestions without selecting the misspelled word

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Grzegorz Czajkowski]

WebCore assumes that a misspelled word has to be selected to get its suggestions (https://trac.webkit.org/browser/trunk/Source/WebCore/editing/Editor.cpp#L1758).
context-menu-suggestions.html just invokes contextClick method (in fact it's right click on the given position) to get the context menu items.
IMHO we should modify the tests and emit double click to select the word to retrieve its suggestions.

[Grzegorz Czajkowski]

Created attachment 176483 [details]
proposed patch

[Tony Chang]

Shouldn't the word get selected when right clicking?  I think in Chromium we only select the word when it has the red misspelling underline.  Hmm, not sure where that happens.

[Grzegorz Czajkowski]

(In reply to comment #2)
> Shouldn't the word get selected when right clicking?  I think in Chromium we only select the word when it has the red misspelling underline.  Hmm, not sure where that happens.

Thanks for your opinions. Yes, you're right. In Chromium, right click on the misspelled word makes the selection.

It looks like WebCore expects selection on misspelled word to retrieve suggestions so I'll check possibility to adapt this behaviour to EFL port.

[Grzegorz Czajkowski]

Comment on attachment 176483 [details]
proposed patch

Clearing the review flag regarding to Tony's comment.

[Grzegorz Czajkowski]

It seems that making selection for misspelled word it's common behaviour in WebKit.
Chromium is doing that at: https://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/ContextMenuClientImpl.cpp#L306
This test passes for Mac too, so probably they are doing the same.

How about moving this ports specific implementation to common place to influence other WebKit ports?

I am not sure where it should be implemented. We can consider to change ContextMenuController::populate() method somewhere around https://trac.webkit.org/browser/trunk/Source/WebCore/page/ContextMenuController.cpp#L912

[Tony Chang]

(In reply to comment #5)
> It seems that making selection for misspelled word it's common behaviour in WebKit.
> Chromium is doing that at: https://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/ContextMenuClientImpl.cpp#L306
> This test passes for Mac too, so probably they are doing the same.

Mac is different.  It always highlights the word under the mouse pointer when you right click, even for non editable text.  See the code in EventHandler::sendContextMenuEvent and the shouldSelectOnContextualMenuClick behavior boolean.

> How about moving this ports specific implementation to common place to influence other WebKit ports?
> 
> I am not sure where it should be implemented. We can consider to change ContextMenuController::populate() method somewhere around https://trac.webkit.org/browser/trunk/Source/WebCore/page/ContextMenuController.cpp#L912

Moving this logic to WebCore sounds OK, although it looks like this behavior may be platform specific (e.g., in gedit, if I turn on "Highlight misspelled words", right clicking on a misspelled word doesn't select the word).  If we move it to WebCore, it should probably also have a boolean added to EditingBehavior.

[Grzegorz Czajkowski]

(In reply to comment #6)
> (In reply to comment #5)
> > It seems that making selection for misspelled word it's common behaviour in WebKit.
> > Chromium is doing that at: https://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/ContextMenuClientImpl.cpp#L306
> > This test passes for Mac too, so probably they are doing the same.
> 
> Mac is different.  It always highlights the word under the mouse pointer when you right click, even for non editable text.  See the code in EventHandler::sendContextMenuEvent and the shouldSelectOnContextualMenuClick behavior boolean.

Right. Thanks for clarification.

> 
> > How about moving this ports specific implementation to common place to influence other WebKit ports?
> > 
> > I am not sure where it should be implemented. We can consider to change ContextMenuController::populate() method somewhere around https://trac.webkit.org/browser/trunk/Source/WebCore/page/ContextMenuController.cpp#L912
> 
> Moving this logic to WebCore sounds OK, although it looks like this behavior may be platform specific (e.g., in gedit, if I turn on "Highlight misspelled words", right clicking on a misspelled word doesn't select the word).  If we move it to WebCore, it should probably also have a boolean added to EditingBehavior.

I'm in a favour of this proposal. I'll try to implementing it in that way. Thanks.

[Grzegorz Czajkowski]

Created attachment 178211 [details]
proposed patch - right click makes selection on misspelled word

This patch is based on Chromium implementation. Tested for 
 - WebKi2-Gtk's MiniBrowser as it implements spelling and context menu features.
 - WebKit2-EFL layout tests.

TODO:
 - context-menu-suggestions.html is still failing for WebKit-GTK, I am investigating it
 - I am wondering whether this policy could be applied to all Unix WebKit ports
 - I am not able to test how it influences Chromium as this change might make duplication/conflict with their original code. I'd be nice if you could express your suggestions about it.

[Hajime Morrita]

Comment on attachment 178211 [details]
proposed patch - right click makes selection on misspelled word

View in context: https://bugs.webkit.org/attachment.cgi?id=178211&action=review

The basic approach looks good. I'm asking for some house keeping.

> Source/WebCore/editing/Editor.cpp:4
> + * Copyright (C) 2012 Samsung Electronics

I would hold back doing this unless the size of contribution is significant.

> Source/WebCore/editing/Editor.cpp:1691
> +bool Editor::isWordMisspelledAtCaret(Node* clickedNode) const

If the code is based on Chromium's implementation, Please file a bug and add FIXME on Chromium side to use this code there.

> Source/WebCore/page/EventHandler.cpp:2882
> +    bool selectOnCtxMenu = m_frame->editor()->behavior().shouldSelectOnContextualMenuClick()

Please don't use abbreviation like ctx.

> Source/WebCore/page/EventHandler.cpp:2886
> +    bool selectOnCtxMenuForMisspelledWord = m_frame->editor()->behavior().shouldSelectOnContextualMenuClickForMisspelledWord()

Ditto.

[Grzegorz Czajkowski]

Created attachment 178511 [details]
applied Morrita's suggestions

[Carlos Garcia Campos]

Comment on attachment 178511 [details]
applied Morrita's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=178511&action=review

> Source/WebCore/editing/Editor.cpp:1715
> +    int wordLength = word.length();

This could be moved after the return.

> Source/WebCore/editing/Editor.cpp:1724
> +    return misspellingLength == wordLength ? true : false;

I guess this could just be return misspellingLength == wordLength;

[Grzegorz Czajkowski]

Created attachment 178548 [details]
apply Carlos's suggestions

[Tony Chang]

Comment on attachment 178548 [details]
apply Carlos's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=178548&action=review

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:307
> +                // FIXME: WebCore selects the misspelled word on context menu, https://bugs.webkit.org/show_bug.cgi?id=104531.
>                  data.misspelledWord = selectMisspelledWord(defaultMenu, selectedFrame);

Is it OK for Chromium Linux to run the code in EventHandler to select the word and to run this code?

[Grzegorz Czajkowski]

(In reply to comment #13)
> (From update of attachment 178548 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178548&action=review
> 
> > Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:307
> > +                // FIXME: WebCore selects the misspelled word on context menu, https://bugs.webkit.org/show_bug.cgi?id=104531.
> >                  data.misspelledWord = selectMisspelledWord(defaultMenu, selectedFrame);
> 
> Is it OK for Chromium Linux to run the code in EventHandler to select the word and to run this code?

IMO it doesn't make sense to run this code once again as code in EventHandler will be applied for all Linux WebKit ports. But there are some differences between newly added code and the existing Chromium implementation like specific code for OS(DARWIN) etc. that I could build, run and test.

I created a bug for this issue (bug 104531) and leave FIXME message in the code.

[Ryosuke Niwa]

I’m not sure if this is something we want on all ports.

[Ryosuke Niwa]

This is not specific to GTK or EFL.

[Martin Robinson]

Grzegorz asked me to comment here about the situation on GTK+. GTK+ apps do not naturally highlight misspelled words when you right-click on them. On the other hand, I'd prefer to fix spelling correction in WebKit sooner and worry about small differences from native apps later, especially if this assumption baked deeply into WebCore.

[Ryosuke Niwa]

(In reply to comment #17)
> Grzegorz asked me to comment here about the situation on GTK+. GTK+ apps do not naturally highlight misspelled words when you right-click on them. On the other hand, I'd prefer to fix spelling correction in WebKit sooner and worry about small differences from native apps later, especially if this assumption baked deeply into WebCore.

I don't think it's baked into WebCore. I'm quite puzzled as to why this is needed for EFL port.

[Ryosuke Niwa]

I'm not convinced that this is a good idea. Passing a test is good only if the tested behavior is desirable. Given that native apps behave differently according to comments here, we should probably implement whatever native Linux apps do instead of trying to match what we implemented for OS X.

[Grzegorz Czajkowski]

(In reply to comment #18)
> (In reply to comment #17)
> > Grzegorz asked me to comment here about the situation on GTK+. GTK+ apps do not naturally highlight misspelled words when you right-click on them. On the other hand, I'd prefer to fix spelling correction in WebKit sooner and worry about small differences from native apps later, especially if this assumption baked deeply into WebCore.
> 
> I don't think it's baked into WebCore. I'm quite puzzled as to why this is needed for EFL port.

Currently, to get spelling suggestions user has to double click first (tried WebKit2-GTK with MiniBrowser, WebKit2-EFL's MiniBrowser lacks context menu feature).
WebCore expects selection to get spelling suggestions. see https://trac.webkit.org/browser/trunk/Source/WebCore/editing/Editor.cpp#L1759

[Ryosuke Niwa]

(In reply to comment #20)
>
> Currently, to get spelling suggestions user has to double click first (tried WebKit2-GTK with MiniBrowser, WebKit2-EFL's MiniBrowser lacks context menu feature).
> WebCore expects selection to get spelling suggestions. see https://trac.webkit.org/browser/trunk/Source/WebCore/editing/Editor.cpp#L1759

We can always change WebCore code. We shouldn't be deciding the UI behavior based on what WebCore currently does. That's backwards. We implement what user wants.

[Grzegorz Czajkowski]

(In reply to comment #21)
> (In reply to comment #20)
> >
> > Currently, to get spelling suggestions user has to double click first (tried WebKit2-GTK with MiniBrowser, WebKit2-EFL's MiniBrowser lacks context menu feature).
> > WebCore expects selection to get spelling suggestions. see https://trac.webkit.org/browser/trunk/Source/WebCore/editing/Editor.cpp#L1759
> 
> We can always change WebCore code. We shouldn't be deciding the UI behavior based on what WebCore currently does. That's backwards. We implement what user wants.

It definitely makes sense.
Initially, when I found the reason of failing the test, I based on WebCore behaviour to solve this issue, especially if Enlightenment doesn't provide own text editor (they just invoke gedit) so it was hard to find out how the EFL should behave.
I will try to allow spelling suggestion without selection for EFL and GTK+ WebKit ports. Thanks for your opinion.

[Grzegorz Czajkowski]

Created attachment 179721 [details]
new approach - allow spelling suggestions without selection

[Build Bot]

Comment on attachment 179721 [details]
new approach - allow spelling suggestions without selection 

Attachment 179721 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15364757

[Grzegorz Czajkowski]

Created attachment 179728 [details]
fix build break on Mac

[Build Bot]

Comment on attachment 179728 [details]
fix build break on Mac

Attachment 179728 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15374734

[Grzegorz Czajkowski]

Created attachment 179744 [details]
update WebCore.exp.in

[Ryosuke Niwa]

Comment on attachment 179744 [details]
update WebCore.exp.in

View in context: https://bugs.webkit.org/attachment.cgi?id=179744&action=review

> Source/WebCore/editing/EditingBehavior.h:67
> +#if PLATFORM(EFL) || PLATFORM(GTK)

It's probably better to say !PLATFORM(CHROMIUM) in accordance with the comment.
Or does Qt port not want this behavior?

> Source/WebCore/editing/Editor.cpp:1684
> +// Helper function to determine whether text is a single word.

We prefer having only "why" comments. This comment repeats the code. Please remove it.

> Source/WebCore/editing/Editor.cpp:1688
> +    return it && textBreakNext(it) == static_cast<int>(text.length());

I don't think you can call wordBreakIterator like this on a fragment of text. There are cases where wordBreakIterator needs more context than the word itself to determine whether something is a word or not (e.g. CJK).

> Source/WebCore/editing/Editor.cpp:1693
> +    // Don't do anything if the spelling for the node is forbidden.

Again, this comment says what the code below already tells us. Please remove it.

> Source/WebCore/editing/Editor.cpp:1708
> +    // Get selected text to check for multiple word selection.
> +    String selectedString = selectedText().stripWhiteSpace();
> +
> +    if (!selectedString.isEmpty()) {
> +        // Don't provide suggestions for multiple words.
> +        if (!isASingleWord(selectedString))
> +            return String();
> +    }

You should use nextWordPosition and so forth to check whether there are more than one word included in the selection.
r- because of this.

> Source/WebCore/editing/Editor.cpp:1710
> +    // Get the word arround the caret.

Instead of adding a comment like this, please extract the code that extracts the word around the caret as a function.

[Ryosuke Niwa]

Comment on attachment 179744 [details]
update WebCore.exp.in

View in context: https://bugs.webkit.org/attachment.cgi?id=179744&action=review

> Source/WebCore/page/ContextMenuController.cpp:348
> +            frame->selection()->modify(FrameSelection::AlterationMove, DirectionBackward, WordGranularity, UserTriggered);
> +            frame->editor()->deleteWithDirection(DirectionForward, WordGranularity, false, true);
> +            TypingCommand::insertText(frame->document(), item->title(), 0);

We need to call shouldInsertText here as we do it in non-Linux case. We also should be using ReplaceSelectionCommand instead of deleteWordWithDirection and insertText.
Finally, I'm not certain why we don't want to call revealSelection here. r- because of this.

[Grzegorz Czajkowski]

Comment on attachment 179744 [details]
update WebCore.exp.in

View in context: https://bugs.webkit.org/attachment.cgi?id=179744&action=review

>> Source/WebCore/editing/EditingBehavior.h:67
>> +#if PLATFORM(EFL) || PLATFORM(GTK)
> 
> It's probably better to say !PLATFORM(CHROMIUM) in accordance with the comment.
> Or does Qt port not want this behavior?

Right. Applied for non Chromium ports.

>> Source/WebCore/editing/Editor.cpp:1684
>> +// Helper function to determine whether text is a single word.
> 
> We prefer having only "why" comments. This comment repeats the code. Please remove it.

OK, this function is no longer needed so it was removed.

>> Source/WebCore/editing/Editor.cpp:1688
>> +    return it && textBreakNext(it) == static_cast<int>(text.length());
> 
> I don't think you can call wordBreakIterator like this on a fragment of text. There are cases where wordBreakIterator needs more context than the word itself to determine whether something is a word or not (e.g. CJK).

Thanks for the hint. I was trying to use nextWordPostion/rightWordPosition as you mentioned but in this case I had to take of some conditions to meet the requirements. I propose to check the range between the word under the caret (using expandUsingGranularity) and real selection. If they are different it means that the selection was made on multiple words. However, we allow to get suggestion when the misspelled word is selected.
expandUsingGranularity and freiends are using nextWordPostion as so on.

>> Source/WebCore/editing/Editor.cpp:1693
>> +    // Don't do anything if the spelling for the node is forbidden.
> 
> Again, this comment says what the code below already tells us. Please remove it.

Removed.

>> Source/WebCore/editing/Editor.cpp:1708
>> +    }
> 
> You should use nextWordPosition and so forth to check whether there are more than one word included in the selection.
> r- because of this.

Did it through expandUsingGranularity to do not take care of all needed conditions.

>> Source/WebCore/editing/Editor.cpp:1710
>> +    // Get the word arround the caret.
> 
> Instead of adding a comment like this, please extract the code that extracts the word around the caret as a function.

The method's content has been changed. Please let me know if the extract of this functionality is still needed.

>> Source/WebCore/page/ContextMenuController.cpp:348
>> +            TypingCommand::insertText(frame->document(), item->title(), 0);
> 
> We need to call shouldInsertText here as we do it in non-Linux case. We also should be using ReplaceSelectionCommand instead of deleteWordWithDirection and insertText.
> Finally, I'm not certain why we don't want to call revealSelection here. r- because of this.

Ok, added shouldInsertText() call for non-Linux case as well as revealSelection(). I am not sure if we can use ReplaceSelectionCommand as it requires the selection that is made on FrameSelection.

[Grzegorz Czajkowski]

Created attachment 181693 [details]
applied  Ryosuke's suggestions

[Ryosuke Niwa]

Comment on attachment 181693 [details]
applied  Ryosuke's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=181693&action=review

> Source/WebCore/editing/Editor.cpp:1693
> +    // Get the word's range arround the caret.

This comment repeats what the code says already. Please remove.

> Source/WebCore/editing/Editor.cpp:1694
> +    VisibleSelection wordSelection(selection.start());

You should use selection.base() instead.

> Source/WebCore/editing/Editor.cpp:1698
> +    // If some text was selected, don't provide suggestions for the multiple words.

I don't think this comment is accurate. The selection could be selecting a part of word instead for the condition below to be true.
Either you have to update the comment or the code, and this is why I prefer making the code self-evident over adding comments.

Also, if you're adding a comment, please provide "why" comment instead of "what". In particular, it's helpful to know why we don't want to provide suggestions when the current selection doesn't exactly match the wordSelection.

At glance, I found it odd that we do this because the bug says we should be able to provide suggestions without selection words and yet we bail out when there's a selection that doesn't match wordSelection.

> Source/WebCore/editing/Editor.cpp:1714
> +String Editor::misspelledSelection() const

We should rename this to something like misspelledSelectionString instead.

> Source/WebCore/page/ContextMenuController.cpp:349
> +                frame->selection()->modify(FrameSelection::AlterationMove, DirectionBackward, WordGranularity, UserTriggered);
> +                frame->editor()->deleteWithDirection(DirectionForward, WordGranularity, false, true);

This ain't right. This, under certain conditions, results in a different selection than VisibleSelection with word granularity. You should do what you did in misspelledWordAtCaretOrRange instead to make sure the selection matches.

> Source/WebCore/page/ContextMenuController.cpp:350
> +                TypingCommand::insertText(document, item->title(), 0);

Please use ReplaceSelectionCommand and share the code with the else clause.

[Grzegorz Czajkowski]

Created attachment 181885 [details]
polish the patch regarding to Ryosuke's comments

[Ryosuke Niwa]

Comment on attachment 181885 [details]
polish the patch regarding to Ryosuke's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=181885&action=review

> Source/WebCore/page/ContextMenuController.cpp:361
> -            frame->selection()->revealSelection(ScrollAlignment::alignToEdgeIfNeeded);
> +            frameSelection->revealSelection(ScrollAlignment::alignToEdgeIfNeeded);

Don't you want to restore selection in this case? Otherwise the caret appears after the replaced text. Is that desired behavior?

[Grzegorz Czajkowski]

Comment on attachment 181885 [details]
polish the patch regarding to Ryosuke's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=181885&action=review

>> Source/WebCore/page/ContextMenuController.cpp:361
>> +            frameSelection->revealSelection(ScrollAlignment::alignToEdgeIfNeeded);
> 
> Don't you want to restore selection in this case? Otherwise the caret appears after the replaced text. Is that desired behavior?

Actually, I don't have strong opinion on that. I just followed non Linux case where the caret is shown after the replaced text. Additionally it also makes selection on replaced text (replaceOptions |= ReplaceSelectionCommand::SelectReplacement;)

From UX point of view (I believe) it's good that the caret appears after, in common case user has just replaced the text with the given suggestion and he probably won't be interested in modification it once again.

[Grzegorz Czajkowski]

Comment on attachment 181885 [details]
polish the patch regarding to Ryosuke's comments

Ryosuke, Tony and Hajime - thanks for supporting me in this bug!

[WebKit Review Bot]

Comment on attachment 181885 [details]
polish the patch regarding to Ryosuke's comments

Clearing flags on attachment: 181885

Committed r139412: <http://trac.webkit.org/changeset/139412>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.