471319 - undoing the last action of a textbox immediately after emptytext was displayed inserts a line feed (breaks search textbox)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081227 Minefield/3.2a1pre ID:20081227020350

Using a search widget with the emptyText attribute set, will be broken when you initially select the undo entry within the context menu. The clear button gets hidden and no results are returned even without a filter set (value="").

As Dao mentioned this bug could probably be fixed by a patch on bug 418874 but I feel safer in having it as it's own bug. A comment can get lost but we will remember that bug. So marking it as depended.

Here are the STR:
1. Open "Tools | Add-ons"
2. Open the context menu of the search text box under "Get Add-ons" and select Undo
3. Focus the text box and hit enter

=> No results are returned

1. Open the preferences and go to the applications pane
2. Open the context menu of the search text box and select Undo

=> The clearButton gets hidden and no MIME types are listed anymore. You have to reopen the pref panel to be able to search again.

Comment 1

[Dão Gottwald [:dao]]

(In reply to comment #0)
> As Dao mentioned this bug could probably be fixed by a patch on bug 418874

No, I was confused by the fact that you commented in that bug. This doesn't seem to be related.

Comment 2

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

Ok. Now that you have changed the summary I have to add one more hint. It only appears for the latest undo action. If you are changing the value several times and start searches, it will still work if you are undoing a value change. But when undoing the last change it will break. That's the same as when you hit undo on the initial state.

Comment 3

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

And if you compare the given examples above by using the about:config search field, you can see it does only happen on search widgets where emptyText is set. So it is related in some way.

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> And if you compare the given examples above by using the about:config search
> field, you can see it does only happen on search widgets where emptyText is
> set.

That's only partly true -- in about:config, you still get the clear icon instead of the search glass.
Do you want this bug to cover that issue?

Comment 5

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

Due to instant searching we don't have the search glass at all. But you are right. If you think its better to file a new bug on that part, I can create one. Otherwise we can cover both things here. It's up to you.

Comment 6

[Dão Gottwald [:dao]]

Yeah, a new bug is better, since the underlying issue is different.

Comment 7

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

Filed as bug 471330.

Comment 8

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1

When the editor is empty it inserts a bogus <br> node as a placeholder.

Now, the textbox creates a batch to ensure the user can't undo the displaying of the empty text. So, what's happening is this:

1) Bogus <br> node inserted
2) TRANSACTION STARTS
3) Bogus <br> node deleted
4) textbox's emptytext inserted
{user clicks on textbox or it gets focus in some other way}
5) textbox's emptytext deleted
6) Bogus <br> node inserted
7) TRANSACTION ENDS

So, when doing or undoing this transaction, there is a bogus <br> node at each endpoint of the transaction.

The problem arises because the plaintext editor's undo/redo handling code doesn't handle this case. It assumes that if there was a bogus node before undoing a transaction, then there won't be one after undoing the transaction. As the above shows, when using the transaction manager's batching facilities this is not necessarily true.

Comment 9

[Graeme McCutcheon [:graememcc]]

Of course, with Henrik's STR, undo shouldn't even be enabled in the context menu at that point... filed as bug 471776

Comment 10

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

Peterv, if you would have time for a review that would be great.

Comment 11

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

Who else could review the patch? Seems like there is no way to reach Peter right now.

Comment 12

[Graeme McCutcheon [:graememcc]]

In fact, you don't even need to mess with transactions to hit this bug. Bug 328056 is basically the same problem.

Comment 13

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

Graeme, does it mean bug 328056 will be also solved by your patch?

Comment 14

[Graeme McCutcheon [:graememcc]]

Should be, yes.

Comment 15

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v2 - slightly better tests

So to clarify, the problem is that nsTextEditRules::Did[Un/Re]do assumes if there is a bogus node before, there isn't after, and vice versa. This assumption can be false for two reasons: 1) Transactions, and 2) the Can[Un/Re]do check in nsEditor may return false, and so the editor may do nothing.

Perhaps in addition to this, it may be worth nsPlaintextEditor etc performing a CanUndo check, and not pre/post notifying the rules if the action cannot succeed. This would have to be implemented consistently for all the actions where this is the case (cut etc).

