It is possible to duplicate the suggestions on spell checking

RESOLVED FIXED in Firefox 3

Status

()

Firefox
Menus
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Maxim Belomerzaev, Assigned: Ehsan)

Tracking

({regression})

Trunk
Firefox 3
regression
Points:
---
Bug Flags:
blocking-firefox3 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
Created attachment 312241 [details]
Screenshot

Screenshot added
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.
Blocks: 404536
Keywords: regression
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

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

Flags: blocking-firefox3?
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).
Component: Form Manager → Menus
QA Contact: form.manager → menus
Hardware: PC → All
(Assignee)

Comment 5

10 years ago
Created attachment 312569 [details] [diff] [review]
Patch (v1)

Patch to fix the problem: don't return early from setTarget(), just set the shouldDisplay flag and proceed.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #312569 - Flags: review?(gavin.sharp)
Flags: blocking-firefox3? → blocking-firefox3+

Updated

10 years ago
Whiteboard: [has patch][needs review gavin]
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...
Attachment #312569 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin] → [has patch]

Updated

10 years ago
Whiteboard: [has patch] → [has patch][has reviews]
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.
(Assignee)

Comment 8

10 years ago
(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...
(Assignee)

Updated

10 years ago
Attachment #312569 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][has reviews] → [needs new patch]
(Assignee)

Comment 9

10 years ago
Created attachment 313026 [details] [diff] [review]
Patch (v1.1) (for check-in)

The updated patch, as promised.
Attachment #313026 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Whiteboard: [needs new patch] → [has patch] [needs review gavin]
(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.
Whiteboard: [has patch] [needs review gavin] → [needs new patch]
Attachment #313026 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Whiteboard: [needs new patch]
(Assignee)

Updated

10 years ago
Attachment #313026 - Attachment description: Patch (v1.1) → Patch (v1.1) (for check-in)
(Assignee)

Comment 11

10 years ago
Created attachment 313046 [details]
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?
(Assignee)

Comment 12

10 years ago
sayrer: seeking help with the unit test (see comment 11).
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
You need to log in before you can comment on or make changes to this bug.