Bug 113742 – Tests added in r141354 erroneously assumes correction suggestion to be in the context menu

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Most of tests added in r141354 asserts that the last item in the context menu contains the correction suggestion.
This isn't true in non-Chromium ports.

[Ryosuke Niwa]

Also see 112554.

[Rouslan Solomakhin]

Where do the other ports show the correction suggestion?

[Ryosuke Niwa]

(In reply to comment #2)
> Where do the other ports show the correction suggestion?

Mac port, for example, has correction panel.

[Grzegorz Czajkowski]

(In reply to comment #2)
> Where do the other ports show the correction suggestion?

EFL uses context menu but correction suggestions are put at the beginning of the list.
So the condition:
shouldBeEqualToString("contextMenuElements[contextMenuElements.length - 1]", "welcome");
is not true for EFL.

Secondly, WK2's contextClick() returns array of objects (not strings) where object contains title (as string) and method click() for the object.

Would it be sufficiently to just check size of contextMenuElements before and after the click? It has been done in context-menu-suggestions.html to pass the test for Mac (r60693).

[Ryosuke Niwa]

Sounds reasonable. Since the test was added by a Chromium contributor, and Chromium no longer uses WebKit, it seems reasonable to modify the test accordingly.

[Rouslan Solomakhin]

(In reply to comment #5)
> Sounds reasonable. Since the test was added by a Chromium contributor, and Chromium no longer uses WebKit, it seems reasonable to modify the test accordingly.

Yep, please go ahead.

[Grzegorz Czajkowski]

Created attachment 202670 [details]
compare quantity of context menu elements

[Ryosuke Niwa]

Comment on attachment 202670 [details]
compare quantity of context menu elements

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

> LayoutTests/editing/spelling/spelling-multiword-selection.html:30
> +    // Context click on 'wellcome' to get its spell check suggestions.
> +    var x = destination.offsetParent.offsetLeft + destination.offsetLeft + 40;
> +    var y = destination.offsetParent.offsetTop + destination.offsetTop + destination.offsetHeight / 2;
> +    eventSender.mouseMoveTo(x, y);
> +    contextMenuElementsWithSpellingSuggestions = eventSender.contextClick();
> +    // Esc key to hide the context menu.
> +    eventSender.keyDown(String.fromCharCode(0x001B), null);
> +

We shouldn't be checking the items inside a context menu. The fact some ports populate context menu with spelling suggestions doesn't mean all ports do.

[Grzegorz Czajkowski]

(In reply to comment #8)
> (From update of attachment 202670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202670&action=review
> 
> > LayoutTests/editing/spelling/spelling-multiword-selection.html:30
> > +    // Context click on 'wellcome' to get its spell check suggestions.
> > +    var x = destination.offsetParent.offsetLeft + destination.offsetLeft + 40;
> > +    var y = destination.offsetParent.offsetTop + destination.offsetTop + destination.offsetHeight / 2;
> > +    eventSender.mouseMoveTo(x, y);
> > +    contextMenuElementsWithSpellingSuggestions = eventSender.contextClick();
> > +    // Esc key to hide the context menu.
> > +    eventSender.keyDown(String.fromCharCode(0x001B), null);
> > +
> 
> We shouldn't be checking the items inside a context menu. The fact some ports populate context menu with spelling suggestions doesn't mean all ports do.

Hi Ryosuke, we've ported some of those tests to unit tests (at lest for EFL, bug 118123). Do you think it's good time to get rid of layout tests which verify spelling suggestions?

[Ryosuke Niwa]

(In reply to comment #9)
> Hi Ryosuke, we've ported some of those tests to unit tests (at lest for EFL, bug 118123). Do you think it's good time to get rid of layout tests which verify spelling suggestions?

Do we have unit tests on all ports? If not, we can only remove parts of the layout tests that check the presence of suggestions in context menus. Checks for markers, etc... are still valuable.

[Grzegorz Czajkowski]

(In reply to comment #10)
> (In reply to comment #9)
> > Hi Ryosuke, we've ported some of those tests to unit tests (at lest for EFL, bug 118123). Do you think it's good time to get rid of layout tests which verify spelling suggestions?
> 
> Do we have unit tests on all ports? If not, we can only remove parts of the layout tests that check the presence of suggestions in context menus. Checks for markers, etc... are still valuable.

Added this to my TODO list. Anyway, is there any WebKit port which does not populate spelling suggestions in context menu? Safari on Mac populates them as well as EFL and GTK+. Please correct me if I am wrong.

[Grzegorz Czajkowski]

Created attachment 213355 [details]
remove spellchecking suggestions from the most tests

[WebKit Commit Bot]

Comment on attachment 213355 [details]
remove spellchecking suggestions from the most tests

Clearing flags on attachment: 213355

Committed r156901: <http://trac.webkit.org/changeset/156901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 213355 [details]
remove spellchecking suggestions from the most tests

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

> LayoutTests/ChangeLog:35
> +        * editing/spelling/context-menu-suggestions-multiword-selection.html: Renamed from LayoutTests/editing/spelling/spelling-multiword-selection.html.

This test used to pass on Mac, but now it's super flaky. Filed bug 122414.