Last Comment Bug 352037 - Undo Add To Dictionary
: Undo Add To Dictionary
Status: RESOLVED FIXED
: ue
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: mozilla12
Assigned To: Quentin Headen [:qheaden]
:
: Jet Villegas (:jet)
Mentors:
: 384234 415779 416400 453148 511202 (view as bug list)
Depends on:
Blocks: 722640 970717 715410 722636
  Show dependency treegraph
 
Reported: 2006-09-10 07:03 PDT by Elliotte Rusty Harold
Modified: 2014-02-10 18:02 PST (History)
28 users (show)
zurtex: wanted1.9.1?
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Functionality Patch - (Tests Not Fixed) (6.80 KB, patch)
2011-11-12 18:14 PST, Quentin Headen [:qheaden]
no flags Details | Diff | Splinter Review
Patch V2 (Almost Complete) (15.24 KB, patch)
2011-12-19 21:37 PST, Quentin Headen [:qheaden]
ehsan: review+
Details | Diff | Splinter Review
Patch v3 (21.60 KB, patch)
2011-12-21 16:17 PST, Quentin Headen [:qheaden]
no flags Details | Diff | Splinter Review
Patch v3.1 (21.18 KB, patch)
2011-12-21 19:37 PST, Quentin Headen [:qheaden]
ehsan: review+
ehsan: checkin+
Details | Diff | Splinter Review

Description Elliotte Rusty Harold 2006-09-10 07:03:37 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2

Undo (Cmnd-Z, Edit/Undo) does not Undo adding a word to the dictionary. Instead it undoes the last edit. 

Reproducible: Always

Steps to Reproduce:
1. Type something misspelled such as "Foog"
2. Right click the word and select "Add to Dictionary" from the context menu
3. Go the edit menu and select Undo. 

Actual Results:  
The last typed text is deleted. 

Expected Results:  
The new word is removed from the dictionary. No text is deleted. The word is once again marked as misspelled.  

Probably related to 312778, but this may be an additional means to trigger the same bug (different context menu item); and does not appear fixed as of 2.0b2. The undo action that's needed here is also different, since the dictionary change must be backed out.
Comment 1 u88484 2008-02-08 16:01:50 PST
*** Bug 384234 has been marked as a duplicate of this bug. ***
Comment 2 u88484 2008-02-08 16:02:01 PST
*** Bug 415779 has been marked as a duplicate of this bug. ***
Comment 3 u88484 2008-02-08 16:02:09 PST
*** Bug 416400 has been marked as a duplicate of this bug. ***
Comment 4 Damian Shaw [Quan] 2008-02-08 16:35:40 PST
o.k, I'd just like to add to this bug 415779 has a good point of moving "add to dictionary" further away from the words, thus causing less problems.
Comment 5 Damian Shaw [Quan] 2008-08-28 12:47:31 PDT
Nominating this bug for being wanted, it seems somewhat in limbo atm.
Comment 6 Ria Klaassen (not reading all bugmail) 2008-09-01 15:50:40 PDT
*** Bug 453148 has been marked as a duplicate of this bug. ***
Comment 7 James Gillies 2008-11-05 03:49:22 PST
Seconded

In the same vain, having the GUI enhancement to move the "Add to Dictionary" button moved further down the menu to prevent accidental clicking would be great.

It's a real pain in the backside to have to manually edit persdict and then re-open Firefox when I had 20+ tabs open at work.

I have had to go through this process three times now in a fortnight. (What can I say, I was really busy, there was only one suggestion in the list because "bacisally" can only be spelt one way and I clicked the wrong button - just.)

That said, I cannot leave it and have the chance of a client get a typo from me, it's just not cricket.

Thank you for your consideration.

