425631 - It is possible to duplicate the suggestions on spell checking

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Maxim Belomerzaev]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032722 Firefox/3.0pre Flock/1.3pre
Build Identifier: Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008032705 Minefield/3.0pre

Spell Checker duplicates the suggestion of wrong spelled word if user clicks on any form element by right mouse button.



Reproducible: Always

Steps to Reproduce:
1. Start FF and download any page with text area or edit field and windows styled button or radio buttons (for example: http://cgi-lib.berkeley.edu/ex/simple-form.html) 
2. Type wrong spelled word in text area (for example: peple). 
3. Enable spell checking in this field. 
4. Verify that wrong spelled word is underlined and spell checker suggests some right variants of word. Do not select any right variant. 
5. Right click 2 or more times on “here” button or any of radio-buttons. 
6. Verify that spell checker duplicates the suggestions of wrong spelled word (Click by right mouse button on wrong spelled word) 
7. Select any of word from spell checker suggestions 
8. Right click on any word and verify that duplicates are still existed in spell checker suggestions
Actual Results:  
Spell Checker duplicates the suggestion of wrong spelled word if user clicks on any form element by right mouse button. Please, see attached screenshot
Duplicated suggestions do not disappear until user close browser.

Expected Results:  
Spell checker should not duplicate spell checker suggestions

Comment 1

[Maxim Belomerzaev]

Attachment: Screenshot

Screenshot added

Comment 2

[Ria Klaassen (not reading all bugmail)]

This regressed on 23 Feb 2008:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1203835260&maxdate=1203839099
so probably a regression from Bug 404536.
The spell suggestions persist during all next actions.

Comment 3

[Brian Polidoro]

Sort of similar to Bug 342675.  So fixing this could kill two birds with one stone.  Or possibly give insight into the other bug.  

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Indeed, this was caused by bug 404536. Returning early from setTarget means clearSuggestionsFromMenu/clearDictionaryListFromMenu aren't called.

I think this should block, because we need to make sure we're resetting the rest of the context menu state correctly (not doing it properly can have security implications).

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Patch to fix the problem: don't return early from setTarget(), just set the shouldDisplay flag and proceed.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 312569 [details] [diff] [review]
Patch (v1)

r=me, but can you file a followup about investigating whether a similar change is needed for the XUL check? Seems like we could get into the same situation if you right click on a XUL element in a mixed XUL/XHTML document. Would be nice to get a browser-chrome test for this as well, shouldn't be too hard...

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 312569 [details] [diff] [review]
Patch (v1)

>Index: browser/base/content/nsContextMenu.js

>   setTarget: function (aNode, aRangeParent, aRangeOffset) {
>     const xulNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>-    if (aNode.namespaceURI == xulNS ||
>-        this.isTargetAFormControl(aNode)) {
>+    if (aNode.namespaceURI == xulNS) {
>       this.shouldDisplay = false;
>       return;
>     }

>+    this.shouldDisplay     = !this.isTargetAFormControl(aNode);

Though it might be clearer to just make this:

if (this.isTargetAFormControl(aNode))
  this.shouldDisplay = false;

and put it right after the xulNS check at the top.

Comment 8

[(no longer active)]

(In reply to comment #6)
> (From update of attachment 312569 [details] [diff] [review])
> r=me, but can you file a followup about investigating whether a similar change
> is needed for the XUL check? Seems like we could get into the same situation if
> you right click on a XUL element in a mixed XUL/XHTML document.

Yes, XUL elements are affected by the same problem as well (see the URL).  Let's fix it in this bug as well.  I'll post an updated patch which also addresses comment 7.

> Would be nice
> to get a browser-chrome test for this as well, shouldn't be too hard...

You mean a test which simulates the right clicks and then looks at the context menu for the duplicate entries?  The only problem is I have no idea how to dismiss the context menu in a chrome test...

Comment 9

[(no longer active)]

Attachment: Patch (v1.1) (for check-in)

The updated patch, as promised.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> > Would be nice
> > to get a browser-chrome test for this as well, shouldn't be too hard...
> 
> You mean a test which simulates the right clicks and then looks at the context
> menu for the duplicate entries?  The only problem is I have no idea how to
> dismiss the context menu in a chrome test...

You don't even need the menu to appear, you can just create an nsContextMenu directly and see that it behaves correctly. See the patch in bug 417483 for an example.

Comment 11

[(no longer active)]

Attachment: Unit test (WIP)

OK, I can't get the unit test to work.  Here's the test I've written.  It fails with "At least one spell check alternative should be suggested." (line 50), which means that no spelling suggestions have been added to the menu.

Can anyone shed any lights on this?

Comment 12

[(no longer active)]

sayrer: seeking help with the unit test (see comment 11).

Comment 13

[Reed Loden [:reed]]

Checking in browser/base/content/nsContextMenu.js;
/cvsroot/mozilla/browser/base/content/nsContextMenu.js,v  <--  nsContextMenu.js
new revision: 1.54; previous revision: 1.53
done