570893 - Disabled state of form control persists across page reload

Status: VERIFIED FIXED

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

Description

[Jason Heeris]

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.

Comment 1

[Jason Heeris]

Attachment: Simple XHTML page exhibiting the problem.

Comment 2

[Erik Rose [:erik][:erikrose]]

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.

Comment 3

[Ioana (away)]

Reproducible on:
Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20130724 Firefox/25.0

Comment 4

[Till Schneidereit [:till]]

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.

Comment 5

[Erik Rose [:erik][:erikrose]]

Is the disabled-ness of a control really part of form value?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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)

Comment 8

[Andrew Overholt [:overholt]]

(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.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 ...

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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"?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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...

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[Cătălin Badea (:catalinb)]

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?

Comment 17

[Boris Zbarsky [: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.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

That doesn't seem unreasonable.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 570893 - Only re-enable the form controls when reload, not re-disable it,

With this patch, the disabled state is still kept in the nsPresState,
but we will only honor that if the state saved asks as to re-enable the
control.

The behavior is changed so that controls disabled by JavaScript will be
kept enabled as the JavaScript world gets reloaded.

Review commit: https://reviewboard.mozilla.org/r/101484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101484/

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't know what tests to fix, or if is any.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbdf39440aceece4102ec523332fcd1388f24d7e

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8822616 [details]
Bug 570893 - Only re-enable the form controls when reload, not re-disable it,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101484/diff/1-2/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17caae57fcd0
Only re-enable the form controls when reload, not re-disable it, r=bz

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/17caae57fcd0

Comment 26

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.