James
Comment 8 Fulano 2008-11-25 10:27:28 PST
I like the idea of adding the undo feature, but for now can we simply move the add to dictionary command next to the undo command so there is at least a spacer between the suggestions and add to dictionary?  Then somebody can add the undo feature another day.
Comment 9 Gavin 2008-11-25 10:33:57 PST
I agree with James. That sounds practical.
Comment 10 Michael Gilbert 2009-03-25 12:26:46 PDT
What about presenting the fact that the "add to dictionary" action has occured in the yellow warning bar (the one that gets presented for things like flash/plugin installation) along with an "undo" button?

Mabye an "edit -> dictionary" menu item would also be useful.

Or maybe the right-click "add to dictionary" menu should be renamed to "dictionary", which would take the user to a more comprehensive dictionary that has an option to add the mispelled word.  This would prevent errant clicks while still allowing the "add to" action if the user deliberately wants to do so.
Comment 11 Michael Gilbert 2009-03-25 12:37:01 PDT
Note that the last suggestion would also reduce some menu clutter -- the "languages" entry in the right-click menu could be moved into the new "dictionary" dialog.
Comment 12 Matt Caywood 2009-06-18 00:44:36 PDT
This might be easier than it looks, and the UI could be pretty simple too (no need for a dictionary editor).

The Personal Dictionary check is actually done in mozHunspell.cpp. It wouldn't be too hard to record whether a "correct" word was found in the main Hunspell dictionary or the Personal Dictionary. 

If it was in the Personal Dictionary, then add a menu item for "Remove from Dictionary" where otherwise there would be an item for "Add to Dictionary."

Not volunteering to fix it yet, just saying.
Comment 13 Matt Caywood 2009-09-30 17:56:09 PDT
*** Bug 511202 has been marked as a duplicate of this bug. ***
Comment 14 Brandon 2011-10-07 05:44:06 PDT
Why in the world hasn't this been fixed? The raw undo functionality alone would be enough, but the other suggestions are awesome too. Yet it's two years old and the problem persists. I'm even looking for a plugin or greasemonkey script to do it since apparently mozilla won't.

This kind of **** and unresponsiveness is why people go to chrome.
Comment 15 :Ehsan Akhgari 2011-10-07 06:30:34 PDT
CCing some UX folks to see if they have ideas on how the user interaction should work here...
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-07 16:04:52 PDT
As far as I can tell, Chrome doesn't have "undo add to dictionary" functionality.
Comment 17 Alex Limi (:limi) — Firefox UX Team 2011-10-11 01:02:17 PDT
A simple way would be to include an extra item in the context menu “Undo adding ‘XYZ’ to the dictionary” and make it only appear when there's something that was just added — ie. the “oops” case.
Comment 18 Alex Limi (:limi) — Firefox UX Team 2011-10-11 01:04:31 PDT
(In reply to Brandon from comment #14)
> This kind of **** and unresponsiveness is why people go to chrome.

Hi Brandon, I know you're new to Bugzilla. We try to keep it civil around here.

Rude remarks like that are not appreciated. There are thousands of things that are more important to fix than this particular issue, so please keep some perspective when commenting. :)
Comment 19 Brandon 2011-10-11 05:03:27 PDT
And that's exactly what I'm talking about. Thanks for proving my point Alex. 

That same fallacious old song and dance. 

I don't care what you consider to be more important. I am the end user and my demands are not structured around developer limitations or opinions, quite the opposite. You exist to serve Us. Your usefulness is defined by what we consider to be problems. 

You remind me of spoiled Corporations convinced that their service is a privilege for which customers should be obsequiously grateful.

No.

It is Your perspective that is in need of revision.

This complaint is as old as punch cards and open source software itself, and the answer has always been the same.

Either you are being paid, in which case its your job to solve the problems We, the users, deem important, or you are not in which case you're a volunteer and your complaint is a bit like punching a brick wall and then complaining about your hand. 

If you don't like it, tough, that's why you're compensated with pay, or If you don't like it, stop, no one's forcing you to volunteer (or even answer).

This kind of "you'll take what we give you and any general complaint is rudeness" attitude is exactly why people reject closed source software in the first place.

