Bug 25639 – Spellcheck disabling does not disable context menu

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

[Julie Parent]

Setting spellcheck=false seems to disable the red squiggly, but if you right click on a misspelled word, the context menu still has spelling suggestions and other spellcheck related items.

[Tony Chang]

Created attachment 55819 [details]
Patch

[Tony Chang]

(In reply to comment #1)
> Created an attachment (id=55819) [details]
> Patch

This is not for review because it has no tests.  But I wanted some suggestions on how to test.  We can't use a pixel test because context menus aren't shown.  I considered adding a method to eventSender, but there's no method in the WebKit api for getting the menu items.  It's probably possible for me to crawl the NSMenu of a showing context menu, but is that too much of a hack?

The other option is to just write a manual layout test.  Suggestions welcome, otherwise I'll probably try to crawl the NSMenu tomorrow.

[Evan Martin]

I wonder how other context menu items (cut/copy/paste/copy link address) are tested?

[Darin Adler]

I don't think this is the right level to do this check; the check itself is OK but a bit too low level. If you look at the call site in ContextMenu::populate, it has some logic about menu items related to spelling that runs even if the function returns false.

I think a better fix would be creating a function that combines the isInPasswordField check with the spellCheckingEnabledInFocusedNode check and calling that in ContextMenu::populate. That function could be local to the ContextMenu file and class or in the Editor or wherever seems most natural and clear.

[Tony Chang]

Created attachment 55937 [details]
Patch

[Tony Chang]

(In reply to comment #3)
> I wonder how other context menu items (cut/copy/paste/copy link address) are tested?

As far as I can tell, people test the behavior (e.g., the result of a paste), not the menu itself.

(In reply to comment #4)
> I don't think this is the right level to do this check; the check itself is OK but a bit too low level. If you look at the call site in ContextMenu::populate, it has some logic about menu items related to spelling that runs even if the function returns false.

I moved the check to ContextMenu.  I wasn't sure what to do about the "Spelling and Grammar" menu.  The current patch keeps it, but I think either behavior would be ok.  I didn't merge with the isInPasswordField because it's used for other things like disabling transformations.

This includes a layout test with an extension to eventSender.  Since it gets the context menu items, I'll have to grab results for Tiger and Snow Leopard from the bots.

[Darin Adler]

Comment on attachment 55937 [details]
Patch

Comments in Skipped files start with "#" rather than "//".

You have a TODO to add a bug number. Please do that.

> +    if (subView) {
> +        NSMenu* menu = [subView menuForEvent:event];
> +        if (shouldPrintMenuItems) {
> +            printf("ContextMenuItems: ");
> +            for (int i = 0; i < [menu numberOfItems]; ++i) {

It doesn't make sense to set up the menu local variable before checking the shouldPrintMenuItems.

> +                if (i > 0)
> +                    printf(", ");
> +                NSMenuItem* menuItem = [menu itemAtIndex:i];
> +                printf("%s", [menuItem isSeparatorItem] ? "----" : [[menuItem title] UTF8String]);

Since menu item titles are localized, it might be better to log something else, perhaps the selector, reformatted in to the style of a WebCore editor command name.

    SEL selector = [menuItem action];
    char commandName[256];
    strcpy(commandName, sel_getName(selector));
    size_t commandName Length = strlen(commandName);
    if (commandNameLength >= 2 && commandName[commandNameLength - 1] == ':') {
        commandName[0] = toupper(commandName[0]);
        commandName[commandNameLength - 1] = '\0';
    }

Using printf with a constant string or a string of just "%s" is an anti-pattern. fputs(stdio, <string>) is better.

[Darin Adler]

Comment on attachment 55937 [details]
Patch

review- so you can rework the tests a bit. The code change seems fine.

[Tony Chang]

Created attachment 56044 [details]
Patch

[Tony Chang]

(In reply to comment #7)
> (From update of attachment 55937 [details])
> Comments in Skipped files start with "#" rather than "//".

Fixed.

> You have a TODO to add a bug number. Please do that.

Fixed.

> > +    if (subView) {
> > +        NSMenu* menu = [subView menuForEvent:event];
> > +        if (shouldPrintMenuItems) {
> > +            printf("ContextMenuItems: ");
> > +            for (int i = 0; i < [menu numberOfItems]; ++i) {
> 
> It doesn't make sense to set up the menu local variable before checking the shouldPrintMenuItems.

We always want to run [subView menuForEvent:event], so I put it outside the shouldPrintMenuItems check.

> > +                if (i > 0)
> > +                    printf(", ");
> > +                NSMenuItem* menuItem = [menu itemAtIndex:i];
> > +                printf("%s", [menuItem isSeparatorItem] ? "----" : [[menuItem title] UTF8String]);
> 
> Since menu item titles are localized, it might be better to log something else, perhaps the selector, reformatted in to the style of a WebCore editor command name.

I see.  I tried this using the code you provided, but I got a bunch of ForwardContextMenuAction and SubmenuAction.  It looks like it keeps the value in tag and there's a big switch statement for determining the action.  We could just write out the tag (int) or maybe have a switch to map enum values to strings (seems like it might be a pain to keep in sync).

The current patch has tag number + title, but it's mostly just to demonstrate the output.  Thoughts?

[Darin Adler]

Comment on attachment 56044 [details]
Patch

I guess dumping the title is best. Probably not all that helpful to dump the tag. Sorry for sending you off on a wild goose chase before.

[Adam Barth]

Attachment 56044 [details] was posted by a committer and has review+, assigning to Tony Chang for commit.

[Adam Barth]

Comment on attachment 56044 [details]
Patch

This looks straightforward enough.  Tossing it on the commit-queue for Tony.

[WebKit Commit Bot]

Comment on attachment 56044 [details]
Patch

Rejecting patch 56044 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1
Last 500 characters of output:
opard/editing/spelling/context-menu-suggestions-expected.txt
patching file LayoutTests/platform/qt/Skipped
Hunk #1 succeeded at 5090 (offset -6 lines).
patching file LayoutTests/platform/win/Skipped
Hunk #1 succeeded at 876 (offset 4 lines).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/ContextMenu.cpp
patching file WebKitTools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKitTools/DumpRenderTree/mac/EventSendingController.mm

Full output: http://webkit-commit-queue.appspot.com/results/2273195

[Tony Chang]

Committed r59585: <http://trac.webkit.org/changeset/59585>