Closed Bug 218297 Opened 21 years ago Closed 21 years ago

disabled property of input elements is not stored in history

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: keeda, Assigned: keeda)

Details

Attachments

(3 files, 5 obsolete files)

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.
Attached file Silly little testcase
Pick Yes to disable the input element. Hit Sumbit. Then hit the back button.
The input element will not be disabled.
This seems to do what I want.
Attached patch This appears do what I want (obsolete) — Splinter Review
Attached patch This seems to do what I want (obsolete) — Splinter Review
Attachment #130908 - Flags: superreview?(jst)
Attachment #130908 - Flags: review?(john)
Sorry for the multiple attachments. I dont know what exactly happened there.
Summary: disabled property of input elements is not stored in history → disabled property of input elements is not stored in history
Comment on attachment 130904 [details]
Silly little testcase

whoops.
Attachment #130904 - Attachment is patch: false
Attachment #130904 - Attachment mime type: text/plain → text/html
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.
Attachment #130908 - Flags: superreview?(jst) → superreview+
Attached patch fixed jst's issues (obsolete) — Splinter Review
Attachment #130905 - Attachment is obsolete: true
Attachment #130907 - Attachment is obsolete: true
Attachment #130908 - Attachment is obsolete: true
Comment on attachment 130908 [details] [diff] [review]
This seems to do what I want

cancelling obsolete request
Attachment #130908 - Flags: review?(john)
Attachment #131213 - Flags: superreview+
Attachment #131213 - Flags: review?(john)
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.
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #131213 - Attachment is obsolete: true
Attachment #131213 - Flags: review?(john)
Attachment #131752 - Flags: review?(john)
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
Attachment #131752 - Flags: review?(john) → review-
Attached patch v3Splinter Review
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.)
Attachment #131752 - Attachment is obsolete: true
Attachment #131801 - Flags: review?(john)
Comment on attachment 131801 [details] [diff] [review]
v3

Looks good.  Thanks!
Attachment #131801 - Flags: review?(john) → review+
Attachment #131801 - Flags: superreview?(jst)
Comment on attachment 131801 [details] [diff] [review]
v3

sr=jst
Attachment #131801 - Flags: superreview?(jst) → superreview+
This has r/sr...isn't it ready to land now that the tree is open for checkins?
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. 
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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. 
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.
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.
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: