134911 - Changing browser charset encoding loses all form data

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[tao]

2002-03-26-03 commercial build: 
1. use browser to load bugzilla bug report.
2. enter comments in the additional comment area (text field)
3. go to "View|Charset" to change viewing charset, e.g., from Chinese to Western.

All the text entered disappear!

Comment 1

[Matthias Versen [:Matti]]

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

Comment 2

[Kevin McCluskey (gone)]

Reassigning to John

Comment 3

[Kevin McCluskey (gone)]

nsbeta1-. We reload the document when the charset changes. A patch for this
would be risky and time consuming.

Comment 4

[John Keiser (jkeiser)]

This is a real Internationalization bug, and while it looks like it's related to
bug 61363, I don't think it really is.  This shouldn't require changing the way
we reload charsets; it just means we have to trigger save/restore when this
happens.  Setting dependent on bug 166636 (Move Save/Restore out of Layout)
since the triggers are going to change there.

Comment 5

[Carine Held]

Reproduced in 1/03/03 Trunk, Win XP. Minimised test case attached.

Comment 6

[Carine Held]

Attachment: Minimised testcase

Comment 7

[Simon Montagu :smontagu]

Judging by bug 92473 comment 2, this is deliberate, but I don't understand why, and the testcases there are no longer available.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I just looked, and it looks to me that with the changes made in bug 108309 we can just back out the change made for bug 92473 without regressing it.  Someone should test, though.  Or at least provide a testcase for bug 92473 so I can test...

Comment 11

[Simon Montagu :smontagu]

I can test that, but not before Saturday night. Meanwhile I've attached a couple of testcases to bug 92473.

Comment 12

[Simon Montagu :smontagu]

Attachment: Patch

This backs out the change to nsDocShell.cpp from bug 92473, and indeed fixes this (and bug 336109) without regressing bug 92473. I deliberately didn't touch the changes to other files, which set the LOAD_FLAGS_CHARSET_CHANGE flag or pass it around.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 282960 [details] [diff] [review]
Patch

sr=me, but please try to write some tests if possible?

Comment 14

[Simon Montagu :smontagu]

"If possible" -- ay, there's the rub. I haven't made tests for bug 382074 and friends because I couldn't work out how to simulate an encoding change in any of our test frameworks. Suggestions are welcome.

Comment 15

[Boris Zbarsky [:bzbarsky]]

You can simulate an encoding change in the "browser" test framework by just running whatever JS runs when the user selects that menuitem...

Comment 16

[Simon Montagu :smontagu]

Comment on attachment 282960 [details] [diff] [review]
Patch

Yup, tests are good :) 
This only works for <textarea>s with no initial content, as in the attached testcase: all other form elements are reset to their initial value on changing charset.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Uh... that's really odd.  I wouldn't expect that to happen at all.  

Comment 18

[Simon Montagu :smontagu]

There seems to be a separate regression that a normal reload resets form elements :(

Comment 19

[Simon Montagu :smontagu]

... but not all the time. I haven't worked out exactly what are the circumstances where that happens.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Please make sure that's filed.  That should be a 1.9 blocker.  And sounds like you want to re-request review on this patch....

Comment 21

[Simon Montagu :smontagu]

Comment on attachment 282960 [details] [diff] [review]
Patch

Rerequesting for now. I'm still having some trouble making it work in the browser test environment, but it works fine manually in the cases where the reload bug doesn't kick in.

Comment 22

[Simon Montagu :smontagu]

Filed bug 398243 on the reset-on-reload bug.

Comment 23

[Simon Montagu :smontagu]

Attachment: Test

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 282960 [details] [diff] [review]
Patch

r=me as well.  Ask for approval, please.  I think we should take this.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Simon, are you going to land this once the tree reopens?

Comment 26

[Simon Montagu :smontagu]

Yeah, I already put it in the queue at http://wiki.mozilla.org/Firefox3/Beta2CheckinQueue#Non-Blockers

Comment 27

[Simon Montagu :smontagu]

Checked in.

Comment 28

[AndrewM]

Should Target Milestone be changed to Firefox 3 M10 since this will have made it in for Beta 2? :)

Comment 29

[Boris Zbarsky [:bzbarsky]]

This test is randomly failing.  See bug 503681