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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

--
critical
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

Tracking

({dataloss, testcase})

Trunk
mozilla1.9alpha1
x86
Windows XP
dataloss, testcase
Points:
---
Bug Flags:
blocking1.8.1 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 189234 [details]
testcase
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.
Keywords: helpwanted

Comment 3

13 years ago
See also bug 315997, "normalize truncates content of long textarea".
*** Bug 315997 has been marked as a duplicate of this bug. ***

Comment 5

13 years ago
We'd be interested in a fix for this for 1.8.1 since it impacts a web API.
Flags: blocking1.8.1+

Comment 6

13 years ago
No patch, not a regression, not a critical bug => not going to block FF2 beta1 for this.
Flags: blocking1.8.1+ → blocking1.8.1-
Created attachment 252032 [details] [diff] [review]
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.
Attachment #252032 - Flags: superreview?(jonas)
Attachment #252032 - Flags: review?(jonas)
(Assignee)

Updated

12 years ago
Assignee: nobody → bzbarsky
Keywords: helpwanted
Summary: normalize() function on textarea removes text that was appended with dom method → [FIX]normalize() function on textarea removes text that was appended with dom method
Target Milestone: --- → mozilla1.9alpha
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.
Attachment #252032 - Flags: superreview?(jonas)
Attachment #252032 - Flags: superreview+
Attachment #252032 - Flags: review?(jonas)
Attachment #252032 - Flags: review+
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.  ;)
Attachment #252032 - Flags: superreview-
Attachment #252032 - Flags: superreview+
Attachment #252032 - Flags: review-
Attachment #252032 - Flags: review+
Created attachment 252176 [details] [diff] [review]
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.  ;)
Attachment #252032 - Attachment is obsolete: true
Attachment #252176 - Flags: superreview?(jonas)
Attachment #252176 - Flags: review?(jonas)
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.
Attachment #252176 - Flags: superreview?(jonas)
Attachment #252176 - Flags: superreview+
Attachment #252176 - Flags: review?(jonas)
Attachment #252176 - Flags: review+
> 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.  ;)
Created attachment 253288 [details] [diff] [review]
Updated to comments
Attachment #252176 - Attachment is obsolete: true
Fixed.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Created attachment 253290 [details] [diff] [review]
Make it build too
Created attachment 253291 [details] [diff] [review]
Make it build harder -- forgot a file.  :(

Comment 18

12 years ago
Could anybody enlighten me as how this being fixed translates to a Firefox release? Will this be in the next FF release?
(Reporter)

Comment 19

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

Comment 20

12 years ago
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

12 years ago
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.
Severity: normal → critical
Keywords: dataloss

Comment 22

12 years ago
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.
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.  ;)

Updated

12 years ago
Duplicate of this bug: 381403
You need to log in before you can comment on or make changes to this bug.