I guess the bloody history of Africa, the Stanford prison experiment, Gaddafi, all the way down to the attitudes of privileged developers like yourself, show the same consistent human trend, when the revolutionary becomes the autocrat, (when the rouge web browser becomes the de facto standard) the regime acts the same.

I think Frank Herbert explained it best speaking through Leto II when he said... 

"All rebellions are ordinary and an ultimate bore. They are copied out of the same pattern, one much like another. The driving force is adrenalin addiction and the desire to gain personal power. All rebels are closet aristocrats."~God Emperor of Dune

Specifically, there are Not "thousands of things" (hyperbole if there ever was any) "more important" to Me, if at all. In fact I'm quite pleased with firefox other than with this Long standing, and now I see intentionally preserved annoyance.

Again, what you consider to be important phases me not one whit. I don't care how hard you job is just like you don't care how hard mine is. I will complain when I feel the need.

A tiny hyper annoying problem that persists for years because of sheer neglect Is ****, and if characterizing it thus makes one rude then propriety becomes a massive Nirvana Fallacy, wherein an infinite progression of more important things prevents anything from getting done as easily fixable problems are constantly shoved to the side in favor of impossibly difficult ones.

Even if your snide condescending statements (and tone) were justified, (which neither are) the fact would remain that importance does not dictate priority. The trade off between action and result dictates priority. In this case, sure there may be more "important" (defined by who? apparently not consensus) problems but the solution to this one is obvious and judging from your own dismissal of it as trivial, apparently quite easy to fix.

This error, judging from this report alone, is at least 5 Years old. I and others have been patient enough. After 5 years a small problem being unsolved becomes a big problem.

Searching for "firefox, remove from dictionary, undo add" in Google produces 24 million results. It's a constant support question that has earned square miles of forum real estate. I doubt there's hardly any avid firefox users who haven't hit this wall.

To ignore that for a minimum of 5 years is by definition unresponsive.

I've said nothing rude. Only truth.

Feel free to kick me for it. I'm used to that behavior from autocrats.
Comment 20 Scott M. Sanders 2011-10-11 05:10:42 PDT
Just edit persdict.dat or write an add-on to do so, rather than expect others to fix it.
Comment 21 Brandon 2011-10-11 05:26:01 PDT
@Scott M. Sanders

If we're expected to fix it ourselves then why bother having a bug forum?
Comment 22 :Ehsan Akhgari 2011-10-11 08:17:54 PDT
Quentin, is this something that you would be interested to work on?  :-)
Comment 23 Quentin Headen [:qheaden] 2011-10-11 13:51:09 PDT
@Ehsan

This looks like an interesting bug. I'll start looking into it, so you can assign me if you want.

Just to be clear, you want the undo button to remove the word from the dictionary?
Comment 24 Alex Limi (:limi) — Firefox UX Team 2011-10-11 15:17:46 PDT
(In reply to Quentin Headen from comment #23)
> @Ehsan
> 
> This looks like an interesting bug. I'll start looking into it, so you can
> assign me if you want.
> 
> Just to be clear, you want the undo button to remove the word from the
> dictionary?

Yes. It should be a separate menu item from the Undo edit menu item, though.
Comment 25 :Ehsan Akhgari 2011-10-11 15:35:31 PDT
Great!  I'm assigning this to you then.  :-)

Let me know if you needed some pointers on how this should be implemented.  I won't attempt to spoil the fun for you, so for now I'll just mention that you need to look at the mozIPersonalDictionary interface as a starting point.  :-)
Comment 26 Alex Faaborg [:faaborg] (Firefox UX) 2011-10-11 16:41:31 PDT
I have to admit I kind of liked the God Emperor of Dune quote, but nonetheless we try to keep bugs focused on specific technical details of fixing the problem at hand, as opposed to a more informal forum of advocacy.

>If we're expected to fix it ourselves then why bother having a bug forum?

it's an effective way to recruit our core developers.
Comment 27 Alex Limi (:limi) — Firefox UX Team 2011-10-11 20:19:38 PDT
>If we're expected to fix it ourselves then why bother having a bug forum?

It's not a forum, where we discuss our taste in literature, reference dictatorships and try to impress women on the internet. It's a bug tracker — we report bugs, they get prioritized, and eventually fixed. 

I do think that people that report a bug in general don't need to get an email every time there's a small change in the bug, but should rather get notified when it's fixed or when another state change happens. In general, Bugzilla is way too noisy.
Comment 28 Quentin Headen [:qheaden] 2011-10-12 10:31:35 PDT
I have another question about desired behavior. How far back should the undo operation be available? In other words, if you add 16 words to the dictionary, for example, does the "undo add" function have to be able to remove all 16 words, or just the last word added?
Comment 29 Alex Limi (:limi) — Firefox UX Team 2011-10-12 17:21:19 PDT
Just the last one. We're trying to fix the "oops" case here.
Comment 30 :Ehsan Akhgari 2011-10-13 13:55:38 PDT
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #29)
> Just the last one. We're trying to fix the "oops" case here.

For the implementation, I think we should use a stack though.  We can set the maximum stack size limit to 1 for now...
Comment 31 Quentin Headen [:qheaden] 2011-10-15 13:54:05 PDT
Where is the code for the context menu that pops up when you right click a misspelled word? I need to add the undo button there, but I don't know where to add the code for it.
Comment 34 Quentin Headen [:qheaden] 2011-10-19 00:14:01 PDT
I have a question. I am following the context element "spell-add-to-dictionary" as a guide on where to place my "spell-remove-from-dictionary" element. 

Why is the "spell-add-to-dictionary" context element defined in two different places? First, it is defined in the toolkit/content/widgets/textbox.xml file, then it is defined in the browser/base/content/browser-context.inc file.

Please let me know.

Thanks
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-19 08:47:31 PDT
(In reply to Quentin Headen from comment #34)
> Why is the "spell-add-to-dictionary" context element defined in two
> different places? First, it is defined in the
> toolkit/content/widgets/textbox.xml file, then it is defined in the
> browser/base/content/browser-context.inc file.

The former is used for <xul:textbox> generically (text input boxes in the Firefox user interface), while the latter is used for the Firefox content menu specifically (text fields in web pages). We probably want to make the same functionality changes to both (they share much of the code via InlineSpellChecker.jsm), but if you'd like you can focus on the browser/ version to start with (since it's a little easier to test).
Comment 36 Quentin Headen [:qheaden] 2011-11-06 17:25:13 PST
Hey guys. Sorry I've been so slow on this bug. College assignments had me busy for a while. :)

So I will start back on this bug again.
Comment 37 Quentin Headen [:qheaden] 2011-11-12 18:14:55 PST
Created attachment 574106 [details] [diff] [review]
Functionality Patch - (Tests Not Fixed)

This is a functionality patch. It does not contain any new tests, and it does not contain fixes for broken tests. I will submit it to the try server and fix any tests that are broken.

To try the enhancement, type a misspelled word, add it to the dictionary, then right click again to find a button that says "Undo Add To Dictionary". Click that, and retype the word. The word should be highlighted again.
Comment 38 Quentin Headen [:qheaden] 2011-11-13 03:14:38 PST
Amazingly, my patch did not break any tests. Here is the TBPL report: https://tbpl.mozilla.org/?tree=Try&rev=5a6c01907165

I still have to write new tests for my patch, which I am working on now.
Comment 39 Quentin Headen [:qheaden] 2011-11-13 03:20:24 PST
I need some help on where I should add tests at for this enhancement.
Comment 40 Quentin Headen [:qheaden] 2011-11-23 06:15:46 PST
Can anyone apply my patch just to make sure I am going down the right track with this? Just verify if the functionality I implemented is correct.
Comment 41 :Ehsan Akhgari 2011-11-25 11:12:35 PST
Comment on attachment 574106 [details] [diff] [review]
Functionality Patch - (Tests Not Fixed)

