If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Disabled state of form control persists across page reload

VERIFIED FIXED in Firefox 53

Status

()

Core
DOM: Core & HTML
P3
normal
VERIFIED FIXED
7 years ago
7 months ago

People

(Reporter: Jason Heeris, Assigned: timdream)

Tracking

({DevAdvocacy})

Trunk
mozilla53
DevAdvocacy
Points:
---

Firefox Tracking Flags

(platform-rel -, firefox53 verified)

Details

(Whiteboard: DUPEME[DevRel:P2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
str
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.1 (KHTML, like Gecko) Chrome/6.0.422.0 Safari/534.1
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100501 Iceweasel/3.5.9 (like Firefox/3.5.9)

If I disable a button using Javascript and then click "reload," the button remains disabled (shift + reload seems to re-enable it though). I've tried it in Opera and Chrome (but not IE) and when I reload the page, the button is re-enabled.

I will attach a test page.


Reproducible: Always

Steps to Reproduce:
1. Open up the test page.
2. Click "submit"
3. Reload the page (using toolbar, Ctrl+R, F5, etc)
Actual Results:  
"Submit" button remains disabled.

Expected Results:  
"Submit" button should become enabled.
(Reporter)

Comment 1

7 years ago
testcase
Created attachment 450029 [details]
Simple XHTML page exhibiting the problem.
I just ran into this in 4.0b5. The JS-set properties persist even after turning JS off, which is how I noticed it. A shift-reload or a press of Return in the address bar clears things up.

Updated

6 years ago
Version: unspecified → 3.5 Branch

Comment 3

4 years ago
Reproducible on:
Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20130724 Firefox/25.0
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: General → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
Version: 3.5 Branch → Trunk
I very much doubt that this is a bug at all: it's session restore restoring the form's value on reload.

Moving over to the session restore component for verification of that assumption.
Assignee: general → nobody
Component: JavaScript Engine → Session Restore
OS: Linux → All
Product: Core → Firefox
Hardware: x86_64 → All
Is the disabled-ness of a control really part of form value?
This has nothing to do with session restore; this is form state restoration.

The disabled state of a control is indeed a part of form state restoration at the moment, because it often depends on the values of other controls, which are also getting restored...

In any case, there are existing bugs with lots of discussion about the behavior and its pros and cons.
Component: Session Restore → DOM: Core & HTML
Product: Firefox → Core
Summary: Javascript DOM changes persist across page reload → Disabled state of form control persists across page reload
Whiteboard: DUPEME
I would argue this is a bug that breaks developer workflow.

(Please remove DevAdvocacy keyword if that's that something I should add by myself)
Keywords: DevAdvocacy
(In reply to Boris Zbarsky [:bz] from comment #6)
> In any case, there are existing bugs with lots of discussion about the
> behavior and its pros and cons.

If someone new to this area of code wanted to find and link to these bugs, that would be helpful.
Whiteboard: DUPEME → DUPEME[DevRel:P2]
Flags: platform-rel?
platform-rel: --- → ?
platform-rel: ? → -
Priority: -- → P3
I've been digging in searchfox.org looking for reason why we are keeping disabling state in the nsPresState. So far I found bug 277724 on implementing the feature on textarea, select, and button, but not anything order than that. Blame info in HTMLInputELement.cpp was paved over by refactor in bug 867950 (and I suspect the actual feature was implement in Netscape days without a bug number -- bz would probably know?).

Reading nsPresState.h I found it hard to imagine why disable state is something that should be kept among other properties (content/resolution/scroll position etc.) So I guess can't answer Andrew's question in comment 8 successfully.

That said, removing this feature is probably a very easy work even for C++ newbie like me :) It would be simply involve identify the function and call sites in searchfox, remove them, make a build on try and remove invalid tests (if any).

If we could just get an agreement on this is a bug, not a feature ...
OK, with multiple clicks of "Show annotated diff" I've managed to find bug 218297.

So the original test case there (attachment 130904 [details]) is about selected item state (Disabled: [Yes/No]) and the text input disabling state. Martijn did correctly made a case there where the bug would be a non-issue if trigger by forward/back since bfcache will cover that case.

Cmd+R the page indeed keeping the drop down at [Yes] and text input disabled, and a Shift+Cmd+R reload indeed reset them to [No] and enabled. And (speak the time of 2016) I can indeed achieve state mismatch in Chrome with the back button.

Instead of removing the feature, I wonder if we want to find out the reason of mapping the keeping-the-state to Cmd+R and must-clean-state to Shift+Cmd+R ? How about let's always clean the state with Cmd+R as user explicitly want to "start over"?
Cmd+R is often used by users for "this page is not working right, let's try to fix it up".  Losing form state on that operation would be quite undesirable dataloss.  This certainly applies to things like textarea values especially, but also combobox values, etc.

Shift+Cmd+R is the "just start over" thing.  That's why it clears the form state...
(In reply to Boris Zbarsky [:bz] from comment #11)
> Cmd+R is often used by users for "this page is not working right, let's try
> to fix it up".  Losing form state on that operation would be quite
> undesirable dataloss.  This certainly applies to things like textarea values
> especially, but also combobox values, etc.

Except that Cmd+R also clean up the JS context and dynamically-injected DOM etc., so part of the data is always lost and the state between everything are mismatched. Why are we arbitrarily restoring part of DOM state and not the rest of the DOM?
Because when Cmd+R behavior was designed and user expectations around it were formed there wasn't much dynamically-injected stuff...

Also, for the common "some of the scripts failed to load the first time, so try again" use case restoring the DOM as it was _without_ those scripts is rather not the point.  The point is to try avoiding user dataloss as much as we can; making the best of a bad situation.
(In reply to Boris Zbarsky [:bz] from comment #13)
> Because when Cmd+R behavior was designed and user expectations around it
> were formed there wasn't much dynamically-injected stuff...

Right, which means we would probably reach a different design if we are looking at the Web of 2016....

> Also, for the common "some of the scripts failed to load the first time, so
> try again" use case restoring the DOM as it was _without_ those scripts is
> rather not the point.  The point is to try avoiding user dataloss as much as
> we can; making the best of a bad situation.

If so, I would argue we should just remove the disabled states in nsPresState, since the goal here is not about restoring DOM state as much as possible, but only about avoid user dataloss. The disable state is hardly user data.
> which means we would probably reach a different design if we are looking at the Web of 2016....

Possibly, yes.  But note that we have to consider user expectations, not just website behavior.

> since the goal here is not about restoring DOM state as much as possible, but only about avoid user dataloss.

"user dataloss" means "user can no longer submit the partially filled-in form", yes?

The goal is to put the page back into a state where the user _can_ finish editing the form and submit it.  For example, if some textarea started off disabled, then the user changed a combobox which made it enabled, then the user typed part of a long message in there, then a reload happened... ideally the user can keep typing that message and then send it.

I agree that we can't always do this right.  The question, again, is what behavior will help in the largest number of cases.  No matter what we do there will be some edge cases that fail... we just want them to be rarer than the cases that succeed.
Looks like chrome, safari, and edge all reset the form. I would be in favor of having the same behavior as the other browsers.

Boris, how would you measure if this is actually helpful to users?
Flags: needinfo?(bzbarsky)
I have no idea how to measure this, short of just trying to reload on various websites that people commonly use and seeing how it behaves.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #15)
> > since the goal here is not about restoring DOM state as much as possible, but only about avoid user dataloss.
> 
> "user dataloss" means "user can no longer submit the partially filled-in
> form", yes?
> 
> The goal is to put the page back into a state where the user _can_ finish
> editing the form and submit it.  For example, if some textarea started off
> disabled, then the user changed a combobox which made it enabled, then the
> user typed part of a long message in there, then a reload happened...
> ideally the user can keep typing that message and then send it.
> 
> I agree that we can't always do this right.  The question, again, is what
> behavior will help in the largest number of cases.  No matter what we do
> there will be some edge cases that fail... we just want them to be rarer
> than the cases that succeed.

Re-read this and I have a quick idea: how about, let's only restore the disable state when the state is "not disabled" and the state of the document when loaded is "disabled"? For the reverse state we don't do restore it. To rephrase, upon reload, we would re-enable a from control, but we won't re-disable a form control.

Yes, we can't always do this right, but at least we could do it in a way that won't be too annoy.
Flags: needinfo?(bzbarsky)
That doesn't seem unreasonable.
Flags: needinfo?(bzbarsky)
Assignee: nobody → catalin.badea392
Comment hidden (mozreview-request)
I don't know what tests to fix, or if is any.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbdf39440aceece4102ec523332fcd1388f24d7e
Comment on attachment 8822616 [details]
Bug 570893 - Only re-enable the form controls when reload, not re-disable it,

https://reviewboard.mozilla.org/r/101484/#review102014

It think it would be clearer to do SetDisabled(false) given the new condition, to make it clear what's really happening there.

r=me with that.
Attachment #8822616 - Flags: review?(bzbarsky) → review+
Assignee: catalin.badea392 → timdream
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Keywords: checkin-needed

Comment 24

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/17caae57fcd0
Only re-enable the form controls when reload, not re-disable it, r=bz
Keywords: checkin-needed

Comment 25

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/17caae57fcd0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: qe-verify+
Reproduced the issue on 53.0a1 (2016-12-27). 
The bug is verified fixed on 53.0b1 build1 (20170307064827), using Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x86.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.