Comment 16

[timeless]

Comment on attachment 387672 [details] [diff] [review]
Patch v2 - slightly better tests

you don't have to address any of these comments. and they're only about the test. i'm commenting since i feel obligated to comment about the entire patch, not just the code fix (thanks for working on editor by the way, it needs the love).

so, most of our code uses "recognize" (hits: 20:1, files: 14:1)

>+        if (t1 instanceof Components.interfaces.nsIDOMNSEditableElement)
>+          t1Editor = t1.editor;
>+
>+        // Did the editor recognise the new bogus node?
>+        t1Editor.undo(1);

this style of doing instanceof and then only conditionally setting the next variable that you need seems odd.

I'd rather (but this is up to you):
t1.QueryInterface(Components.interfaces.nsIDOMNSEditableElement);
t1Editor = t1.editor;
...

Otherwise you get an exception later (t1Editor.undo(1)) and have to backtrack and realize that implicit QI failed

>+        // Test 2: Redo on an empty editor should not result in the bogus node
>+        // not being recognised as such

... should not result in editor forgetting that its bogus node is bogus

(double negatives are painful for human parsers)

Comment 17

[timeless]

neil suggests:

... should not forget about the bogus node

Comment 18

[Peter Van der Beken [:peterv]]

I think we really should fix the problems the right way and remove these "bogus" nodes instead of adding hacks upon hacks to deal with them (I personally think we should not take any more of these patches). Same for bug 318065 and bug 483651.

Comment 19

[Graeme McCutcheon [:graememcc]]

I certainly have no problem with the desire to get the real fix of killing the bogus node etc. As I'm sure they'll attest, after a thread in dev.planning where bz and roc talked about this and noted it would be unlikely anyone in MoCo could be assigned to it any time soon, I emailed them about one day wanting to attempt such a fix myself (although I made it somewhat non-commital - the amount of free time I have to volunteer being the limiting factor as to whether I would ever be able to spend enough time to do this). I'm not thoughtlessly layering on additional hacks without having thought about the wider problem.

318065 and 483651 could reasonably classified as hacks upon hacks, but I'd say this is really a logic-correctness fix. That of course could just be a too proud patch author quibbling over semantics. And given no definite timescale for a real fix, I wonder how much taking this hurts us versus having to hope that until we do fix it our users never thoughtlessly press Ctrl-Z in an empty HTML input. Yes, ultimately it's a workaround, but for a pretty general case, and I don't see that taking it significantly increases the maintenance overhead on what is already ill-maintained code.

Anyway, I'll attach the patch I would have pushed with the comments are addressed: I'll leave it for others to decide whether it makes it in the tree or not.

Comment 20

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v3 - comments addressed

Comment 21

[Peter Van der Beken [:peterv]]

(In reply to comment #19)
> I'm not
> thoughtlessly layering on additional hacks without having thought about the
> wider problem.

I never implied that.

> I don't see that taking it significantly increases the maintenance overhead
> on what is already ill-maintained code.

You're assuming there won't be any regressions from this fix, I hope you're right. Note that I'm not questioning the quality of your patches, but we're starting to have a history of regressions with this code and I'm just trying to raise that I think that points to the fragility of the architecture and we need to fix it.

Comment 22

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v4 - updated to trunk, change test to chrome test

Re-requesting review, primarily to check Ehsan is OK with taking this.

Since the original patch went up, use of enablePrivilege has been discouraged for new tests, so have switched the test to a chrome test.

Comment 23

[(no longer active)]

Comment on attachment 581921 [details] [diff] [review]
Patch v4 - updated to trunk, change test to chrome test

Peter's comments on us needing to fix the bogus node mess withstanding, I think not taking this fix will do more harm than good.  So, r=me.

Comment 24

[(no longer active)]

Graeme, do you need help in sending this patch to try and landing it?  If yes, please let me know and I'll help you.  :-)

Comment 25

[Graeme McCutcheon [:graememcc]]

(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> Graeme, do you need help in sending this patch to try and landing it?  If
> yes, please let me know and I'll help you.  :-)

Somehow never spotted the r+ bugmail!

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac1cbff2a67

Comment 26

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0ac1cbff2a67