Review of attachment 574106 [details] [diff] [review]:
-----------------------------------------------------------------

Your general approach looks good to me!  Please find my comments below.  Sorry that it took me so long to get back to you.  Please feel free to ping me on IRC if you need anything from me, because for the past few weeks I have been unable to keep up with the volume of my bugmail.  :-)

::: editor/txtsvc/public/nsIInlineSpellChecker.idl
@@ +67,5 @@
>  
>    nsIDOMRange getMisspelledWord(in nsIDOMNode aNode, in long aOffset);
>    void replaceWord(in nsIDOMNode aNode, in long aOffset, in AString aNewword);
>    void addWordToDictionary(in AString aWord);
> +  void removeWordFromDictionary(in AString aWord);

Please change the IID of nsInlineSpellChecker too.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +880,5 @@
> +  nsresult rv = mSpellCheck->RemoveWordFromDictionary(wordstr.get());
> +  NS_ENSURE_SUCCESS(rv, rv); 
> +
> +  mozInlineSpellStatus status(this);
> +  rv = status.InitForSelection();

Hmm, I'm not sure why you're only spell checking for the active selection...  Removing a suggested word could potentially change the result of spell checking on the entire document, right?

::: toolkit/content/InlineSpellChecker.jsm
@@ +40,5 @@
>  var gRegionBundle;
>  
>  function InlineSpellChecker(aEditor) {
>    this.init(aEditor);
> +  this.mAddedWordStack = []; //This must be persistant, thus initialized once

Nit: please add a trailing space after //.

@@ +283,5 @@
>  
>    // callback for adding the current misspelling to the user-defined dictionary
>    addToDictionary: function()
>    {
> +    this.mAddedWordStack.push(this.mMisspelling);

So, I think we want to limit the stack depth to 1 for now.  You could probably define a const at the top of the file named something like MAX_UNDO_STACK_DEPTH, and check for it here before pushing something on to the stack (and remove 1 element from the bottom of the stack before pushing it when you hit that limit.)

@@ +297,5 @@
> +    }
> +  },
> +  canUndo : function()
> +  {
> +    //Return true if we have words on the stack

Nit: same spacing nit as above.  :-)
Comment 42 :Ehsan Akhgari 2011-11-25 11:14:55 PST
(In reply to Quentin Headen from comment #39)
> I need some help on where I should add tests at for this enhancement.

I think your best bet is to add your tests to <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html?force=1>, because it already has most of the infrastructure that you need.  Also, please remember to add the context menu entry to textbox.xml as well, as Gavin pointed out in comment 35.

Thanks!
Comment 43 Quentin Headen [:qheaden] 2011-11-25 17:30:33 PST
Thanks a lot for the feedback Ehsan, and no problem with the late response. I've been ultra busy lately too, so I understand. :)

I'll get to work on this right away.
Comment 44 Quentin Headen [:qheaden] 2011-12-19 21:37:56 PST
Created attachment 583068 [details] [diff] [review]
Patch V2 (Almost Complete)

Here is the second version of my patch. I made all of the changes suggested in the feedback from version 1 of my patch. I also modified the context menu mochitest to test for the "Undo Add to Dictionary" menu element.

I still haven't implemented the code needed for the browser's XUL textbox element. That will be in my next patch. The next patch will be the feature complete one.
Comment 45 :Ehsan Akhgari 2011-12-21 14:29:27 PST
Comment on attachment 583068 [details] [diff] [review]
Patch V2 (Almost Complete)

Review of attachment 583068 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments fixed.  Please feel free to submit a new patch for the rest of the bug if you don't need me to look at this part again.  Thanks!

::: browser/base/content/test/test_contextmenu.html
@@ +467,5 @@
>          closeContextMenu();
>          openContextMenuFor(image_in_iframe); // Invoke context menu for next test.
>          break;
>  
> +    case 13:          

Nit: trailing spaces.

@@ +517,5 @@
>                           ].concat(inspectItems));
> +                        
> +        
> +        // Simulate a click on the "Add to Dictionary" button
> +        contextMenu.ownerDocument.getElementById("spell-add-to-dictionary").doCommand();

I wonder if you could simply use goDoCommand here...

::: toolkit/content/InlineSpellChecker.jsm
@@ +42,4 @@
>  
>  function InlineSpellChecker(aEditor) {
>    this.init(aEditor);
> +  this.mAddedWordStack = []; // This must be persistent, thus initialized once  

Please move this to the uninit function.  This way it would also be called as part of the init call above.

Also, I think the comment here is incorrect and needs to be removed.

@@ +285,5 @@
>    // callback for adding the current misspelling to the user-defined dictionary
>    addToDictionary: function()
>    {
> +    // Prevent the undo stack from growing over the max depth
> +    if(this.mAddedWordStack.length == MAX_UNDO_STACK_DEPTH)

Nit: space before parenthesis.

@@ +294,5 @@
>    },
> +  // callback for removing the last added word to the dictionary LIFO fashion
> +  undoAddToDictionary: function()
> +  {
> +    if(this.mAddedWordStack.length > 0)

Ditto.
Comment 46 Quentin Headen [:qheaden] 2011-12-21 16:17:01 PST
Created attachment 583666 [details] [diff] [review]
Patch v3

This is the final, feature-complete patch. This version added the changes and tests to the XUL textbox element so it can also have the ability to undo words from the personal spell check dictionary.
Comment 47 Quentin Headen [:qheaden] 2011-12-21 19:37:52 PST
Created attachment 583711 [details] [diff] [review]
Patch v3.1

I had to come out with a fix to the last patch. I discovered that moving the initialization of the mAddedWordStack array inside of the InineSpellChecker.uninit() method completely broke the undo add functionality. This is because uninit seems to be called every time the textbox is right clicked, thus clearing the stack before you can undo a word. This had to be initialized outside.
Comment 48 :Ehsan Akhgari 2011-12-23 17:09:43 PST
Comment on attachment 583711 [details] [diff] [review]
Patch v3.1

Oh you're right, sorry about the wrong tip!

This looks great, r=me.  If you have run this through try server, please mark it as checkin-needed.  Thanks!
Comment 49 Quentin Headen [:qheaden] 2011-12-24 02:37:17 PST
I guess green means go. Here are the try server results https://tbpl.mozilla.org/?tree=Try&rev=25cbc985905d
Comment 50 :Ehsan Akhgari 2011-12-29 13:08:09 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/314f940ca86e

Thanks a lot, Quentin!
Comment 51 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-30 05:02:21 PST
https://hg.mozilla.org/mozilla-central/rev/314f940ca86e
Comment 52 Matthew N. [:MattN] (PM me if requests are blocking you) 2012-01-30 20:45:46 PST
*** Bug 384234 has been marked as a duplicate of this bug. ***
Comment 53 neil@parkwaycc.co.uk 2012-05-27 09:28:43 PDT
Comment on attachment 583711 [details] [diff] [review]
Patch v3.1

>+  canUndo : function()
Why is this not get canUndo()?
Compare get canSpellCheck(), get enabled(), get overMisspelling().
(Also, space before colon is not normal JS object style.)
Comment 54 :Ehsan Akhgari 2012-05-31 16:11:19 PDT
(In reply to neil@parkwaycc.co.uk from comment #53)
> Comment on attachment 583711 [details] [diff] [review]
> Patch v3.1
> 
> >+  canUndo : function()
> Why is this not get canUndo()?
> Compare get canSpellCheck(), get enabled(), get overMisspelling().
> (Also, space before colon is not normal JS object style.)

Please file a follow-up bug.

Note You need to log in before you can comment on or make changes to this bug.