All text fields in Firefox can undo changes to the text, except for the toolbar search field. Undo should be enabled there as well. 1. Type or paste something into the toolbar search field. 2. Press Ctrl+Z or right click and select "Undo" from context menu. In step 2, Ctrl+Z does nothing and the menu item is greyed out (both the item on the context menu and in the Edit menu). Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022004 Minefield/3.0b4pre ID:2008022004

Seems likely to be related to the changes made in bug 406095 (bug 406095 comment 24, second last paragraph).

That being said, I can't reproduce with a nightly on Windows.

works for me too. Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre

Perhaps it's only a problem on Linux, due to some strange focus issue?

I'll test that, can't do it right now.

I can confirm with Ubuntu 7.10 using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre

Bug 423158 was filed against windows, but I still can't reproduce on Windows. Are any addons involved?

I can reproduce this on Windows on a clean profile.

Yes, its reproducible on each platform with some simple steps: 1. Open Firefox 2. Click into the search field (don't use Cmd/Ctrl+K!!!) 3. Enter some words 4. Open the context menu This happens only if the cursor is placed by a left mouse click into the box. But the undo function is available when using the keyboard shortcut. The regression window is: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021704 Minefield/3.0b4pre Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021804 Minefield/3.0b4pre Bonsai checkins within this timeframe: http://tinyurl.com/5ccgzf It doesn't look like that this bug is a regression from Dao's patch on bug 406095. But no idea which one of the list could have been raised this one.

FWIW, this happens for any textbox control with the emptyText property, most notably the location bar on trunk. IRC conversation with gavin which is relevant: <ehsan> gavin_: any idea of a possible fix? <gavin|> well, reverting the code from the second last paragraph of https://bugzilla.mozilla.org/show_bug.cgi?id=406095#c24 would be a good first attempt <ehsan> hmm, I'm not familiar with that code would it be good enough to whip up a patch and ask for review? :) <gavin|> well, that would probably regress bug 338168 <ehsan> bummer

That change would still be valuable as an experiment, though, to see whether it's actually at fault.

It's more annoying for any instance of a search widget which is using the emptyText attribute and is doing an immediate search, e.g. within the application pane of the preferences panel. Right click into the text box while the empty text is visible and select Undo from the context menu. No entry is shown anymore even after doing an additional search and hitting the x button. Dao, shall I file a new bug on that or can it be fixed with a patch on this bug?

This bug covers all textboxes with emptyText.

Trivial patch. When constructing a textbox with empty text, both the constructor and emptyText attribute setting code were calling _updateVisibleText, and hitting the code to begin a new batch, thus mucking up the undo/redo stack.

Comment on attachment 354633 [details] [diff] [review] Trivial patch Thanks a bunch for looking into this! Any chance I could convince you to write a test? :) Why is the emptyText setter being called, out of curiosity? nit: I'd leave the empty line you're removing.

Comment on attachment 354633 [details] [diff] [review] Trivial patch Sure that this makes a difference? There's already 'if (textbox.hasAttribute("empty"))' in the timeout callback.

I was confused -- you're testing for _not_ this.hasAttribute(...), which makes sense of course. (In reply to comment #17) > Why is the emptyText setter being called, out of curiosity? The search bar sets emptyText in updateDisplay() via init(). The value setter also calls _updateVisibleText for empty vals.

> nit: I'd leave the empty line you're removing. Meh. That shouldn't have slipped in there, and I should have caught it. To my surprise and delight, this was time-consuming because I couldn't get my test to fail (I more often have the opposite problem!) - specifically, I struggled to get it to fail against trunk when I was checking the test was indeed testing the problem. In the end, I found I had to write this as a chrome test - I originally intended to append my tests to the existing test_textbox_emptytext.xul. However, when I did that, I eventually discovered that any attempt within textbox.xml to touch the editor property was failing with NS_ERROR_DOM_SECURITY_ERR. Not sure if that was my misuse of XUL or a genuine bug. > + t1.editor.canUndo(t1Enabled, t1CanUndo); Yuck. Alex Vincent's XPCOM Cheat Sheet states that the last out parameter in the IDL definition becomes the return value of the call in js, but this seemed to return undefined...

(In reply to comment #20) > In the end, I found I had to write this as a chrome test - I originally > intended to append my tests to the existing test_textbox_emptytext.xul. Does netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect") help?

> Does netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect") > help? Alas no. Leaving my patch for this bug aside, on trunk if you drop a dump in the catch block of _updateVisibleText in textbox.xml to spit out the error message, and run the test_textbox_emptytext.xul mochitest, you will see that construction of the textbox with id t1 hits the security error. This makes me think that the textbox widget isn't playing well on non-elevated XUL pages.

Based on bug 321879 comment 4, it looks like I was running into bug 59701. Can we live with a chrome test for the moment? Whilst it would still pass living in test_textbox_emptytext.xul, it would be passing because of the failures in textbox.xml and not because it's the right fix!

Comment on attachment 354761 [details] [diff] [review] Patch with mochitest Yeah, a chrome test is fine. I think all of the widget tests should be chrome tests, really, but that's a separate issue. Thanks again!

Comment on attachment 354761 [details] [diff] [review] Patch with mochitest a191=beltzner

The test is failing randomly on linux. For example, http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231077828.1231081560.13121.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1231049029.1231053730.955.gz

Failed again today: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232605415.1232610668.12421.gz

Verified on trunk and 1.9.1 with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre ID:20090201020604 Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090201 Minefield/3.2a1pre ID:20090201164324 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090202 Shiretoko/3.1b3pre Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre ID:20090201033606 (In reply to comment #29) > Failed again today: > http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232605415.1232610668.12421.gz I cannot see failures anymore. Seems that the test is processed successfully now.

The test failed again today: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1236051486.1236057120.27846.gz

The sporatic mochitest failure is causing a lot of problems. We should have it disabled, backed out, or rewritten; strong preference for the latter.

Failed again today: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236486607.1236491401.19948.gz