441930 - Page refresh of textarea disabled via Javascript fails to display content.

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Mike Walker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

If you have a page containing a <textarea> that has been disabled using a Javascript call to modify the HTML DOM, its contents are not loaded after you have refreshed the page (crtl-r). 

Textareas disabled using the static "disabled" attribute work fine, it's just those which have been disabled using the following Javascript call:

    document.getElementById(field).disabled = true;

Note: even though the page I set up to show the problem doesn't show this, the problem persists even after you enable the textarea control again. While the text contents remain present in the page source, nothing shows up on the web page.  This could be a serious problem for some web sites.

This works fine on IE 7.0 and FF 2.0.14 -- the content is loaded every time you hit refresh -- so this bug is a regression from FF 2.0.

Reproducible: Always

Steps to Reproduce:
1. Go to: http://englishmike.net/azindex-test-page/test-page-for-firefox-30-bug/
2. Note the content of the disabled textarea on the web page is displayed.
3. Hit Ctrl-R to refresh the page.
4. The textarea is now empty.
Actual Results:  
The content of the disabled textfield is no longer loaded, even though it is still in the page source (Ctrl-U)

Expected Results:  
As with FF 2.0, the content of the textfield should always be loaded after a refresh, even though the textfield is disabled.

This could have a serious affect on web sites that use Javascript to dynamically restrict access to certain fields by disabling them.  There could be a critical loss of data if the user refreshes the screen and does not notice that text has disappeared.  If the user then clicks save on that page, then sites that save data from all fields whether they are disabled or not could erroneous lose information.  Note that even when you enable the textarea again, the content does not reappear -- it is lost for good. 

I do not know of an easy workaround to this problem.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Regression range is:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-11-27+13%3A00&maxdate=2006-11-27+23%3A00
Could be caused by Bug 277724.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

nsHTMLTextAreaElement::RestoreState needs to check the rv from the first GetStateProperty call (compare it to NS_STATE_PROPERTY_EXISTS to be precise).

No tree here, but I can post a patch in a week and a half if no one else gets to it first.

Comment 4

[Johnny Stenback (:jst)]

bz, since you know what to do you'll get this one.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Basically, this code needs to be more like that for nsHTMLInputElement::RestoreState.

Comment 6

[Johnny Stenback (:jst)]

Reassigning to Benjamin Newman who'll take a look at this one.

Comment 7

[Ben Newman (:bnewman) (:benjamn)]

Attachment: proposed patch with mochitest case

Did as suggested.  One slight uncertainty: I use GetValue instead of GetValueInternal.  Thoughts?  Also, I hope you don't take offense to my hack for reloading the test page :)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 329903 [details] [diff] [review]
proposed patch with mochitest case

>@@ -948,7 +948,8 @@ nsHTMLTextAreaElement::RestoreState(nsPr
>   nsAutoString value;
>   nsresult rv =
>     aState->GetStateProperty(NS_LITERAL_STRING("value"), value);
>-  NS_ASSERTION(NS_SUCCEEDED(rv), "value restore failed!");
>+  if (rv != NS_STATE_PROPERTY_EXISTS)
>+    GetValue(value); // use default instead of nothing
>   SetValue(value);

That's not quite right, since we want to avoid making the SetValue call (which has side-effects!) if there was no property in the state.

This can be mochitested for:

1) create an input with a default value
2) reload
3) Change the input's defaultValue property
4) Verify that the .value changed in step 3.

I think your patch fails that test, no?  Instead, we should just put the SetValue() call in a |if (rv == NS_STATE_PROPERTY_EXISTS)| conditional, I think.

>+++ b/content/html/content/test/test_bug441930.html	Wed Jul 16 14:36:34 2008 -0700

Reloading the test page will likely confuse the test runner (did you run it in the test runner?).  Put the stuff to be reloaded in a subframe and reload that?  You can set a onload listener on the subframe right before calling reload() to get notified when it's done reloading.

This patch is missing makefile changes to add the test file.

Comment 9

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #8)
> (From update of attachment 329903 [details] [diff] [review])
> >@@ -948,7 +948,8 @@ nsHTMLTextAreaElement::RestoreState(nsPr
> >   nsAutoString value;
> >   nsresult rv =
> >     aState->GetStateProperty(NS_LITERAL_STRING("value"), value);
> >-  NS_ASSERTION(NS_SUCCEEDED(rv), "value restore failed!");
> >+  if (rv != NS_STATE_PROPERTY_EXISTS)
> >+    GetValue(value); // use default instead of nothing
> >   SetValue(value);
> 
> That's not quite right, since we want to avoid making the SetValue call (which
> has side-effects!) if there was no property in the state.

Ah, you're right.  I understood the problem to be a matter of passing the correct value to SetValue, but really it's a matter of preventing SetValue from clobbering the default value with an empty string when no value property has been saved.

