418874 - undo function does not work in emptyText textboxes (e.g. Firefox search field, URL bar)

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Elmar Ludwig]

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: [e-mail]]]

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

Comment 2

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

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

Comment 3

[Dão Gottwald [:dao]]

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: [e-mail]]]

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

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Screenshot of trunk Minefield's search toolbar context-menu under Ubuntu 7.10

Comment 7

[Stephen Donner [:stephend] Not actively reading bugmail]

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 9

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

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

Comment 10

[Adam Kowalczyk]

I can reproduce this on Windows on a clean profile.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

[(no longer active)]

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: [e-mail]]]

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

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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]]

This bug covers all textboxes with emptyText.

Comment 16

[Graeme McCutcheon [:graememcc]]

Attachment: 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: [e-mail]]]

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]]

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]]

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]]

Attachment: 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]]

(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]]

> 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]]

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: [e-mail]]]

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]]

http://hg.mozilla.org/mozilla-central/rev/91a98e404def

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 27

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8270ef907e9e

Comment 28

[Graeme McCutcheon [:graememcc]]

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

Comment 29

[Daniel Holbert [:dholbert]]

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

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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]]

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]]

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)]

Failed again today:

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