Last Comment Bug 418874 - undo function does not work in emptyText textboxes (e.g. Firefox search field, URL bar)
: undo function does not work in emptyText textboxes (e.g. Firefox search field...
Status: VERIFIED FIXED
: regression, verified1.9.1
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: 1.9.0 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.2a1
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
: 423158 (view as bug list)
Depends on: 472302 472515 477631
Blocks: 406095 463202
  Show dependency treegraph
 
Reported: 2008-02-21 11:50 PST by Elmar Ludwig
Modified: 2009-04-06 00:06 PDT (History)
16 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of trunk Minefield's search toolbar context-menu under Ubuntu 7.10 (138.48 KB, image/png)
2008-02-21 17:15 PST, Stephen Donner [:stephend]
no flags Details
Trivial patch (1.13 KB, patch)
2008-12-28 15:19 PST, Graeme McCutcheon [:graememcc]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch with mochitest (5.59 KB, patch)
2008-12-29 16:57 PST, Graeme McCutcheon [:graememcc]
gavin.sharp: review+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description Elmar Ludwig 2008-02-21 11:50:55 PST
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
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-02-21 12:13:19 PST
Seems likely to be related to the changes made in bug 406095 (bug 406095 comment 24, second last paragraph).
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-02-21 12:14:15 PST
That being said, I can't reproduce with a nightly on Windows.
Comment 3 Dão Gottwald [:dao] 2008-02-21 12:19:45 PST
works for me too.

Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-02-21 16:38:03 PST
Perhaps it's only a problem on Linux, due to some strange focus issue?
Comment 5 Dão Gottwald [:dao] 2008-02-21 16:59:54 PST
I'll test that, can't do it right now.
Comment 6 Stephen Donner [:stephend] 2008-02-21 17:15:09 PST
Created attachment 304870 [details]
Screenshot of trunk Minefield's search toolbar context-menu under Ubuntu 7.10
Comment 7 Stephen Donner [:stephend] 2008-02-21 17:16:15 PST
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
Comment 8 Dão Gottwald [:dao] 2008-03-16 01:06:30 PDT
*** Bug 423158 has been marked as a duplicate of this bug. ***
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-16 01:10:01 PDT
Bug 423158 was filed against windows, but I still can't reproduce on Windows. Are any addons involved?
Comment 10 Adam Kowalczyk 2008-03-16 04:57:56 PDT
I can reproduce this on Windows on a clean profile.
Comment 11 Henrik Skupin (:whimboo) 2008-08-26 15:48:46 PDT
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.
Comment 12 :Ehsan Akhgari 2008-11-06 11:57:03 PST
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
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-12-26 22:07:51 PST
That change would still be valuable as an experiment, though, to see whether it's actually at fault.
Comment 14 Henrik Skupin (:whimboo) 2008-12-27 11:03:08 PST
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?
Comment 15 Dão Gottwald [:dao] 2008-12-27 11:10:01 PST
This bug covers all textboxes with emptyText.
Comment 16 Graeme McCutcheon [:graememcc] 2008-12-28 15:19:56 PST
Created attachment 354633 [details] [diff] [review]
Trivial patch

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 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-12-28 15:29:01 PST
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 18 Dão Gottwald [:dao] 2008-12-29 00:54:48 PST
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.
Comment 19 Dão Gottwald [:dao] 2008-12-29 01:06:20 PST
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.
Comment 20 Graeme McCutcheon [:graememcc] 2008-12-29 16:57:08 PST
Created attachment 354761 [details] [diff] [review]
Patch with mochitest

> 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...
Comment 21 Dão Gottwald [:dao] 2008-12-30 02:44:35 PST
(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?
Comment 22 Graeme McCutcheon [:graememcc] 2008-12-30 11:37:37 PST
> 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.
Comment 23 Graeme McCutcheon [:graememcc] 2008-12-30 12:01:33 PST
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 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-12-30 14:43:51 PST
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 25 Dão Gottwald [:dao] 2008-12-31 02:41:59 PST
http://hg.mozilla.org/mozilla-central/rev/91a98e404def
Comment 26 Mike Beltzner [:beltzner, not reading bugmail] 2009-01-03 00:32:53 PST
Comment on attachment 354761 [details] [diff] [review]
Patch with mochitest

a191=beltzner
Comment 29 Daniel Holbert [:dholbert] 2009-01-22 00:00:50 PST
Failed again today:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232605415.1232610668.12421.gz
Comment 30 Henrik Skupin (:whimboo) 2009-02-02 08:28:23 PST
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.
Comment 31 Mike Beltzner [:beltzner, not reading bugmail] 2009-03-02 21:44:45 PST
The test failed again today:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1236051486.1236057120.27846.gz
Comment 32 Mike Beltzner [:beltzner, not reading bugmail] 2009-03-02 21:46:51 PST
The sporatic mochitest failure is causing a lot of problems. We should have it disabled, backed out, or rewritten; strong preference for the latter.
Comment 33 cajbir (:cajbir) 2009-03-07 22:08:53 PST
Failed again today:

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

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