218297 - disabled property of input elements is not stored in history

Status: RESOLVED FIXED

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

Description

[Harshal Pradhan]

If some page changes disabled through js, you move on to some other page, and
then hit back, then we don't and restore the input element to the old state. My
copy of IE6 does what I would expect in this case. I think we should fix this up.

Comment 1

[Harshal Pradhan]

Attachment: Silly little testcase

Pick Yes to disable the input element. Hit Sumbit. Then hit the back button.
The input element will not be disabled.

Comment 2

[Harshal Pradhan]

Attachment: Store disabled in history like other properties

This seems to do what I want.

Comment 3

[Harshal Pradhan]

Attachment: This appears do what I want

Comment 4

[Harshal Pradhan]

Attachment: This seems to do what I want

Comment 5

[Harshal Pradhan]

Sorry for the multiple attachments. I dont know what exactly happened there.

Comment 6

[Harshal Pradhan]

Comment on attachment 130904 [details]
Silly little testcase

whoops.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 130908 [details] [diff] [review]
This seems to do what I want

- In nsHTMLInputElement::SetDisabled(PRBool aDisabled):

+  if (aDisabled) {
+    nsHTMLValue empty(eHTMLUnit_Empty);
+    return SetHTMLAttribute(nsHTMLAtoms::disabled, empty, PR_TRUE);
+  }
+  else {
+    UnsetAttr(kNameSpaceID_None, nsHTMLAtoms::disabled, PR_TRUE);
+    return NS_OK;
+  }
+}

Unnecessary else-after-return, remove it.

- In nsHTMLInputElement::SaveState():

+  if (GET_BOOLBIT(mBitField, BF_DISABLED_CHANGED)) {
+    rv = GetPrimaryPresState(this, getter_AddRefs(state));
...
   return rv;
 }

You're stomping over rv there (and in a few more places inside that if block),
use another local (or use |= when assigning into rv).

sr=jst with the above fixed.

Comment 8

[Harshal Pradhan]

Attachment: fixed jst's issues

Comment 9

[Harshal Pradhan]

Comment on attachment 130908 [details] [diff] [review]
This seems to do what I want

cancelling obsolete request

Comment 10

[John Keiser (jkeiser)]

Attachment: Sillier little testcase

I may just be totally off my rocker, but it seems to me that the code here
unconditionally restores the disabled state, regardless of whether it was
saved.	The existing code does that for value and checked, but since there was
only one state property a state object would not have existed if we didn't want
to restore the value.

So here's a scenario: what if disabled *starts* as true but does not change,
and then you go back?  It seems like this will end up setting disabled to false
since there was no value saved for disabled.

I suppose a history state object would have to exist, so I have made this
testcase modify the value of the input.

NOTE: I am dubious about whether to really save disabled, but I'll allow it
since we seem to want to move to a "save the DOM" model instead of this
space-saving but much more complex and error-prone "save only the things we
think are important" model.

Comment 11

[Harshal Pradhan]

Attachment: v2

This should fix the problems that jkeiser pointed out. I had overlooked the
fact that this code relied upon only one property being set per input element,
and having state only when it is set.

Comment 12

[John Keiser (jkeiser)]

Comment on attachment 131752 [details] [diff] [review]
v2

Final remaining problem: if you have <input name="blah" value="something"> and
the user deleted all the text in the 

There are two possible ways I can see to deal with this problem: (a) create
nsIPresState::HasStateProperty(), or (b) give GetStateProperty() multiple
return values like getAttr(): in other words, have an equivalent of
NS_CONTENT_ATTR_HAS_VALUE and NS_CONTENT_ATTR_NOT_THERE (perhaps even use those
exact values).

I prefer the second one personally, but I won't knock it if you do the first.

The current definition of NS_CONTENT_ATTR_*:
http://lxr.mozilla.org/mozilla/source/content/base/public/nsContentErrors.h#44

An example of GetAttr returning said values:
http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericElement.cpp#355
4

An example usage:
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLInputEleme
nt.cpp#1046

Comment 13

[Harshal Pradhan]

Attachment: v3

Yep, that case wouldn't have worked. 

I've done b) as you suggested. I introduced new errors in nsLayoutErrors.h.
(Feel free to suggest better names.) Re-using the content attribute error
values seemed like the wrong thing to do. Also, in this case, it looks like we
don't really need the equivalent of NS_CONTENT_ATTR_NO_VALUE.

I had a brief look at the other callers of GetStateProperty() and it didn't
appear like any of them would be affected by this change. (but I need to make
sure.)

Comment 14

[John Keiser (jkeiser)]

Comment on attachment 131801 [details] [diff] [review]
v3

Looks good.  Thanks!

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 131801 [details] [diff] [review]
v3

sr=jst

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

This has r/sr...isn't it ready to land now that the tree is open for checkins?

Comment 17

[Harshal Pradhan]

Yeah, I need to land this. Haven't had a build environment or time to watch the
tree for the last few weeks. I'll land it this weekend. 

Comment 18

[Harshal Pradhan]

Checked in.

Comment 19

[Martijn Wargers (dead)]

I'm really surprised at this.
From comment 0:
"If some page changes disabled through js, you move on to some other page, and
then hit back, then we don't and restore the input element to the old state."
This is exactly what my copy of IE6 is doing, but apparently not what reporter's
IE6 version is doing?
Has IE6's behavior changed since this was fixed? Or do I have a strange IE6 version?
This fix might have caused bug 300364. 

Comment 20

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b3) Gecko/20050710
Firefox/1.0+ ID:2005071017
Win2k + sp4 + rollup package 1
IE 6.0.2800.1106

In IE: Going to the 'Silly little testcase' and changing 'Disabled?' to YES
doesn't grey out the textbox like firefox. However, the textbox is disabled
(can't get a caret in it). On hitting 'submit', then 'back' the 'Disabled?' says
YES but the textbox accepts a caret now.

In firefox, on hitting back the 'Disabled' is displaying YES, and the textbox IS
greyed out (disabled).

So from what I can see, firefox is doing the right thing, and IE isn't.

Comment 21

[Martijn Wargers (dead)]

Thanks, Steve. So IE6 isn't doing what is stated in comment 0 of this bug, and
the fix from this bug makes Mozilla act different regarding to IE6.

I prefer IE6's behavior, it makes more sense to me.
The fix here is in fact incomplete, since it only touches input's and not
textareas and selects.
Note that with bfcache enabled, this is a non-issue, since you always get the
saved dom state on pressing back.