Bug 113742 - Tests added in r141354 erroneously assumes correction suggestion to be in the context menu
Summary: Tests added in r141354 erroneously assumes correction suggestion to be in the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on: 122414
Blocks: 108528
  Show dependency treegraph
 
Reported: 2013-04-01 21:02 PDT by Ryosuke Niwa
Modified: 2013-10-06 10:27 PDT (History)
6 users (show)

See Also:


Attachments
compare quantity of context menu elements (10.29 KB, patch)
2013-05-23 06:02 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
remove spellchecking suggestions from the most tests (38.68 KB, patch)
2013-10-04 06:12 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-04-01 21:02:08 PDT
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.
Comment 1 Ryosuke Niwa 2013-04-01 21:02:42 PDT
Also see 112554.
Comment 2 Rouslan Solomakhin 2013-04-02 09:40:07 PDT
Where do the other ports show the correction suggestion?
Comment 3 Ryosuke Niwa 2013-04-02 20:37:07 PDT
(In reply to comment #2)
> Where do the other ports show the correction suggestion?

Mac port, for example, has correction panel.
Comment 4 Grzegorz Czajkowski 2013-04-04 01:34:59 PDT
(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).
Comment 5 Ryosuke Niwa 2013-04-04 02:08:16 PDT
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.
Comment 6 Rouslan Solomakhin 2013-04-04 08:39:21 PDT
(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.
Comment 7 Grzegorz Czajkowski 2013-05-23 06:02:03 PDT
Created attachment 202670 [details]
compare quantity of context menu elements
Comment 8 Ryosuke Niwa 2013-05-28 11:33:58 PDT
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.
Comment 9 Grzegorz Czajkowski 2013-07-05 02:38:05 PDT
(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?
Comment 10 Ryosuke Niwa 2013-07-14 17:42:55 PDT
(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.
Comment 11 Grzegorz Czajkowski 2013-09-17 07:25:10 PDT
(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.
Comment 12 Grzegorz Czajkowski 2013-10-04 06:12:04 PDT
Created attachment 213355 [details]
remove spellchecking suggestions from the most tests
Comment 13 WebKit Commit Bot 2013-10-04 11:38:40 PDT
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>
Comment 14 WebKit Commit Bot 2013-10-04 11:38:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Alexey Proskuryakov 2013-10-06 10:27:25 PDT
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.