300691 - [FIX]normalize() function on textarea removes text that was appended with dom method

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Martijn Wargers (dead)]

Now that bug 152329 is fixed, I see another bug.

See upcoming testcase.
When clicking multiple times on the first button and then on the second (the
"normalize" button), the last added "text2" is removed.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

The problem is the same as that of changing the text of a textnode child of a textarea directly.  The issue is that the textarea doesn't see a change.

I thought we had a bug on that already, but I don't see one.

Comment 3

[Jesse Ruderman]

See also bug 315997, "normalize truncates content of long textarea".

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 315997 has been marked as a duplicate of this bug. ***

Comment 5

[Darin Fisher]

We'd be interested in a fix for this for 1.8.1 since it impacts a web API.

Comment 6

[Darin Fisher]

No patch, not a regression, not a critical bug => not going to block FF2 beta1 for this.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

I copied the script code, and then couldn't get it to work for a bit... ;)

I'll add some regression tests for both scripts and textareas.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 252032 [details] [diff] [review]
Proposed fix

If we end up doing this in many places we could add a class similar to nsRunnableMethod that can be reused for all callsites.

r/sr=me either way.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 252032 [details] [diff] [review]
Proposed fix

Actually, this breaks typing in textareas, because we end up calling Reset() on the anonymous content mutations triggered by the editor....

I _really_ need to write some regression tests here.  ;)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix that actually works

Now I can actually type too, and we have regression tests for that, as well as for the script issue that the patch fixes.  ;)

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 252176 [details] [diff] [review]
Fix that actually works

The name and return values from IsAnonymousIfDescendant are a bit non-intuitive. How about calling the function IsInSameAnonymousTree or something similar instead? That would make it more clear what the function does and what it returns.

I'm not following the logic for script-test 4. The test says "Shouldn't execute whitespace-only script on child append", and yet you're relying on that the whitespace-only script has executed and prevents. Same thing for test 6.

I'm a little bit hesitant that the behaviour of test 15 is something that is set in stone (this is in fact true for many of the tests to a varying degree). But we can always change the test if we decide to change things.

r/sr=sicking with that fixed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> How about calling the function IsInSameAnonymousTree

Hmm...  That might work, ok.

> The test says "Shouldn't execute whitespace-only script on child append"

The point is that we have executed it before, so we should't execute when the append happens.  I'll make this clearer.

> But we can always change the test if we decide to change things.

Yeah.  I was just writing tests to test existing behavior so if we change something inadvertently we can notice and then decide whether we want the change.  ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Make it build too

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Make it build harder -- forgot a file. :(

Comment 17

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests&command=DIFF_FRAMESET&file=reftest.list&rev1=1.27&rev2=1.28&root=/cvsroot

Comment 18

[Bernd Kuemmerlen]

Could anybody enlighten me as how this being fixed translates to a Firefox release? Will this be in the next FF release?

Comment 19

[Martijn Wargers (dead)]

This will be in Firefox3.
This will not be in any of the releases of Firefox2.

Comment 20

[Bernd Kuemmerlen]

The problem I see here: Bug 315997 which has been resolved as duplicate of this one was flagged "critical" with keyword "dataloss" (in contrast to comment 6). Could/would this trigger a merge of this fix to FF2?

Comment 21

[Jesse Ruderman]

The "dataloss" keyword does not automatically trigger merges to stable branches.  That's usually decided by the person who wrote the patch along with branch drivers, who consider things like the number of users affected by the bug and the size+riskiness of the patch.

Comment 22

[Bernd Kuemmerlen]

OK, thanks for adjusting keywords and severity here. Is there a way to request a merge? I don't understand how the mozilla blocking-flags relate to other branches.

Comment 23

[Boris Zbarsky [:bzbarsky]]

You'd set the appropriate blocking flags to the '?' value.  The 1.8.1.x flags are for Firefox 2, the 1.8.0.x flags are for Firefox 1.5.

That said, there's no way to safely merge this to branches in sane amounts of time, it's been a problem ever since Gecko existed, and it's not a security issue.  So I doubt the branch drivers would decide to block on it, and if they do decide to do so I'm not sure who'd work on it.

I personally don't think this is branch material.

For what it's worth, I also don't think that "critical" is the right severity here.  ;)