> This can be mochitested for:
> 
> 1) create an input with a default value
> 2) reload
> 3) Change the input's defaultValue property
> 4) Verify that the .value changed in step 3.

Making sure I understand: you mean that changing the defaultValue here *should* change the value?  If that's the condition for success, then indeed my first patch failed.  (Not disagreeing, just making sure.)

Adding this test to the mochitest case (see second patch).

> I think your patch fails that test, no?  Instead, we should just put the
> SetValue() call in a |if (rv == NS_STATE_PROPERTY_EXISTS)| conditional, I
> think.

That's correct.

> >+++ b/content/html/content/test/test_bug441930.html	Wed Jul 16 14:36:34 2008 -0700
> 
> Reloading the test page will likely confuse the test runner (did you run it in
> the test runner?).  Put the stuff to be reloaded in a subframe and reload that?
>  You can set a onload listener on the subframe right before calling reload() to
> get notified when it's done reloading.

It actually didn't faze the test runner, maybe because the runner loads the test page itself in an iframe, but I agree that the subframe approach is cleaner, so I've rewritten the test.

> This patch is missing makefile changes to add the test file.

Doh!  See second patch.

Thanks for the speedy feedback, by the way.

Comment 10

[Ben Newman (:bnewman) (:benjamn)]

Attachment: patch iteration, additional test case

Comment 11

[Boris Zbarsky [:bzbarsky]]

> you mean that changing the defaultValue here *should* change the value?

Yep.  Basically, changing defaultValue should change the value unless either the user has typed in the control or the page has set .value in JS.

Thanks for having small, easy-to-review patches.  ;)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 329929 [details] [diff] [review]
patch iteration, additional test case

>+++ b/content/html/content/test/test_bug441930.html	Wed Jul 16 17:07:05 2008 -0700
>+<script class="testbody" type="text/javascript">
>+
>+/** Test for Bug 441930: see bug441930_iframe.html **/
>+
>+</script>

That assumes that onload for the parent won't fire until the second load of the subframe.  Might be better to add:

  SimpleTest.waitForExplicitFinish()

on this page before we parse the <iframe>, and then have the iframe directly call parent.SimpleTest.finish() when it's done with the test.

With that change, looks good.

Comment 13

[Ben Newman (:bnewman) (:benjamn)]

Attachment: commit candidate (added inter-frame signaling)

All set?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ben, you want the call before we parsethe iframe, like I said in comment 12.  Otherwise the timing might work out such the test will all run before waitForExplicitFinish() is called, leading to mysterious tinderbox orange.

Comment 15

[Ben Newman (:bnewman) (:benjamn)]

Attachment: commit candidate (waiting before iframe parsing)

Comment 16

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #14)
> Ben, you want the call before we parsethe iframe, like I said in comment 12. 
> Otherwise the timing might work out such the test will all run before
> waitForExplicitFinish() is called, leading to mysterious tinderbox orange.

I should have caught that from the other iframe test cases, not to mention reading your feedback more carefully... sorry.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 330068 [details] [diff] [review]
commit candidate (waiting before iframe parsing)

Looks great.  Thanks!

Comment 18

[Boris Zbarsky [:bzbarsky]]

I assume this got resolved by accident?

Comment 19

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #18)
> I assume this got resolved by accident?
> 

Yes, my mistake.  (Bugs are resolved only once they've been checked in?)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yes.

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/d96fcf124f97

Comment 22

[Carsten Book [:Tomcat]]

verified fixed in 1.9 B1 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1 and the testcase from martijn. 

-> Verified fixed

Comment 23

[Daniel Veditz [:dveditz]]

Is this reasonable to fix on 1.9.0.x, or even wanted there?

Comment 24

[Boris Zbarsky [:bzbarsky]]

I think it might be good to fix this on 1.9.0.x, yes.  It's a quite safe patch that fixes a regression from 1.8.

Comment 25

[Samuel Sidler (old account; do not CC)]

Ben, can you please create a 1.9.0 branch patch for this bug (and request review on it)?

Comment 26

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #25)
> Ben, can you please create a 1.9.0 branch patch for this bug (and request
> review on it)?

I was able to apply the same patch (with mochitests and all) to 1.9.0 just now, and the tests passed.  I used this command from the top-level mozilla directory: patch -p1 < bug441930-4.patch.txt

Comment 27

[Samuel Sidler (old account; do not CC)]

Comment on attachment 330068 [details] [diff] [review]
commit candidate (waiting before iframe parsing)

Nominating this patch for the 1.9.0 branch per comment 24 and comment 26.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 330068 [details] [diff] [review]
commit candidate (waiting before iframe parsing)

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 29

[Johnny Stenback (:jst)]

Fix landed in CVS.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre.